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

Path to regexp fix #11965

Closed
wants to merge 6 commits into from
Closed

Path to regexp fix #11965

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f4fa4d1
Updated packages. path-to-regexp to latest version
hkbertoson Sep 10, 2024
02a0340
Changeset
hkbertoson Sep 10, 2024
45b7e2a
Updated generator function
hkbertoson Sep 10, 2024
fd0c086
Update generator.ts
hkbertoson Sep 11, 2024
0e07ae7
Refactor parameter sanitization and array preservation
hkbertoson Sep 11, 2024
a06cfaa
Merge branch 'main' into path-to-regexp-fix
hkbertoson Sep 13, 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/afraid-scissors-explain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': minor
---

Updated path-to-regexp
2 changes: 1 addition & 1 deletion packages/astro/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@
"ora": "^8.1.0",
"p-limit": "^6.1.0",
"p-queue": "^8.0.1",
"path-to-regexp": "^6.2.2",
"path-to-regexp": "^8.1.0",
"preferred-pm": "^4.0.0",
"prompts": "^2.4.2",
"rehype": "^13.0.1",
Expand Down
8 changes: 5 additions & 3 deletions packages/astro/src/core/routing/manifest/generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { AstroConfig, RoutePart } from '../../../@types/astro.js';

import { compile } from 'path-to-regexp';

/**
Expand All @@ -9,11 +8,14 @@ import { compile } from 'path-to-regexp';
*/
function sanitizeParams(
params: Record<string, string | number | undefined>,
): Record<string, string | number | undefined> {
): Record<string, string | undefined> {
return Object.fromEntries(
Object.entries(params).map(([key, value]) => {
if (typeof value === 'string') {
return [key, value.normalize().replace(/#/g, '%23').replace(/\?/g, '%3F')];
} else if (typeof value === 'number') {
// Explicitly convert numbers to strings
return [key, String(value)];
}
return [key, value];
}),
Expand All @@ -31,7 +33,7 @@ export function getRouteGenerator(
segment
.map((part) => {
if (part.spread) {
return `:${part.content.slice(3)}(.*)?`;
return `${part.content.slice(3)}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes this part from being a parameter understood by path-to-regexp to just being a literal, e.g. if the route was [...slug], previously this template literal created :slug(.*) but after this change it just creates slug. That’s probably why a lot of tests are failing.

We’ll need to check how to match the (.*) behaviour in the latest version of path-to-regexp (if possible).

Copy link
Member

@delucis delucis Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From taking a look at the docs, it seems like this might be the way?

Suggested change
return `${part.content.slice(3)}`;
return `*${part.content.slice(3)}`;

In a quick sandbox I spun up I ran this code:

const { compile } = require('path-to-regexp');
const toPath = compile('/*spread/:slug');
const path = toPath({ spread: ['one','two','three'], slug: 'four' });
console.log(path);

Which logged /one/two/three/four as expected. We’d still need to see if it fully matches all behaviour specified in tests though or if there are edge cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this as well. It errors out and says "expected "slug" to be a non-empty array"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I guess currently we allow [...spread] to match nothing (i.e. [...spread] can match /) and *spread in path-to-regexp doesn’t include support for an optional segment. They do mention {} for optional segments, could try combining that with the wildcard?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then there is still the issue with the actual parameter value for rest (...) parameters, today it's "somedir/file.ext" and with the new version it should be ["somedir", "file.ext"], i.e. the parameter value needs to be split.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, when I did a quick investigation it looked like path-to-regexp already expected an array in v6, so assumed that hadn’t changed, but I may have misunderstood.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I made some changes and im testing it all now to see.

} else if (part.dynamic) {
return `:${part.content}`;
} else {
Expand Down
13 changes: 6 additions & 7 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading