Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: ISR opt out for routes with rest parameters #373

Merged
merged 5 commits into from
Oct 7, 2024
Merged

fix: ISR opt out for routes with rest parameters #373

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
602c90e
Fix match pattern for segments with spreads
hrabiel Sep 2, 2024
77d4bfd
Add changeset
hrabiel Sep 2, 2024
2625aa3
Fix code formatting
hrabiel Sep 3, 2024
2c1cfe6
edit: add comment on leading slash implementation
bholmesdev Sep 10, 2024
939cbe7
Fix formatting
hrabiel Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/early-scissors-wait.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/vercel': patch
---

Fix excluding routes with rest parameters from ISR
29 changes: 16 additions & 13 deletions packages/vercel/src/lib/redirects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,25 +45,28 @@ function getParts(part: string, file: string) {
// 2022-04-26
function getMatchPattern(segments: RoutePart[][]) {
return segments
.map((segment) => {
return segment[0].spread
.map((segment, segmentIndex) => {
return segment.length === 1 && segment[0].spread
? '(?:\\/(.*?))?'
: segment
: (segmentIndex === 0 ? '' : '/') +
Copy link
Contributor

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?

Copy link
Contributor Author

@hrabiel hrabiel Sep 3, 2024 •
edited
Loading

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:

adapters/packages/vercel/src/lib/redirects.ts

Line 66 in 2246e6e

.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):

adapters/packages/vercel/src/lib/redirects.ts

Line 56 in 2625aa3

? '(.*?)'

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!

Copy link
Contributor

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.

bholmesdev marked this conversation as resolved.
Show resolved Hide resolved
segment
.map((part) => {
if (part)
return part.dynamic
? '([^/]+?)'
: part.content
.normalize()
.replace(/\?/g, '%3F')
.replace(/#/g, '%23')
.replace(/%5B/g, '[')
.replace(/%5D/g, ']')
.replace(/[*+?^${}()|[\]\\]/g, '\\$&');
return part.spread
? '(.*?)'
: part.dynamic
? '([^/]+?)'
: part.content
.normalize()
.replace(/\?/g, '%3F')
.replace(/#/g, '%23')
.replace(/%5B/g, '[')
.replace(/%5D/g, ']')
.replace(/[*+?^${}()|[\]\\]/g, '\\$&');
})
.join('');
})
.join('/');
.join('');
}

function getReplacePattern(segments: RoutePart[][]) {
Expand Down
2 changes: 1 addition & 1 deletion packages/vercel/test/fixtures/isr/astro.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export default defineConfig({
isr: {
bypassToken: "1c9e601d-9943-4e7c-9575-005556d774a8",
expiration: 120,
exclude: ["/two", "/excluded/[dynamic]"]
exclude: ["/two", "/excluded/[dynamic]", "/excluded/[...rest]"]
}
})
});
8 changes: 8 additions & 0 deletions packages/vercel/test/fixtures/isr/src/pages/excluded/[...rest].astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<html>
<head>
<title>Rest</title>
</head>
<body>
<h1>Rest</h1>
</body>
</html>
8 changes: 8 additions & 0 deletions packages/vercel/test/isr.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ describe('ISR', () => {
src: '^/excluded/([^/]+?)$',
dest: '_render',
},
{
src: '^/excluded(?:\\/(.*?))?$',
dest: '_render',
},
{
src: '^\\/_image$',
dest: '_render',
Expand All @@ -45,6 +49,10 @@ describe('ISR', () => {
src: '^\\/excluded\\/([^/]+?)\\/?$',
dest: '/_isr?x_astro_path=$0',
},
{
src: '^\\/excluded(?:\\/(.*?))?\\/?$',
dest: '/_isr?x_astro_path=$0',
},
{
src: '^\\/one\\/?$',
dest: '/_isr?x_astro_path=$0',
Expand Down
Loading