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

Missing param due to case-sensitive token regex #2925

Closed
pimlie opened this issue Sep 9, 2019 · 6 comments
Closed

Missing param due to case-sensitive token regex #2925

pimlie opened this issue Sep 9, 2019 · 6 comments

Comments

@pimlie
Copy link
Contributor

pimlie commented Sep 9, 2019

Version

3.1.3

Reproduction link

https://jsfiddle.net/j4s8ka1v/

Steps to reproduce

Unfortunately the above reproduction link does not reproduce the issue, not sure why the tokensToFunction method isnt used in the jsfiddle. But I've traced the issue to tokensToFunction from path-to-regexp. The issue seems to have been fixed in v3.1.0 due to this pr. You can check eg dist/vue-router.esm.js to see the RegExp does not set the i modifier

That makes this report more or less a fyi / request to upgrade path-to-regexp 😃

Locally the error is caused by a route similar to

  routes: [{
      path: "/:locale(en|pt-br)?/:source(.*)?",
      component: myComponent,
    }],

which fails when you browse to e.g. /pt-BR/path:

image

What is expected?

That the token / param is matched case-insensitively

What is actually happening?

It doesnt, causing a missing param error

@posva
Copy link
Member

posva commented Sep 9, 2019

Thanks, unfortunately upgrading it comes with some breaking changes, one of them is changing '*' routes to '(.*)' which is widely used. The upgrade will happen in next major version though.
So this is something people will have to workaround by providing different possible versions (case sensitive and case insensitive) until next major version.

@pimlie
Copy link
Contributor Author

pimlie commented Sep 9, 2019

Thanks, could you please explain what you think the workaround is?

Unfortunately I dont fully understand why this issue should be closed, this issue exists and directly contradicts with the docs which says that all routes are matched case insensitively by default. Wouldnt it be better for visibility to keep this open with a target label like breaking-change and/or target: v4?

@boringame
Copy link

boringame commented Sep 30, 2019

This problem also bothers me, which is why I submitted the PR.
Here's what I did while waiting for the merge: generate expressions with upper and lower case letters.

function escapeIgnoreCase(
    text: string
): string {
    const holderFlag = "((holderFlag))";
    const holderFlagEscape = RegExp.escape(holderFlag);
    const holder = text.replace(/[a-zA-Z]/g, a => `${holderFlag}${a}`);
    const escapeHolder = RegExp.escape(holder);
    const result = escapeHolder.
        replace(
            new RegExp(`${RegExp.escape(holderFlagEscape)}([a-zA-Z])`, "g"),
            a => `[${a.replace(holderFlagEscape, "").toLowerCase()}${a.replace(holderFlagEscape, "").toUpperCase()}]`
        );
    return result;
}
// escapeIgnoreCase("pt-br"): "[pP][tT]-[bB][rR]"
routes: [{
    path: `/:locale(${escapeIgnoreCase("en")}|${escapeIgnoreCase("pt-br")})?/:source(.*)?`,
    component: myComponent,
}]

@pimlie
Copy link
Contributor Author

pimlie commented Nov 11, 2019

fyi: path-to-regexp v1.8.0 has been released which includes the backported tokenstofunctionoptions fix by @boringame

@posva
Copy link
Member

posva commented Nov 17, 2019

Thanks for that! Let's reopen this then. Feel free to upgrade the version to 1.8.0 in a PR

@posva
Copy link
Member

posva commented May 11, 2020

fixed by #3032

@posva posva closed this as completed May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants