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.

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

Routes formatted as $foo-bar are being prioritised over routes formatted as foo-bar. #11159

Closed
jtaccinelli opened this issue Jan 2, 2024 · 5 comments · Fixed by #11160
Closed

Comments

@jtaccinelli
Copy link

Reproduction

  1. Go to this reproduction on StackBlitz
  2. Visit /pages/foo-bar

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.4.2 - /usr/local/bin/npm
    pnpm: 8.10.5 - /usr/local/bin/pnpm
  npmPackages:
    @remix-run/css-bundle: * => 2.4.1 
    @remix-run/dev: * => 2.4.1 
    @remix-run/node: * => 2.4.1 
    @remix-run/react: * => 2.4.1 
    @remix-run/serve: * => 2.4.1

Used Package Manager

npm

Expected Behavior

In the case where there are routes using pages.$foo-bar.tsx and pages.foo-bar.tsx routes, I would expect /pages/foo-bar to match with the pages.foo-bar.tsx file, not the pages.$foo-bar.tsx file.

Actual Behavior

When visiting /pages/foo-bar, the pages.$foo-bar.tsx file is matched over the pages.foo-bar.tsx file.

@jtaccinelli
Copy link
Author

For additional context:

We are aware that the behaviour of $foo-bar matching all routes ending in -bar might not be intended behaviour, but it is behaviour we'd love to preserve if possible. It actually works fine as is with the exception of this bug.

In our case, we're building out an e-commerce site that uses different collection page templates depending on the route suffixes. We have:

  1. Standard Collection Pages (formatted as collections.$handle.tsx).
  2. Bundle Collection Pages (formatted as collections.$handle-bundle.tsx).
  3. Look Collection Pages (formatted as collections.$handle-look.tsx).

Before realising this was a thing, we were originally loading all collection templates into the collections.$handle.tsx file and doing all the fetching for all three pages in one loader. Being able to format routes to then match against these suffixes works much better for us, and let's us handle all loader logic and page templates in seperate files.

The only issue we're running into is that we have a SEO heavy index page on /collections/shop-the-look which matches the collections.$handle-look.tsx instead of the existing collections.shop-the-look.tsx page.

@brophdawg11
Copy link
Contributor

We are aware that the behaviour of $foo-bar matching all routes ending in -bar might not be intended behaviour, but it is behaviour we'd love to preserve if possible. It actually works fine as is with the exception of this bug

Unfortunately, it's not It's not intended behavior - the behavior itself is the bug.

Dynamic segments (named parameters) are required to be the full URL segment. Partial named parameters are not supported (they inadvertently worked for a small period of time, and were fixed in 6.5.0).

This appears to be a bug due to an internal assumption that named parameters are only word characters (via an internal \w regex match), so the - is not being picked up as part of the param name when it should be. I.e., /pages/:foo-bar should populate params["foo-bar"].

I'm going to transfer this over to an issue in React Router but to align with expected behavior, I would recommend avoiding the dash in the param name ($foo-bar -> $fooBar).

If you need to do this type of route/bundle spitting, I would do it via URL segment:

/collections/$handle.tsx
/collections/bundle/$handle.tsx
/collections/look/$handle.tsx

@brophdawg11 brophdawg11 transferred this issue from remix-run/remix Jan 3, 2024
@brophdawg11 brophdawg11 self-assigned this Jan 3, 2024
@brophdawg11 brophdawg11 linked a pull request Jan 3, 2024 that will close this issue
@brophdawg11
Copy link
Contributor

This is resolved by #11160 and will be fixed in the next release

@brophdawg11 brophdawg11 added the awaiting release This issue have been fixed and will be released soon label Jan 9, 2024
@brophdawg11 brophdawg11 removed their assignment Jan 9, 2024
Copy link
Contributor

github-actions bot commented Jan 9, 2024

🤖 Hello there,

We just published version 6.21.2-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 6.21.2 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue have been fixed and will be released soon label Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants