-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix: ISR opt out for routes with rest parameters #373
fix: ISR opt out for routes with rest parameters #373
Conversation
🦋 Changeset detectedLatest commit: 939cbe7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
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 |
4ed8836
to
77d4bfd
Compare
packages/vercel/src/lib/redirects.ts
Outdated
.replace(/[*+?^${}()|[\]\\]/g, '\\$&'); | ||
}) | ||
.join(''); | ||
: (segmentIndex === 0 ? '' : '/') + |
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 noticed this check differs from the core implementation. I believe you said this address the case where a spread param lacks a leading slash (e.g. /path[...spread]
). Is there a reason Vercel needs special handling?
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.
@bholmesdev great question!
Not exactly. This a replacement for join('/')
defined here:
.join('/'); |
Combined with the check
segment.length === 1 && segment[0].spread
above, it means "join all route segments with '/'
, but omit it if the segment is a spread" (e.g. "/path/[...spread]").
And the case you are describing is handled here (I should probably cover it with tests as well):
? '(.*?)' |
Initially I copied the core implementation of getPattern()
method and tried to use that, but I realized that it would require more code changes around redirects. I've tried to achieve the goal without changing the interface of getMatchPattern()
function. If you think that the correct way is to copy getPattern()
from core and adjust the rest of the Vercel adapter code accordingly, let me know!
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.
Got it. I can see why we took this route to avoid a larger refactor. I think that's the right move for this PR! I'd consider exposing a utility for this from astro
core to consolidate code in the future.
Changes
The logic for generating a match pattern (getMatchPattern) was initially copied from the core astro repository. Since then, the implementation in Astro Core was changed a couple of times. Here is the current implementation of this function in Astro Core: https://github.com/withastro/astro/blob/cd54210/packages/astro/src/core/routing/manifest/pattern.ts#L3.
Looking into
.vercel/output/config.json
, it was found that the route generated for the excluded path contains an extra slash. Here is an example for a"/excluded/[...rest]"
route:However, it should look like this:
getMatchPattern()
so that it doesn't put a redundant slash before a route segment with rest/spread parameters.getMatchPattern()
so that it also works for route segments that includes the rest parameter not in the beginning of the segment (e.g./one/two[...three]
).Testing
Was tested by:
@astro/vercel
package to the example project..vercel/output/config.json
an making sure that the route with the rest parameter is added with the"_render"
dest and the"src"
regex is no longer contains an extra prepending slash.Docs
No need to change the docs because it already mentions this case: