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

Pass route array through to pathRegexp() #2207

Closed
mscdex opened this issue Jul 4, 2014 · 21 comments
Closed

Pass route array through to pathRegexp() #2207

mscdex opened this issue Jul 4, 2014 · 21 comments

Comments

@mscdex
Copy link

mscdex commented Jul 4, 2014

Currently it's possible to do:

app.get(['/foo', '/bar'], ...);

or:

router.get(['/foo', '/bar'], ...);

but not:

app.use(['/foo', '/bar'], ...);

or:

router.use(['/foo', '/bar'], ...);

I started to write a PR for this but it's conflicting with the work currently being done on implementing request path retrieval.

@dougwilson
Copy link
Contributor

.use() is for mounting, so it doesn't particularly make sense to pass multiple paths like it does for routes.

@dougwilson
Copy link
Contributor

@mscdex can you provide like a use-case for how allowing an array would be useful? I'm not particularly against adding it, but without a compelling use-case, it's just yet another thing to maintain for possibly no real reason.

@dougwilson dougwilson assigned dougwilson and unassigned dougwilson Jul 4, 2014
@mscdex
Copy link
Author

mscdex commented Jul 4, 2014

It can be useful for reusing logic for multiple routers/routes, such as loading information from a database, etc. It's also easier to scan than doing a normal .use(function(..){}) and doing an if/else ladder on the request path.

@dougwilson
Copy link
Contributor

So would this be an example of your use?

// use session for /admin and /forum
app.use(['/admin', '/forum'], session)

@mscdex
Copy link
Author

mscdex commented Jul 4, 2014

That is one possible scenario, yes.

@dougwilson
Copy link
Contributor

lol. Just throw me a bone :) If you can just like paste some scenario you want to use it for, I will likely add it; I do need to use something for tests as well.

@dougwilson
Copy link
Contributor

OK, so it's actually not possible to do this until 5.x unless you can get path-to-regexp to release a 0.1.3 version with the fix "Better support for non-ending strict mode matches with a trailing slash" in it. That fix is only in 0.2.0+ which is not backwards-compatible with the routing syntax, so cannot be included until 5.x

@dougwilson
Copy link
Contributor

/cc @blakeembrey

@dougwilson
Copy link
Contributor

@blakeembrey I cc'd you here to find out if you would be willing to make a 0.1.3 release with the fix "Better support for non-ending strict mode matches with a trailing slash" in it (one of the changes made in component/path-to-regexp@0702846). Without it, non-ending mode ({ end: false }) will compile the string / to /(^\/\/?(?=\/|$))/, for instance, which doesn't match anything.

@blakeembrey
Copy link
Member

@dougwilson No problem, I'll leave a note here once I push out a fix for you.

@dougwilson
Copy link
Contributor

Sweet :) Oh, and it looks like the patch was actually a part of component/path-to-regexp@e3df263 not the other commit I sited, haha.

@blakeembrey
Copy link
Member

@dougwilson I just realised there is an issue with this feature and the mergeParams feature (the numerical indexes test). Multiple array parts can have matching groups which is why I manually set the indexes in that commit - but it breaks the feature you have in express. Can we remove the feature from express for numerical indexes?

@dougwilson
Copy link
Contributor

@blakeembrey hm... Can you elaborate more? Example? I just am not following the word you wrote, sorry! We can always talk in #2173 if that's what it relates to, a new issue, or in freenode IRC #express.

For this issue, I'm just looking to basically get path += (end ? '$' : (path[path.length - 1] === '/' ? '' : '(?=\\/|$)')); added to a 0.1.x release.

@dougwilson
Copy link
Contributor

So @blakeembrey specifically for this issue, these are the changes I'm thinking of: https://github.com/dougwilson/path-to-regexp/compare/fix/for-express-2207

@dougwilson
Copy link
Contributor

I went ahead and made the PR https://github.com/component/path-to-regexp/pull/32 for what I think could become a 0.1.3 that I could put into an express 4.6 release, if you want to take a look.

dougwilson added a commit that referenced this issue Jul 5, 2014
@dougwilson
Copy link
Contributor

@mscdex depending on the situation of path-to-regexp, if it works out, I have implemented your request. You can try it out with npm install visionmedia/express#expressive-mount-paths and provide feedback.

@jonathanong
Copy link
Member

maybe we should just release a 5.x and drop semver so we could stop worrying about all these backwards compatibility edge cases

@dougwilson
Copy link
Contributor

@jonathanong eh, idk just yet :D I'm actually going to make a 5.x branch soon to push a bunch of future stuff, like path changes and such.

@aredridel
Copy link
Contributor

how about a proposed or next branch?

@dougwilson
Copy link
Contributor

@aredridel it's going to be 5.x, so you can consider the 5.x branch to be the "next" branch, if you prefer.

@aredridel
Copy link
Contributor

Cool.

dougwilson added a commit that referenced this issue Jul 6, 2014
dougwilson added a commit that referenced this issue Jul 8, 2014
dougwilson added a commit that referenced this issue Jul 9, 2014
@expressjs expressjs locked and limited conversation to collaborators Jul 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants