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

Older version of path-to-regexp in Express package.json causing problems with some paths #4741

Closed
jfriend00 opened this issue Oct 30, 2021 · 7 comments
Labels

Comments

@jfriend00
Copy link

Running Express 4.17.1, if you define a route like this:

app.get("/(user|u)", ...)

with the intention of matching either /user or /u, then you get this error when the app.get() first executes to register the route:

  return new RegExp(path, flags);
         ^

SyntaxError: Invalid regular expression: /^\/(?(?:([^\/]+?))|u)\/?$/: Invalid group
    at new RegExp (<anonymous>)
    at pathtoRegexp (D:\code\test\temp\node_modules\express\node_modules\path-to-regexp\index.js:128:10)
    at new Layer (D:\code\test\temp\node_modules\express\lib\router\layer.js:45:17)
    at Function.route (D:\code\test\temp\node_modules\express\lib\router\index.js:494:15)
    at Function.app.<computed> [as get] (D:\code\test\temp\node_modules\express\lib\application.js:481:30)
    at Object.<anonymous> (D:\code\test\temp\temp.js:45:5)

The issue occurs in the path-to-regexp module. But, it appears to be something that has been fixed in more recent versions of path-to-regexp. Express 4.17.1 has path-to-regexp: 0.1.7 in its package.json, whereas the latest version of path-to-regexp is 6.2.0. And, when I manually call path-to-regexp("/(user|u)") in this latest version, it does not cause the problem seen above. In fact, the code that causes the problem above has been completely rewritten in path-to-regexp as it looks nothing like the older version I step through in Express.

@dougwilson
Copy link
Contributor

Hello, yes, the 4.x line has the newest version of thay dependency that is not a breaking version. You can test out Express with a more up to date dependency: http://expressjs.com/en/guide/migrating-5.html

@dougwilson
Copy link
Contributor

You can also, in both versions of Express, pass in a regular expression directly instead of a string that is subject to how path-to-regexp works if that helps at all.

@dougwilson
Copy link
Contributor

I believe in the version of path-to-regexp in 4.x, you are just required to use a named parameter for that syntax: app.get("/:p(user|u)", ...)

@dougwilson
Copy link
Contributor

When I look at the package.json for the 5.x branch, it's still pointing at the old version of path-to-regexp. I don't see how that version is going to fix anything. Shouldn't there be an open issue on this somewhere?

I'm not sure why that would be, as the latest 5.x of Express has no direct dependency at all on path-to-regexp, so it shouldn't be in the package.json. What is the version field in the package.json you are looking at?

@jfriend00
Copy link
Author

When I look at the package.json for the 5.x branch, it's still pointing at the old version of path-to-regexp. I don't see how that version is going to fix anything. Shouldn't there be an open issue on this somewhere?

I'm not sure why that would be, as the latest 5.x of Express has no direct dependency at all on path-to-regexp, so it shouldn't be in the package.json. What is the version field in the package.json you are looking at?

I deleted my previous comment because I was looking a alpha.5, not alpha.8.

@dougwilson
Copy link
Contributor

I deleted my previous comment because I was looking a alpha.5, not alpha.8.

Oh, apologies; I am just replying via email.

@dougwilson
Copy link
Contributor

Also, apologies, I took a look and apparently the updated router is not in alpha 8. PR #4321 is the change and just needs changes to account for the different functionality in the test suite (it is failing in the PR since that contained breaking changes). I'll take a look in to helping that PR along over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants