Updated to path-to-regexp@3.0.0 #42

wesleytodd · 2016-05-19T18:47:30Z

This would probably be a major version bump for router since the path matching semantics are so different.

Also. you will notice a change in the tests. I opened a separate issue to discuss, #41.

dougwilson · 2016-05-19T18:56:26Z

Also. you will notice a change in the tests. I opened a separate issue to discuss, #41.

The test has been fixed for quite a while on master. I can see this PR cannot even be merged by GitHub, so I think you have it based off some stale code.

wesleytodd · 2016-05-19T18:58:00Z

Yep, forgot to update my forked version. Will do that now, sorry.

dougwilson · 2016-05-19T18:58:16Z

And yes, bumping would be 2.0, which is fine :) Can we get a migration guide written up for users to understand how to translate their routes from the old style to the new style to add with this? I think that's really the only blocker.

wesleytodd · 2016-05-19T19:20:53Z

Based on the tests there is one breaking use case because * is not a valid regexp. This means /:foo(*) needs to become /:foo(.*).

I think @blakeembrey could probably provide a more comprehensive list of changes, but then I would be happy to write them up into a guide.

blakeembrey · 2016-05-19T19:28:00Z

I've written the differences a bundle of times, I think the most comprehensive was for Doug in the Express repo.

dougwilson · 2016-05-19T19:28:44Z

The tests in this module do not actually exercise the different syntaxes currently, so knowing what broke is not possible from this test suite (for example, I don't see you adding any tests for all the new syntax in 1.x, continuing that trend ;) ).

wesleytodd · 2016-05-19T20:27:08Z

I can add a bunch more tests for the supported syntax if you like. It would seem weird to test features of path-to-regexp in this module though, so I would prefer to keep it the way it is.

For documentation, is what is listed here comprehensive? Can we just do a link here to that section?

wesleytodd · 2017-03-25T15:42:31Z

I cannot find any more docs that what is in that readme. So I added them here.

As for tests, I think testing dependencies is not a great decision. But I understand the reasoning here because path-to-regexp makes up a HUGE portion of the interface for the router. If we could come up with a way to exercise the tests from the underlying module where when it updated, we automatically were testing the new features, then I think it would be fine. Otherwise it is just a maintenance headache.

Either way, this update does not make us worse off. So IMO it could be merged.

dougwilson · 2017-03-27T00:55:05Z

I agree that extensively testing dependencies is not great. But sometimes I still think that at least basic tests of the main features of a dependency as they are directly exposed through the interface of this module helps flush out accidental regressions upstream and can help us understand if we can bump a major in that dep without bumping our dep.

dougwilson · 2017-03-27T00:56:42Z

Thanks for starting on the migration guide :) ! One thing I noticed right away is that the migration guides doesn't provide any guidance on whatever the reason the tests were changed in order to up this dep?

wesleytodd · 2017-03-27T02:07:24Z

I added tests, I think they should be satisfactory, hits the major points. Let me know if there is anything else you would like me to add.

As for the guide thing, I am really unsure what the core changes were, so I don't actually know what to add. I am happy to add what other people post here, or let them add to this PR as necessary.

gajus · 2017-10-13T16:17:38Z

The latest version is 2.0.0.

dougwilson · 2017-10-13T16:21:21Z

Yea, so we still need a migration guide from the respect of this module. Users need to understand what routes they need to change and how during the upgrade or they'll never be able to upgrade their apps successfully. It seems the 2.0 release breaks even more tha. The 1.0, which is fine, but I'm not even clear on how to upgrade the route paths for the transition.

blakeembrey · 2017-10-13T21:58:23Z

Just a note, but compatibility is still listed in https://github.com/pillarjs/path-to-regexp#compatibility-with-express--4x. The primary difference between 1.x and 2.x was that I needed to drop support for * - it wasn't clear how it was meant to be encoded when converting a "path string" like /* into /foo/bar.

wesleytodd · 2017-10-13T22:04:15Z

Ok, I just updated this PR. But it appears that something also changed with the handling of trailing slashes. @blakeembrey Know what I need to do to fix the two tests I added skip to?

test/route.js

wesleytodd · 2017-10-13T22:15:19Z

I just updated the upgrade guide as well. Any thoughts or changes necessary?

dougwilson · 2017-10-13T22:16:59Z

Does this include the changes between 0.1.x and 2.x or just 0.2.x and 2.x? What were the differences between 0.1.x and 0.2.x, again?

wesleytodd · 2017-10-13T22:17:33Z

AFAIK, this guide is ALL of the changes since 0.1.7.

dougwilson · 2017-10-13T22:19:48Z

README.md

+The main change is the update to `path-to-regexp@2.0.0`, which has a few breaking changes:
+
+- No longer a direct conversion to a RegExp with sugar on top - it's a path matcher with named and unnamed matching groups
+  - It's unlikely you previously abused this feature, it's rare and you could always use a RegExp instead


We should explain how to know if you are using the feature rather than editorial that "you're probably not using it"

@blakeembrey I copied this from your readme. Can you give me a short blurb to help explain what this means?

Like what is an example "abuse of this feature"?

Sure. It's anywhere someone is abusing stuff like /\\d+ outside of a matching group (e.g. instead of /(\\d+)). It's possible it's used in places today, but wouldn't be too useful since it wouldn't result in a match the user could use.

If you end up coming up with a better or more succinct explanation, happy to replace the one in the README 😄

You'd be surprised what people do with these routes. Express 4.16.1 is a release just because it seemed to be popular to do something thag shouldn't work, but did.

Question: What specifically does this mean?

Parameters have suffixes that augment meaning - *, + and ?. E.g. /:user*

Couldn't we always use those suffixes? How are they different from 0.1.7 to 2.0.0?

No, 0.x never had a concept of "suffixes". There's places where it happened to work because everything is treated as a regexp with based search and replace. E.g. /* became /(.*) in search and replace (that's the reason you can't use * in Express.js). It also means that, yes, stuff like /:user? might have worked but purely because of the search and replace implications and definitely not consistently (for instance, /:test* never would have done anything expected).

Note: I haven't tested the specific examples above, but it's possible a comparison (0.x -> 2.x) might be the easiest way to understand the differences.

Yeah that is probably why those worked for me :)

I was actually thinking it might be nice to update the demo app to support multiple versions, which might be helpful for people to discover the differences for their routes. Maybe I can work on that next.

Ok, got a PR for that feature in the demo app: ForbesLindesay/express-route-tester#11

If you all need to see the differences in the versions you can view it here until the PR is merged: https://wesleytodd.github.io/express-route-tester/

dougwilson · 2017-10-13T22:20:24Z

README.md

+- No longer a direct conversion to a RegExp with sugar on top - it's a path matcher with named and unnamed matching groups
+  - It's unlikely you previously abused this feature, it's rare and you could always use a RegExp instead
+- All matching RegExp special characters can be used in a matching group. E.g. `:user(*)` needs to become `/:user(.*)`
+  - Other RegExp features are not support - no nested matching groups, non-capturing groups or look aheads


Typo in "support"

blakeembrey · 2019-05-26T19:41:39Z

@wesleytodd See https://github.com/pillarjs/path-to-regexp/blob/master/History.md#300--2019-01-13. Specifically it was the treatment around delimiter tokens (e.g. the character before :token), it was improved to handle a couple of use-cases more elegantly and removed "partial" token support which is actually better handled by escaping characters.

wesleytodd · 2019-05-26T19:59:04Z

Awesome, so I updated to 3.0, it caused one test failure. I added another section to the readme to describe the change, and made the corresponding test change. This should be good to merge now I think.

wesleytodd · 2019-11-18T22:01:00Z

Too many versions too fast :P

@blakeembrey Should we update this to 5? What are the breaking changes in 4 and 5?

blakeembrey · 2019-11-19T05:59:38Z

@wesleytodd See https://github.com/pillarjs/path-to-regexp/releases. 4.0 was for ES2015, 5.0 was because I added a feature(s) I shouldn't have.

wesleytodd · 2019-11-19T21:46:51Z

Ahh, I was wondering about that quick jump from 3 to 5. There was too many notifications for me to read all of them and follow. Could you summarize the changes so we can add it to the readme and docs? I will update the PR from there.

blakeembrey · 2019-11-20T00:45:44Z

@wesleytodd The only mentionable feature is support for nested non-capturing regex groups, e.g. /(abc(?=d)). No breaking changes in these releases for paths, only library usage (e.g. module.exports -> module.exports.pathToRegexp).

Edit: Technically someone could be doing /(abc(def) and expecting it to escape the (, so it's not technically true that there are no breaking changes and only features.

blakeembrey · 2019-11-20T04:19:59Z

Final release I intend to make for this library is https://github.com/pillarjs/path-to-regexp/releases/tag/v6.0.0, which reverts some of the confusion I added in v3 and adds an alternative ({}) to support the reason I did the any character prefix in v3.

blakeembrey · 2019-11-20T04:21:11Z

README.md

+console.log(req.params) // {'foo': 'bar'}
+```
+
+#### Partial matching, prefer escaping delimiter


This section shouldn't be relevant anymore. However, you can make the example /user{s}? if you wanted to.

blakeembrey · 2019-11-20T04:22:39Z

test/route.js

@@ -645,7 +645,7 @@ describe('Router', function () {
        it('should work following a partial capture group', function (done) {
          var cb = after(2, done)
          var router = new Router()
-          var route = router.route('/user(s)?/:user/:op')
+          var route = router.route('\\/user(s)?/:user/:op')


Should be good to revert this.

gireeshpunathil · 2020-03-06T14:51:14Z

this needs a rebase. anything else?

dougwilson · 2020-03-06T14:56:31Z

This needs nothing at this time as Wes rebased it on a separate branch instead of within the PR. Github will close it automatically after I move the changes back to the PR branch.

dougwilson · 2020-03-13T03:24:42Z

So it looks like I don't have permission to push up to the remote branch. This is just a heads-up, so the changes will end up landing on the 2.0 branch, and will end up stating this PR is "closed" instead of "merged".

gireeshpunathil · 2020-03-13T03:55:37Z

the changes will end up landing on the 2.0 branch, and will end up stating this PR is "closed" instead of "merged".

@dougwilson - can you clarify what does this mean? the PR is originally destined for 2.0 branch itself, right? or is there a change? or is it that the landing will be same (to 2.0) but the github web interface will show it as closed, while the code would have landed?

dougwilson · 2020-03-13T04:02:43Z

This PR has merge conflicts that must be resolved, and even has an incomplete section in the migration guide among other changes that are needed. I am working on the changes, but I get permission denied trying to push them up to the PR branch. Since I cannot push them into this PR first, landing this PR + changes onto 2.0 will cause GitHub to mark this PR as "closed" instead of "merged", is all.

closes #42

dougwilson · 2020-03-13T04:48:27Z

All the changes here + various fix ups has landed into the 2.0 branch 🎉

dougwilson added pr deps labels May 19, 2016

wesleytodd force-pushed the update-path-to-regexp branch from 8b77a8b to 868ae16 Compare May 19, 2016 19:18

wesleytodd mentioned this pull request May 19, 2016

router.bind is overridden as an http method #41

Open

wesleytodd changed the title ~~Updated to path-to-regexp@1.4.0~~ Updated to path-to-regexp@1.7.0 Mar 25, 2017

wesleytodd force-pushed the update-path-to-regexp branch from 9a4a859 to 6d0dba6 Compare March 27, 2017 01:57

wesleytodd mentioned this pull request Oct 13, 2017

path-to-regexp library is toooooo old expressjs/express#3419

Closed

wesleytodd force-pushed the update-path-to-regexp branch from 6d0dba6 to e49c42b Compare October 13, 2017 22:03

wesleytodd changed the title ~~Updated to path-to-regexp@1.7.0~~ Updated to path-to-regexp@2.0.0 Oct 13, 2017

wesleytodd commented Oct 13, 2017

View reviewed changes

test/route.js Show resolved Hide resolved

wesleytodd force-pushed the update-path-to-regexp branch from e49c42b to 2118d04 Compare October 13, 2017 22:14

dougwilson reviewed Oct 13, 2017

View reviewed changes

dep: path-to-regexp@3.0.0

a5a94f3

wesleytodd force-pushed the update-path-to-regexp branch from 9d829da to a5a94f3 Compare May 26, 2019 19:57

wesleytodd changed the title ~~Updated to path-to-regexp@2.40~~ Updated to path-to-regexp@3.0.0 May 26, 2019

wesleytodd mentioned this pull request Jul 18, 2019

deps: setprototypeof@1.2.0 #81

Merged

wesleytodd mentioned this pull request Aug 16, 2019

Upgrade path-to-regexp to version 1.7.0 expressjs/express#4015

Closed

blakeembrey reviewed Nov 20, 2019

View reviewed changes

dougwilson mentioned this pull request Dec 19, 2019

Incompatible with path-to-regex ≥ 6 expressjs/express#4136

Closed

wesleytodd mentioned this pull request Jan 17, 2020

Update path-to-regexp to 1.0.3 expressjs/express#2530

Closed

HarshithaKP mentioned this pull request Mar 3, 2020

document path-to-regexp change in express 5 migration guide expressjs/expressjs.com#1114

Merged

wesleytodd mentioned this pull request Mar 5, 2020

Express TC Meeting 03-05-2020 expressjs/discussions#106

Closed

gireeshpunathil mentioned this pull request Mar 9, 2020

express 5.0.0 expressjs/discussions#100

Closed

dougwilson pushed a commit that referenced this pull request Mar 13, 2020

deps: path-to-regexp@3.2.0

4acf108

closes #42

dougwilson closed this Mar 13, 2020

dougwilson mentioned this pull request Nov 15, 2021

feat: bump router to beta.1 expressjs/express#4752

Closed

krzysdz mentioned this pull request Jan 11, 2023

res.locals value not available in template engine expressjs/express#5081

Closed

wesleytodd mentioned this pull request Mar 16, 2024

deps: path-to-regexp@6.2.0 #102

Closed

Updated to path-to-regexp@3.0.0 #42

Updated to path-to-regexp@3.0.0 #42

Conversation

wesleytodd commented May 19, 2016

dougwilson commented May 19, 2016

wesleytodd commented May 19, 2016

dougwilson commented May 19, 2016

wesleytodd commented May 19, 2016

blakeembrey commented May 19, 2016

dougwilson commented May 19, 2016

wesleytodd commented May 19, 2016

wesleytodd commented Mar 25, 2017

dougwilson commented Mar 27, 2017

dougwilson commented Mar 27, 2017

wesleytodd commented Mar 27, 2017 • edited Loading

gajus commented Oct 13, 2017

dougwilson commented Oct 13, 2017

blakeembrey commented Oct 13, 2017

wesleytodd commented Oct 13, 2017

wesleytodd commented Oct 13, 2017

dougwilson commented Oct 13, 2017

wesleytodd commented Oct 13, 2017

Choose a reason for hiding this comment

wesleytodd Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleytodd Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeembrey commented May 26, 2019

wesleytodd commented May 26, 2019

wesleytodd commented Nov 18, 2019

blakeembrey commented Nov 19, 2019

wesleytodd commented Nov 19, 2019

blakeembrey commented Nov 20, 2019 • edited Loading

blakeembrey commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Mar 6, 2020

dougwilson commented Mar 6, 2020

dougwilson commented Mar 13, 2020

gireeshpunathil commented Mar 13, 2020

dougwilson commented Mar 13, 2020

dougwilson commented Mar 13, 2020

wesleytodd commented Mar 27, 2017 •

edited

Loading

wesleytodd Oct 13, 2017 •

edited

Loading

wesleytodd Oct 14, 2017 •

edited

Loading

blakeembrey commented Nov 20, 2019 •

edited

Loading