Optional parameter with matcher followed by a rest parameter route results in 404

Oreilles commented 1 year ago

Describe the bug

Considering the following route path:

/[[locale=locale]]/[category]/[...slug]

/fr/about/something succeed.
/about/something succeed.
/fr/about/something/else succeed.
/about/something/else fails with 404 (not matched by the router).

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-r3nnht

Logs

No response

System Info

System:
    OS: macOS 13.0.1
    CPU: (8) arm64 Apple M1
    Memory: 73.06 MB / 8.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.11.0 - /opt/homebrew/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 8.19.2 - /opt/homebrew/bin/npm
  Browsers:
    Brave Browser: 109.1.47.171
    Firefox: 108.0.2
    Safari: 16.1
  npmPackages:
    @sveltejs/adapter-auto: ^1.0.0 => 1.0.1
    @sveltejs/kit: ^1.0.0 => 1.1.3
    svelte: ^3.54.0 => 3.55.1
    vite: ^4.0.0 => 4.0.4

Severity

serious, but I can work around it

Additional Information

No response

pdrbrnd commented 1 year ago

to add more info to this issue, it seems that optional matchers only shift values if the next segment is a rest segment.

a test like this in packages/kit/src/utils/routing.spec.js will fail:

{
  route: '/[[slug=doesntmatch]]/[[slug2=doesntmatch]]/[...rest]',
  path: '/foo/bar/baz',
  expected: { rest: 'foo/bar/baz' }
},

from a (semi-)quick inspection, the problem seems to be in these lines, more specifically in if (next?.rest && next.chained) – which isn't necessarily the case: you can have several optional segments that don't match that should shift their value to the next segment.

This relates to https://github.com/sveltejs/kit/pull/7753 and https://github.com/sveltejs/kit/issues/7548

A valid scenario for something like this would be: [[lang=lang]]/[[region=region]]/[...rest]

'/foo/bar' // { rest: 'foo/bar' }
'/en/foo/bar' // { lang: 'en', rest: 'foo/bar' }
'/gbr/foo/bar' // { region: 'gbr', rest: 'foo/bar' }
'/en/gbr/foo/bar' // { lang: 'en', region: 'gbr', rest: 'foo/bar' }

Naturally we can work around this and use a single [...path] and extract the segments ourselves but we have significant impact if, for instance, we have a +layout.server.ts load function that depends on params.lang to fetch layout data. Now every path change will trigger an invalidation regardless of whether params.lang changed – making a lot of unnecessary requests that will impact navigation performance.

Rich-Harris commented 1 year ago

marking this with the ready to implement label though i know from bitter experience that this will likely be a tough issue to close...

sveltejs / kit