-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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(vercel): ISR #9714
feat(vercel): ISR #9714
Conversation
🦋 Changeset detectedLatest commit: 44d4ced The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
d3d25b4
to
a9cc77c
Compare
!preview isr |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
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.
Great work! I'd love at least one other approval before we merge this given what a big feature this is!
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.
Great work! I'd love at least one other approval before we merge this given what a big feature this is!
Co-authored-by: Nate Moore <[email protected]>
// isr functions do not have access to search params, this middleware removes them for the dev mode | ||
if (isr) { | ||
const exclude_ = typeof isr === "object" ? isr.exclude ?? [] : []; | ||
const exclude = exclude_.concat("/_image").map(ex => new RegExp(ex)); |
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.
Instead of a regex expression, should we be a little accurate here? Looks like if it's passed ['/foo']
, it could also match requests for /other/foo
.
If we want to allow marking entire directories to exclude, maybe we can accept (string | RegExp)[]
instead?
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.
+1 from me too. Also, if a user passes a string, we should do an equality check, and not create a regex.
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 agree we should be stricter.
It's a RegExp here because that's how vercel processes the routes file.
I'll look into making the exclude paths behave as literal paths.
isr, | ||
}); | ||
} | ||
if (isr === false || (isr && typeof isr === "object" && isr.exclude)) { |
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'm a bit confused about this check, should this be an else
block because we already check isr
above?
I'm not sure why this is checking isr.exclude
specifically to create another function folder which could've been already generated above.
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.
The condition here checks whether a normal function needs to be created. Even when ISR is enabled, the excluded paths would need to create one.
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.
Cleaned up. Here is the new control flow
/** | ||
* Expiration time (in seconds) before the pages will be re-generated. | ||
* | ||
* By default, as long as the current deployment is in production. |
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 don't understand this phrase. By default, what? It's not a boolean, so the default should be a number
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.
The default javascript value is false, but that just doesn't communicate anything.
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.
That's false because the parent object is optional, right? But if I pass an object, this field must have a default as stated in the docs. Which means I don't understand what's the default value in numbers of this option.
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.
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.
New lines 146-153.
revalidate?: number; | ||
|
||
/** | ||
* Paths that will always be served fresh. |
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 would add a longer documentation here. From the implementation, I inferred that paths can be regex, but here I couldn't figure it out. I thought only strings were accepted.
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.
That is correct. Only strings are accepted. Vercel would interpret the string as a regex but that's a vercel implementation detail. I will look into adding a processing step so that they really only ever act like strings.
// isr functions do not have access to search params, this middleware removes them for the dev mode | ||
if (isr) { | ||
const exclude_ = typeof isr === "object" ? isr.exclude ?? [] : []; | ||
const exclude = exclude_.concat("/_image").map(ex => new RegExp(ex)); |
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.
+1 from me too. Also, if a user passes a string, we should do an equality check, and not create a regex.
Converting to draft because there's something about routing I need to confirm after these changes, but I can't today. Update: I had erika confirm - |
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 good!
I was asked to review the typed documentation here, and it's looking pretty good! Right now, there's nothing here that would show up in docs itself, nor a changeset to edit. But, I'd say this is a good basis for API docs. Carry on! 🫡 |
Oh, didn't even realise I missed the changeset. |
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.
Thanks @lilnasy ! Looking good! Here are my initial thoughts!
Also noting: there is more info here than you put in the actual docs PR. So... all this stuff should end up over there!
Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
route-specific configurationout of scopeUsage
Basic usage - enough for 80% of the users - caches all pages on first request and saves forever
Time based invalidation - caches all pages on first request and saves for 1 day
Manual invalidation - caches all pages on first request and lets you create an endpoint where you clear it
Testing
Added isr.test.js with a fixture
Docs