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

Dynamic routes can hijack more specific routes #152

Closed
rchl opened this issue Nov 5, 2018 · 9 comments · Fixed by #526
Closed

Dynamic routes can hijack more specific routes #152

rchl opened this issue Nov 5, 2018 · 9 comments · Fixed by #526
Assignees
Labels

Comments

@rchl
Copy link
Collaborator

rchl commented Nov 5, 2018

Version

v5.3.0

Reproduction link

https://codesandbox.io/s/ly9pnqvp79

Steps to reproduce

  1. Open codesandbox testcase
  2. Add /us to the URL

What is expected ?

Page shows 'US' in big letters

What is actually happening?

Page shows 'FR'

Additional comments?

This error happens with dynamic nuxt routes. Those are named with _.vue (or _slug.vue but I haven't checked those).

The bug seems to be in the order in which routes are generated. For example in this case:

['nuxt-i18n', {
            locales: [
                {code: 'pt', iso: 'pt-PT', file: 'pt.json'},
                {code: 'fr', iso: 'fr-FR', file: 'fr.json'},
                {code: 'us', iso: 'us-US', file: 'us.json'},
                // ...
            ],
            // ...
            defaultLocale: 'fr',
        }],

routes are generated in the order in which languages are defined in locales array. With this setting, generated routes will look like this:

routes: [
        {
                path: "/pt/*",
                component: _865a92ea,
                name: "all___pt"
        },
        {
                path: "/*",
                component: _865a92ea,
                name: "all___fr"
        },
        {
                path: "/us/*",
                component: _865a92ea,
                name: "all___us"
        }
],

As you can see, the 'catch-all' route (default language) was placed before more specific 'us' route. With this setup, 'us' route is inacessible due to routes being matched by iterating routes in the order they are defined.

Problem can be fixed by generating routes in proper order.

This bug report is available on Nuxt community (#c168)
@ghost ghost added the cmty:bug-report label Nov 5, 2018
@rchl rchl changed the title Not able to match locale properly on dynamic routes Dynamic routes can hijack more specific routes Feb 8, 2019
@rchl
Copy link
Collaborator Author

rchl commented Feb 8, 2019

More mature implementations like ui-router for angular sort the rules by specificity. It's quite complex algorithm though and I'm not sure exactly about specifics.

You could argue that it could be job for vue-router itself but if vue router expects users to add routes manually, I guess it won't take responsibility for sorting them. nuxt-i18n generates routes automatically so it probably should handle that.

@stale

This comment has been minimized.

@stale stale bot added the wontfix label Jun 13, 2019
@rchl

This comment has been minimized.

@stale stale bot removed the wontfix label Jun 13, 2019
@stale

This comment has been minimized.

@stale

This comment has been minimized.

@stale stale bot added the wontfix label Oct 15, 2019
@rchl rchl added bug 🐛 and removed wontfix labels Oct 15, 2019
@rchl rchl self-assigned this Oct 15, 2019
@pimlie
Copy link

pimlie commented Oct 16, 2019

Is case-insensitivity a must for the locale part of routes? If not, you could use similar routes as we do in nuxtpress: https://github.com/nuxt/press/blob/develop/packages/core/src/blueprint/index.js#L435-L440

But those routes are case sensitive due to an issue in path-to-regexp. That issue has been fixed but only in the v3 branch and vue-route and nuxt still use v1 due to breaking changes. I've submitted a pr to backport the fix to the v1 branch but that hasnt been accepted yet: pillarjs/path-to-regexp#195

@rchl
Copy link
Collaborator Author

rchl commented Oct 16, 2019

Do you mean to use route param (like :locale) for generated routes?
If so then that would mean the validation of supported locales would have to move from routes definition to a middleware.
Also, routes would have extra param. It could potentially be an interoperability issue with existing code.

I was thinking to fix that by just using routes sorting function that you've provided in Nuxt utils.

@pimlie
Copy link

pimlie commented Oct 16, 2019

I am not fully sure how nuxt-i18n validation works in all cases, but if you create a list of supported locales for a route and apply them like we do in nuxtpress there is no validation needed anymore. path: /:locale(en|pl)/ will match /en/ and /pl/ but not /nl/

--edit--
But due to that issue in path-to-regexp it also doesnt match /EN/ or /eN/

@rchl
Copy link
Collaborator Author

rchl commented Oct 16, 2019

Actually that would require quite some refactoring as now code relies on separate routes for each locale so that those can be matched by name. I think I will try fixing it by sorting routes by specificity.

@rchl rchl closed this as completed in #526 Nov 11, 2019
rchl added a commit that referenced this issue Nov 11, 2019
With there being a catch-all route (_.vue) in pages directory,
depending on the order of locales in configuration, default locale's
route could shadow locale-specific one due to being first on the list
of generated routes. For example, routes were generated like so:

```
  routes: [{
    path: "/*",
    component: _0bdbeb02,
    name: "all___en"
  }, {
    path: "/fr/*",
    component: _0bdbeb02,
    name: "all___fr"
  }],
```

which meant that `/*` route never allowed later ones to be matched.

Fix by sorting routes by specificity, using function exposed
by `@nuxt/utils` (only from 2.10.2).

Resolves #152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants