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

query parameters and RegExp's involving path parameter #91

Closed
evanyeatts opened this issue Sep 29, 2016 · 2 comments
Closed

query parameters and RegExp's involving path parameter #91

evanyeatts opened this issue Sep 29, 2016 · 2 comments

Comments

@evanyeatts
Copy link

evanyeatts commented Sep 29, 2016

I encountered an inconsistency when constructing RegExp's for paths involving path parameters. Here is an example:

var matcher = pathToRegexp('/fetch');   // /^\/fetch(?:\/(?=$))?$/i
var a = '/fetch';
var b = '/fetch/';
var c = '/fetch?foo=bar';
var d = '/fetch/?foo=bar';

Strings a and b match against the regex, and both c and d do not. This matches my expectation that pathToRegexp is not meant to manage query parameter parsing without additional code.

var matcher = pathToRegexp('/fetch/:id');   // /^\/fetch\/((?:[^\/]+?))(?:\/(?=$))?$/i
var a = '/fetch/1';
var b = '/fetch/1/';
var c = '/fetch/1?foo=bar';
var d = '/fetch/1/?foo=bar';

Strings a, b, and c will all match against the regex, but d will not. I would expect c and d to both fail to match given the behavior in the previous example. The trailing / makes the difference: regexp visualization. The capture group intended to match the path param ends up also matching against the query with string c.

Is the issue here just that pathToRegexp is not designed to analyze request url's that have not had their query portion removed? Should I clean the query first?

@blakeembrey
Copy link
Member

Yes, you should remove the query first. This only handles pathnames, not the query portion, as that would be a mess to correct route on 😄

@dougwilson
Copy link

Probably add a note to the README to mention that the returned regular expression is meant to be matched against the pathname

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

No branches or pull requests

3 participants