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

fix for #1234, order less greedy routes first #1275

Closed
wants to merge 2 commits into from

Conversation

pimlie
Copy link

@pimlie pimlie commented Aug 3, 2017

As it is possible to use dynamic route matching with nuxt, you can get into problems when you have multiple routes that match the same url. See #1234 for an example case.

The vue-router documentation states that:

Sometimes the same URL may be matched by multiple routes. In such a case the matching priority is determined by the order of route definition: the earlier a route is defined, the higher priority it gets

This commit changes the route sorting behaviour by making sure that less greedy routes are listed before more greedy routes. In this context a less greedy route is defined as having more segments in its path (e.g. /a/b/c is less greedy then /a)

@pimlie
Copy link
Author

pimlie commented Aug 3, 2017

CI fails because the order of the routes in .nuxt/router.js has changed. If you give this commit your approval I will update the test to reflect the new route orders before merging

@alexchopin
Copy link
Member

alexchopin commented Aug 3, 2017

Hi @pimlie, I see what you mean, however, this PR resolve only your use case. (e.g: /* route will be sorted after /*/* unfortunately)
I'll take a look on it 👍

@pimlie
Copy link
Author

pimlie commented Aug 3, 2017

@alexchopin In my rc3 project the / route is still set as the first entry. This is because the sorting function explictly checks for the / route: https://github.com/nuxt/nuxt.js/blob/dev/lib/common/utils.js#L216

            {
                        path: "/", 
                        component: _7dcc7ef7,
                        name: "index"
                },
                {
                        path: "/*/p",
                        component: _1f3bd925,
                        name: "all-p",
                        children: [
                                {
                                        path: "*", 
                                        component: _3ffae7f5,
                                        name: "all-p-all"
                                }
                        ]
                },
                {       
                        path: "/disclaimer",
                        component: _50775646,
                        name: "disclaimer"
                },
                {       
                        path: "/blog",
                        component: _c7e6c1e6,
                        children: [
                                {
                                        path: "",
                                        component: _2556e7d0,
                                        name: "blog"
                                },
                                {
                                        path: "*", 
                                        component: _689c5846,
                                        name: "blog-all"
                                }
                        ]
                },
                {       
                        path: "/*",
                        component: _15c1aa07,
                        name: "all"
                }

@alexchopin
Copy link
Member

alexchopin commented Aug 3, 2017

@pimlie What's happening if you go to this URL /blog/p/a?

@pimlie pimlie force-pushed the issue-1234-route-ordering branch 2 times, most recently from 4d08e7c to 167dba8 Compare August 3, 2017 15:03
@pimlie
Copy link
Author

pimlie commented Aug 3, 2017

Then it indeed goes to the product component and not the blog.

I rewrote the commit to check for segment length within the for loop, now the following route list is generated:

    routes: [
                {
                        path: "/",
                        component: _7dcc7ef7,
                        name: "index"
                },
                {
                        path: "/disclaimer",
                        component: _50775646,
                        name: "disclaimer"
                },
                {
                        path: "/blog",
                        component: _c7e6c1e6,
                        children: [
                                {
                                        path: "",
                                        component: _2556e7d0,
                                        name: "blog"
                                },
                                {
                                        path: "*",
                                        component: _689c5846,
                                        name: "blog-all"
                                }
                        ]
                },
                {
                        path: "/*/p",
                        component: _1f3bd925,
                        name: "all-p",
                        children: [
                                {
                                        path: "*",
                                        component: _3ffae7f5,
                                        name: "all-p-all"
                                }
                        ]
                },
                {
                        path: "/*",
                        component: _15c1aa07,
                        name: "all"
                }
    ],

Seems to work better indeed.

@pimlie
Copy link
Author

pimlie commented Aug 7, 2017

@alexchopin Looking at the code a final time, what is exactly the meaning of the following code?

          if (i === _b.length - 1 && res === 0) {
            res = 1
          }

Was that meant to fix this issue already? Do we still need it?

Never mind, I got it. To fix the order this should return -1 and not 1.

@Atinux
Copy link
Member

Atinux commented Aug 14, 2017

Hi @pimlie

It was a bit different to make it work, but your issue and PR helped us to update the code to make the fix, thank you!

See the updated tests and code to see the fix: db47df0

It will be available soon in rc5

@Atinux Atinux closed this Aug 14, 2017
@pimlie
Copy link
Author

pimlie commented Aug 14, 2017

@Atinux Thanks, as long as the issue is resolved I am happy 👍

@pimlie pimlie deleted the issue-1234-route-ordering branch September 14, 2018 08:50
@lock
Copy link

lock bot commented Oct 31, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants