Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add schedule helper #226

Merged
merged 2 commits into from
Jan 21, 2022
Merged

feat: add schedule helper #226

merged 2 commits into from
Jan 21, 2022

Conversation

netlify-team-account-1
Copy link
Contributor

@netlify-team-account-1 netlify-team-account-1 commented Nov 30, 2021

Which problem is this pull request solving?

This PR adds a schedule marker function that's used for ISC detection.

Describe the solution you've chosen

We decided to make schedule a no-op.
This requires developers to explicitly return { statusCode: 200 } or { statusCode: 500 }, which is redundant & verbose given the usecase.
There is potential for simplified behaviour with the schedule helper, but we'd need to bring the netlify.toml-configured behaviour in-line with that first.

@netlify-team-account-1 netlify-team-account-1 added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Nov 30, 2021
* @param schedule expressed as cron string. see https://crontab.guru for help.
* @param handler
* @see https://docs.netlify.com/functions/<schedule>
* @tutorial https://announcement-blogpost
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two would obvsly be replaced with actual links, or ntl.fyi shortlinks

* @tutorial https://announcement-blogpost
*/
const schedule =
(cron: string, handler: HandlerWithoutResponse): Handler =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cron seems like a good candidate for a template literal type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was also thinking that! I didn't include it in the first draft since it would require all consumers to use a TypeScript version that supports template literal types (introduced in 4.1). I couldn't find any distribution information to quantify this, though ... Do you think that's a good tradeoff to make?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a policy for minimum TS version? I'd think a year-old one would be ok

Copy link
Contributor Author

@netlify-team-account-1 netlify-team-account-1 Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried translating this EBNF to a template literal string type.

Toggle to see resulting TypeScript

// adapted from this ebnf:
// https://docs.atlassian.com/caesium-parent/1.3.0/apidocs/com/atlassian/scheduler/caesium/cron/parser/CronExpressionParser.html

type NUMBER = 'NUMBER'
type NAME = 'NUMBER'

type SlashInterval = `/${NUMBER}`
type NumberRange = `${NUMBER}-${NUMBER}${SlashInterval | ''}`
type NameRange = `${NAME}-${NAME}${SlashInterval | ''}`
type NumberExpression = `${NumberRange}${SlashInterval | ''}` | `${NUMBER}${SlashInterval}`
type NameExpression = NameRange | `${NAME}${SlashInterval | ''}`

type AsteriskExpression = `*${SlashInterval | ''}`

type SimpleExpression = NameExpression | NumberExpression | AsteriskExpression | SlashInterval // what is (* implied asterisk *) ?

type SimpleField =
  | SimpleExpression
  | `${SimpleExpression},${SimpleExpression}`
  | `${SimpleExpression},${SimpleExpression},${SimpleExpression}`
  | `${SimpleExpression},${SimpleExpression},${SimpleExpression},${SimpleExpression}`

type SpecialDomLastOffsetNumber = NUMBER | `${NUMBER}W`
type SpecialDomLastOffset = SpecialDomLastOffsetNumber
type SpecialDomLastWeekday = 'W'

type SpecialDomLast = `L-${SpecialDomLastOffset}` | `L${SpecialDomLastWeekday}` | 'L'

type SpecialDomWeekday = 'W'

type SpecialDomField = SpecialDomLast | SpecialDomWeekday

type SecondField = SimpleField
type MinuteField = SimpleField
type HourField = SimpleField
type DomField = SpecialDomField | SimpleField
type MonthField = SimpleField
type YearField = '' | SimpleField | `${SimpleField} ${string}`

type SecondMinuteHour = `${SecondField} ${MinuteField} ${HourField}`
type QmMonthDow = `? ${MonthField} ${DowField}`

type DomMonthQm = `${DomField} ${Monthfield} ?`
type DomMonthDow = QmMonthDow | DomMonthQm

type CronExpressionWithoutYear = `${SecondMinuteHour}${DomMonthDow}`
export type CronExpression = CronExpressionWithoutYear | `${CronExpressionWithoutYear} ${YearField}`

Sadly, that seems to be complex of an expression for TypeScript :(

❯ npm run build

> @netlify/functions@0.10.0 build
> tsc

src/lib/cron.ts:40:25 - error TS2590: Expression produces a union type that is too complex to represent.

40 type SecondMinuteHour = `${SecondField} ${MinuteField} ${HourField}`
                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/lib/cron.ts:41:38 - error TS2552: Cannot find name 'DowField'. Did you mean 'DomField'?

41 type QmMonthDow = `? ${MonthField} ${DowField}`
                                        ~~~~~~~~

src/lib/cron.ts:41:38 - error TS4081: Exported type alias 'QmMonthDow' has or is using private name 'DowField'.

41 type QmMonthDow = `? ${MonthField} ${DowField}`
                                        ~~~~~~~~

src/lib/cron.ts:43:34 - error TS2552: Cannot find name 'Monthfield'. Did you mean 'MonthField'?

43 type DomMonthQm = `${DomField} ${Monthfield} ?`
                                    ~~~~~~~~~~

src/lib/cron.ts:43:34 - error TS4081: Exported type alias 'DomMonthQm' has or is using private name 'Monthfield'.

43 type DomMonthQm = `${DomField} ${Monthfield} ?`
                                    ~~~~~~~~~~

src/lib/cron.ts:47:58 - error TS2590: Expression produces a union type that is too complex to represent.

47 export type CronExpression = CronExpressionWithoutYear | `${CronExpressionWithoutYear} ${YearField}`
                                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Found 6 errors.

Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to give the handler the return type Promise<void> to signal that this is a cron job, thus the response won't be observed by any customer logic.

I understand the rationale for this, but I think we need to take into consideration the fact that the schedule helper is not the only entry point into scheduled functions. If someone prefers to define a schedule via netlify.toml, their function handler will not have this modification you're introducing and I think that discrepancy can make things harder to debug.

I would be more in favour of keeping the return value unaltered for now, and then we can discuss whether we want to do any modifications at the invocation level, which would apply to all functions, regardless of their runtime or the method used to define the schedule.

@netlify-team-account-1
Copy link
Contributor Author

Agreed. Implemented in 7f93582, updated the PR description to reflect that.

@netlify-team-account-1 netlify-team-account-1 marked this pull request as ready for review December 14, 2021 18:12
@netlify-team-account-1
Copy link
Contributor Author

published @netlify/functions@v0.11.0-rc for testing.

@netlify-team-account-1 netlify-team-account-1 merged commit ae10749 into main Jan 21, 2022
@netlify-team-account-1 netlify-team-account-1 deleted the schedule-helper branch January 21, 2022 15:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants