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

[🐞] translated routes from rewriteRoutes get ignored for [...catchall] route in same folder #5665

Closed
denisyilmaz opened this issue Jan 4, 2024 · 5 comments · Fixed by #6101
Labels
COMMUNITY: good first issue Good for newcomers COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it COMP: qwik-city TYPE: bug Something isn't working

Comments

@denisyilmaz
Copy link

Which component is affected?

Qwik City (routing)

Describe the bug

I have the following routing folder structure:

  • [lang]
    • [...uri]
    • projects

The paths /en/projects//de/projects are correctly rendered by the projectsroute folder.
Any other urls /en/some-pageor /de/example/some-other-sub-path get correctly rendered by the [...catchall] route folder.

For better i18n for the application i know wanted to use the new rewriteRoutes feature and added the following:

qwikCity({
      rewriteRoutes: [
        {
          prefix: '',
          paths: {
            'projects': 'projekte'
          },
        },
      ],
    })

Expected behavior:
/de/projekte/some-projekt should be rendered via the projects route folder.

Actual behavior:
Unfortunately this is not the case but i can see the catchall route is used instead. When deleting the catchall route from the folder structure the rewriteRoutes is working correctly though.

It seems that the catchall routes are matched before the routes defined via the rewriteRoutes config. Is this an expected behavior or is there a way to prioritise the rewriteRoutes?

Reproduction

https://stackblitz.com/edit/qwik-starter-jnup5o?file=vite.config.ts

Steps to reproduce

  1. open reproduction
  2. visit: https://qwikstarterjnup5o-s2um--5173--a2aabdd9.local-credentialless.webcontainer.io/de/projects
  3. visit: https://qwikstarterjnup5o-s2um--5173--a2aabdd9.local-credentialless.webcontainer.io/de/projects/someproject
  4. visit: https://qwikstarterjnup5o-s2um--5173--a2aabdd9.local-credentialless.webcontainer.io/de/somepage/test
  5. visit: https://qwikstarterjnup5o-s2um--5173--a2aabdd9.local-credentialless.webcontainer.io/de/projekte/someproject
    • it renders the catchall route
  6. delete or rename the [...uri] route folder
  7. visit: https://qwikstarterjnup5o-s2um--5173--a2aabdd9.local-credentialless.webcontainer.io/de/projekte/someproject
    • it renders the projects route

System Info

System:
    OS: macOS 14.2.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 2.74 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 16.15.0 - ~/.nvm/versions/node/v16.15.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.15.0/bin/yarn
    npm: 8.15.1 - ~/.nvm/versions/node/v16.15.0/bin/npm
  Browsers:
    Chrome: 120.0.6099.199
    Edge: 119.0.2151.46
    Safari: 17.2.1
  npmPackages:
    @builder.io/qwik: ^1.3.2 => 1.3.2 
    @builder.io/qwik-city: ^1.3.2 => 1.3.2 
    @builder.io/sdk-qwik: ^0.7.5 => 0.7.5 
    undici: 5.26.0 => 5.26.0 
    vite: 4.4.9 => 4.4.9

Additional Information

No response

@denisyilmaz denisyilmaz added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Jan 4, 2024
@denisyilmaz denisyilmaz changed the title [🐞] translated routes in rewriteRoutes get ignored for [...catchall] route in same folder [🐞] translated routes from rewriteRoutes get ignored for [...catchall] route in same folder Jan 4, 2024
@wmertens
Copy link
Member

Indeed, that can be considered a bug. Would you be interested in contributing a PR? The rewrites should happen before the route matching.

@wmertens wmertens added COMMUNITY: good first issue Good for newcomers COMP: qwik-city COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it and removed STATUS-1: needs triage New issue which needs to be triaged labels Jan 17, 2024
@denisyilmaz
Copy link
Author

Alright, thanks for the confirmation that this is not an expected behavior. I hope to find some time the next few days to dig into this!

@fprl
Copy link
Contributor

fprl commented Mar 19, 2024

Damn, I was going to write exactly this issue. Will take a look if I can create a PR. Any clue @wmertens

Thanks

@wmertens
Copy link
Member

wmertens commented Mar 19, 2024

@fprl
Copy link
Contributor

fprl commented Apr 7, 2024

This is where routes are matched, you can look up the call chain from there.

https://github.com/BuilderIO/qwik/blob/302b27eb7f6cd9c7e47c70cdfcf478c13c916faf/packages/qwik-city/middleware/request-handler/request-handler.ts#L20

Hi @wmertens , thanks for the tip. I finally realised the issue was at build time, as the rewritten routes were pushed at the end of the routes array, instead of the original route + 1 index.

Hope its all good and it can be merged soon!

wmertens pushed a commit that referenced this issue Apr 9, 2024
…es (#6101)

* fix(qwikcity): add rewritten route after the original route

fix #5665

* chore(qwikcity): add route and rewriteRoutes to qwik-city starter to test fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMMUNITY: good first issue Good for newcomers COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it COMP: qwik-city TYPE: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants