-
Notifications
You must be signed in to change notification settings - Fork 365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add local dev experience for scheduled functions #3689
Conversation
📊 Benchmark resultsComparing with 40b1cf9 Package size: 360 MB⬆️ 0.01% increase vs. 40b1cf9
Legend
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Left a few minor comments.
93878c1
to
3fa4c14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Once we bring this branch up-to-date and get the tests passing, I think we're good to go.
36b6c20
to
c14699b
Compare
77ef972 updates to the prerelease of netlify/zip-it-and-ship-it#899 (comment) to move this PR forward |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments, none of them blocking for the beta launch.
let headers = {} | ||
const headers = { | ||
'user-agent': 'netlify-cli', | ||
} | ||
let body = {} | ||
|
||
if (eventTriggeredFunctions.has(functionToTrigger)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think scheduled functions should fall on this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is addressed by c61a214#diff-9664db1fa0286aca02effcde71a026913114080a1c960e2257a357718ebb41eeR165-R171
we decided to move forward with the change. let's get this shipped! 🚢 @eduardoboucas @lukasholzer would love another review :) |
Something is killing the tests: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! Nice PR 🐳, some small nit pick
@@ -129,6 +130,13 @@ const getFunctionToTrigger = function (options, argumentName) { | |||
return argumentName | |||
} | |||
|
|||
const getNextRun = function (schedule) { | |||
const cron = CronParser.parseExpression(schedule, { | |||
tz: 'Etc/UTC', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we document that somewhere that we rely here un UTC and not on the users timezone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will be document in the Labs docs.
|
||
const handleScheduledFunction = ({ error, request, response, result }) => { | ||
const acceptsHtml = request.headers.accept && request.headers.accept.includes('text/html') | ||
const paragraph = (text) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I think you can move this out of this function as you don't need the outer context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/lib/functions/scheduled.js
Outdated
|
||
const { formatLambdaError } = require('./utils') | ||
|
||
const handleScheduledFunction = ({ error, request, response, result }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please write a small unit test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
6015fd6
Co-authored-by: Lukas Holzer <lukas.holzer@netlify.com>
@lukasholzer do you have an idea what's causing this? do we know if it's a problem with the PR, or an external thing? |
My first wild guess would be that it is somehow connected with the PR as the error is somehow related to functions and I've never seen this one before. So if tests are flaky then they are failing differently (by http endpoint timed out or something like that). This one seems to be more on the mem crashing side of things on macos :D while the ubuntu-latest test seems to be something different but related to scheduled functions: |
@netlify-team-account-1 do they pass locally? |
co-authored-by: Lukas Holzer <lukas.holzer@netlify.com>
@Skn0tt still not passing? 😬 |
nope |
🎉 Thanks for submitting a pull request! 🎉
Summary
This PR adds special behaviour for scheduled functions. To mimic planned production behaviour, the local dev environment will not accept HTTP calls to scheduled functions. It will accept calls made through
ntl functions:invoke
.A picture of a cute animal (not mandatory, but encouraged)