Fix leading slash creating incorrect conflict resolution between pages generated from static routes and rest parameters

[frankbits]

Changes

Following the PR #10298 to fix issue #9103, my specific problem was still not fixed, as mentioned in #9103 (comment).
In #9103 (comment) I explained my following findings:

• having a "static path"-object without a leading slash works fine (trailing slash is fine):

{
	slug: 'test/ing/',
	title: 'Rest TestIng',
},

• but adding a trailing slash if a Rest-parameter can generate the same page as a static page, it will overwrite it in build-mode, while everything works as expected in dev-mode:

{
	slug: '/test/ing/',
	title: 'Rest TestIng',
},

• this is related to the generated route being //test/ing/
  • first hotfix was to just replace leading // with / frankbits@737d407
• to fix this, I replaced the direct call to route.generate() with stringifyParams() which calls route.generate() internally, after trimming slashes frankbits@78d604c
  • I also removed the JSON.stringify() of the return-value of stringifyParams(), as the returned value of route.generate() is already a string, and I had to parse it back to a string without "" and the other 2 places where the function is used should not have any problem anywhere without the JSON.stringify(), as the return-value i used as an array-key to store/retrieve "static path"-objects and only have to fit to each other.

Testing

I added frankbits@a913d04 ( edited frankbits@7cba77b, frankbits@22fb67e) a test with the slug having a leading slash (similar to the failing path in my MRE of my original issue #9103)

Also added a test for leading slash in a dynamic route d4bd99e (see #10607 (comment))

All test in packages/astro/test/dynamic-route-collision.test.js finish successfully.

I couldn't run all test of the project, as pnpm run test ran into errors setting up tests.

@astrojs/telemetry:test: cache miss, executing cfa2dc085a22837a
@astrojs/telemetry:test: 
@astrojs/telemetry:test: > @astrojs/telemetry@3.0.4 test C:\Users\Frank\PhpstormProjects\astro\packages\telemetry
@astrojs/telemetry:test: > astro-scripts test "test/**/*.test.js"
@astrojs/telemetry:test: 
@astrojs/telemetry:test: node:internal/errors:490
@astrojs/telemetry:test:     ErrorCaptureStackTrace(err);
@astrojs/telemetry:test:     ^
@astrojs/telemetry:test: 
@astrojs/telemetry:test: Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:test/reporters
@astrojs/telemetry:test:     at new NodeError (node:internal/errors:399:5)
@astrojs/telemetry:test:     at ESMLoader.builtinStrategy (node:internal/modules/esm/translators:259:11)
@astrojs/telemetry:test:     at ESMLoader.moduleProvider (node:internal/modules/esm/loader:468:14) {
@astrojs/telemetry:test:   code: 'ERR_UNKNOWN_BUILTIN_MODULE'
@astrojs/telemetry:test: }
@astrojs/telemetry:test: 
@astrojs/telemetry:test: Node.js v18.15.0
@astrojs/telemetry:test: ERROR: command finished with error: command (C:\Users\Frank\PhpstormProjects\astro\packages\telemetry) C:\Users\Frank\AppData\Roaming\npm\pnpm.cmd run test exited (1)

Docs

/cc @withastro/maintainers-docs for feedback!

I have not updated the docs, as my change is not in conflict with the documented behaviour.
But it could probably be sensible to add information to the routing-doc about leading and trailing slashes in dynamic and rest-parameters. (as mentioned in #9103 (comment))

[changeset-bot]

🦋 Changeset detected

Latest commit: d4bd99e

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

[frankbits]

I just noticed, that calling removeTrailingForwardSlash() in packages/astro/src/core/build/generate.ts:362, packages/astro/src/core/build/generate.ts:377 and possibly other places would be redundant with this change, as trailing slashes should already be trimmed by trimSlashes() in stringifyParams().

[frankbits]

Maybe the Changeset should be updated, to include dynamic-routes too,
but I don't know if this issue also existed for those, or if yes, then if this fix would fix it too.

[frankbits]

Maybe the Changeset should be updated, to include dynamic-routes too, but I don't know if this issue also existed for those, or if yes, then if this fix would fix it too.

Added second test for dynamic route with leading slash, which I locally ran with and without my fix.
Dynamic Routes normally don't allow any slashes, with my change leading and trailing slashes can now be added, as they get trimmed before the validation.
I don't think this would be a problem, as only adding leading/trailing slashes still describes one directory-level.