feat: support after() #2717
Merged
feat: support after() #2717
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pieh
force-pushed
the
feat/next-after
branch
2 times, most recently
from
d3d3f98 to
5cc78cf
Compare
Contributor
📊 Package size report 0.03%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
force-pushed
the
feat/next-after
branch
6 times, most recently
from
a86b52d to
a5e4dc4
Compare
pieh
force-pushed
the
feat/next-after
branch
2 times, most recently
from
cc72e89 to
1cd01a5
Compare
…ions installation time making it timeout
pieh
commented
Dec 6, 2024
Comment on lines
+132
to
+135
| // Next.js relies on `close` event emitted by response to trigger running callback variant of `next/after` | ||
| // however @fastly/http-compute-js never actually emits that event - so we have to emit it ourselves, | ||
| // otherwise Next would never run the callback variant of `next/after` | ||
| res.emit('close') |
Contributor
Author
There was a problem hiding this comment.
This is to support callback variant of after():
after(async () => {
// this will not start executing immediately - instead this function will be stored to and start executing after response was ~sent
// make sense to not compete for any resources while working on response and delay executing this function to after that
})
This is not needed to support promise variant like so:
after(new Promise(resolve => {
// this starts executing immediately and will ensure it will finish as long
// as function timeout is not reached
})
pieh
commented
Dec 6, 2024
| }) | ||
| test( | ||
| 'yarn monorepo multiple next versions site is compatible', | ||
| { retry: 0, timeout: 1_000 * 60 * 5 }, |
Contributor
Author
There was a problem hiding this comment.
this change just bumps/sets timeout - we have 3 extensions being installed which we don't even use because site is in Netlify-testing account and that's pretty slow and this test case in particular was already slow before, but those 2 things combined meant that it was always timing out now
pieh
commented
Dec 6, 2024
mrstork
reviewed
Dec 6, 2024
mrstork
reviewed
Dec 6, 2024
pieh
added
the
test all versions
orinokai
approved these changes
Dec 9, 2024
Contributor
orinokai
left a comment
orinokai
left a comment
There was a problem hiding this comment.
Looks good to me. Feels nicely abstracted in the createRequestContext function 👍🏼
mrstork
approved these changes
Dec 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This provides
waitUntilimplementation that Next.js will use forafter()feature. Ifcontext.waitUntilis defined (it's conditionally available now depending on rollout) we will use just that. If it is not defined - we will use our TransformStream trick with keeping response open while awaitingafter()(and other background work) to finish.Documentation
Tests
Test added as second commit - fails without making any changes and work with code changes.
The test use similar setup as Next.js e2e
aftertest - https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/next-after-app-deploy/index.test.ts and uses 2 pages:/after/check- cacheable page that renders timestamp/after/triggerwhich is SSR that usesafter()to invalidate/after/checkpageFirst we hit the
/after/checkand will get immediately stale content and regenerate it in background getting fresh timestampThen we hit that page twice again making sure we get stable timestamp in both hits
Then we hit
/after/triggerwhich should invalidate/after/checkifafter()is allowed to fully executeThen we hit
/after/checkagain and compare timestamp to timestamps we saw before invalidating it - timestamp now should be greater than beforeRelevant links (GitHub issues, etc.) or a picture of cute animal
Closes #2695