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

Case Sensitive Option for Routes #1214 #1215

Merged
merged 12 commits into from
Jun 16, 2017
Merged

Conversation

smitt04
Copy link
Contributor

@smitt04 smitt04 commented Feb 28, 2017

Added both a caseSenstive and pathToRegexpOptions property on RouteConfig. The caseSensitive property will override and add to pathToRegexpOptions which defaults to {}.

Along with that RouteRecord gets pathToRegexpOptions that comes from RouteConfig

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some test case, please?
I have one concern, it's about the route cache: if we allow overwriting routes, changing the caseSensitive option wouldn't do anything. We can also point it out in docs though, I don't see an actual use case for that but it's a gotcha 😆

@smitt04
Copy link
Contributor Author

smitt04 commented Feb 28, 2017

I was looking at where to add tests. I couldn't find any similar tests in unit and not sure where to add in e2e, didn't see any redirect tests (which is where we were having the problem). I did add it to the examples though. And before my code was working the examples broke. I would be happy adding other tests if you could point me in the right direction.

@smitt04
Copy link
Contributor Author

smitt04 commented Mar 1, 2017

@posva I have added some tests, let me know if this is good.

@smitt04
Copy link
Contributor Author

smitt04 commented Mar 14, 2017

Do we have an eta for this?

@posva posva requested a review from fnlctrl March 14, 2017 16:27
@posva
Copy link
Member

posva commented Mar 14, 2017

Sorry for the delay 😅

@posva posva requested a review from yyx990803 March 14, 2017 16:29
@posva
Copy link
Member

posva commented May 17, 2017

I'm looking into this again, there are some conflicts I'm fixing. I should get a working pr soon

@posva
Copy link
Member

posva commented May 17, 2017

Right now, the global option doesn' work. I'm looking at that

@posva
Copy link
Member

posva commented May 20, 2017

The things that I'm not sure yet:

  • Should we remove the caseSensitive option?
    • if yes, keeping only the pathToRegexOptions restrict us to using that lib
    • if no, we'd have 2 ways of setting the case sensitive option. Why the case is aliased while the others aren't?
  • The naming of pathToRegexOptions is a bit too long and very technical, maybe pathToRegex is enough but it's unclear. regexOptions or pathOptions are other alternatives

@posva
Copy link
Member

posva commented May 22, 2017

Note to myself, do not forget to close #1214 when merging

@posva
Copy link
Member

posva commented May 28, 2017

ping @yyx990803 tldr: #1215 (comment)

@rivieiraa
Copy link

When will this be merged ?

@dotBits
Copy link

dotBits commented Jun 14, 2017

I'm waiting for this merge as well. Is important to have consistency between indexed URLs with trailing slashes, otherwise there will be duplicate URLs for crawlers

@yyx990803 yyx990803 merged commit 52f4a9a into vuejs:dev Jun 16, 2017
@MachinisteWeb
Copy link
Contributor

MachinisteWeb commented Jun 16, 2017

Champagne ! (that resolve also my point: #1443)

Thx guys.

@MachinisteWeb
Copy link
Contributor

Where is the pathToRegexpOptions documentation ? Or maybe an example ?

For example the { strict: true } mode not work as expected. It's allows only url not ending with / to work. And it's not work with defaultpage.

I have only : https://router.vuejs.org/en/api/options.html#routes that indicate the property exist, but I do not have any example or detail. Where I know ``{ strict: true }` exist and what is it's true purpose ?

@rivieiraa
Copy link

rivieiraa commented Jun 30, 2017

@haeresis

Where is the pathToRegexpOptions documentation ?

pathToRegexp github page
Try this.

@MachinisteWeb
Copy link
Contributor

MachinisteWeb commented Jun 30, 2017

I will add this information to the vue-router doc. Thx @rivieiraa !

@rivieiraa
Copy link

It's pathToRegex(p)Options

strict When false the trailing slash is optional. (default: false)

It is correct to use pathToRegexpOptions: { strict: true }

@MachinisteWeb
Copy link
Contributor

MachinisteWeb commented Jun 30, 2017

So I have an issue. If I set pathToRegexpOptions: { strict: true }, only page « without » trailing work. And I expected page only « with » that's work...

On my server I create:

/ and /test/ for exemple

/ <= OK
/test/ <= OK
/test <= NOK

and with vue-router, by default it's that:

/ <= OK
/test/ <= OK
/test <= OK

With usage of pathToRegexpOptions: { strict: true } I expected this

/ <= OK
/test/ <= OK
/test <= NOK

But what I have is that

With usage of pathToRegexpOptions: { strict: true }

/ <= OK
/test/ <= NOK
/test <= OK

...

Normal?

@rivieiraa
Copy link

rivieiraa commented Jun 30, 2017

I understand your situation.

that resolve also my point: #1443

Nope, it doesn't actually. This particular pull request was about case sensitivity but also involved another pathToRegexp options as they all passed together.

@MachinisteWeb
Copy link
Contributor

MachinisteWeb commented Jun 30, 2017

So, if I correctly understand,
you think it's not normal and
there is another pull request dedicated to pathToRegexp .

You know the link to this pull request?

@rivieiraa
Copy link

Sure, i believe there must be an option to strict trailing slashes more clearly (to make slash mandatory or to deprecate it). But as long as vue router uses pathToRegexp it's a problem of third party software and i'm not sure if it could be done anyways.
No, there is no PR dedicated to trailing slash, but there is an issue you pointed to earlier #1443

@MachinisteWeb
Copy link
Contributor

I will ask to re-open it so. Thx a lot !

@posva
Copy link
Member

posva commented Jun 30, 2017

@haeresis I created an issue on the repo instead: pillarjs/path-to-regexp#112 Feel free to share your thoughts there

@rivieiraa
Copy link

rivieiraa commented Jun 30, 2017

@posva
hello! what is the difference between caseSensitive and pathToRegexpOptions: { sensitive: true } options?

@posva
Copy link
Member

posva commented Jun 30, 2017

the same, it's just that caseSensitive seems like a very common use case

@adamreisnz
Copy link

Has this been removed? pathToRegexpOptions: {strict: true} doesn't seem to do anything, still getting URL's with a trailing slash.

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 this pull request may close these issues.

7 participants