deps: path-to-regexp@6.2.0

[nfantone]

• Bump path-to-regexp to 6.2.0.
• Add tests for new features introduced since path-to-regexp@3.2.0.
• Fix minor typos in test descriptions and some .md files.

[dougwilson]

What are the changes between 3.x and 6.x? We need to document and add tests for whatever new features that were added to route paths, if any. The test suite is alsl unfortunately not comprehensive, so no failure does not mean there isn't some breaking change we need to document, and perhaps test to add. Getting a list of what the changes were in the dependency would help determine that.

For example, the "Custom Prefix and Suffix" system I believe was added in 6.0 and of course we have no tests or any docs around that. I'm sure there are other changes, but I know that is at least one.

[nfantone]

@dougwilson I 100% understand where you're coming from and I'm all for doing this at some point - but may I ask you to elaborate on why you think we should do it now? path-to-regexpis an internal implementation detail. The API surface of router hasn't changed; we haven't added any new features to this module. Everything that we want to support and is documented still works. I'm not sure I see a need for what you're requesting.

[dougwilson]

The path-to-regexp module is not an internal implementation detail; it is actually the main component of this module's (and Express's) public API: the route strings. Are you suggesting that Express should not document what kind of things you can put in the argument for something like app.get?

[dougwilson]

Here is a breaking change I just found with this upgrade that breaks the router public API and should be documented: The path '/:foo([a-z()]+)' would construct a path that captures a URL that contains the characters a-z and (, and ), but with this upgrade, that path now throws with TypeError: Capturing groups are not allowed at 10.

I'm sure I'm just scratching the surface of the breaking changes in how the paths work with these two major version bumps, as I just stopped once I saw the first breaking change in path-to-regexp dependency. As noted, the test suite in here is not very great, especially when a PR like this adds no tests in order to detect future breaking changes :)

[nfantone]

The path-to-regexp module is not an internal implementation detail; it is actually the main component of this module's (and Express's) public API.

Gotcha. I disagree, though. From my POV, it most definitely is an implementation detail. How much router relies on it is also an implementation detail. Users installing router don't know nothing about path-to-regexp, the same way they know nothing about methods or array-flatten. If it wasn't an internal dependency, people would just install path-to-regexp instead of this library :)

Are you suggesting that Express should not document what kind of things you can put in the argument for something like app.get,

No. I'm suggesting that it is already documented and supported and there's no further need to add more details than the ones we have now, unless we introduce a breaking change.

[nfantone]

Here is a breaking change I just found with this upgrade that breaks the router public API and should be documented

Agreed, then. If it breaks the existing public API, it should def be tested and documented. Good catch.

[dougwilson]

Gotcha. So I get it, they should not know path-to-regexp exists today, apologies there. I think we are on the same page. So when a user sees the following path app.get('/user{-:id}', fn) where do they go to find out what that is supposed to do, exactly? And also, does the req.params this module is constructing for that new path-to-regexp feature even make sense (I think it does, but I have not gone through all the variations of that new feature yet)?

[nfantone]

Perhaps the problem with my focus is that I may be relying a bit too much on the test suite here. I saw all 703 tests passing and I assumed everything was working as expected. Having said that, my answers to your questions would be:

So when a user sees the following path app.get('/user{-:id}', fn) where do they go to find out what that is supposed to do, exactly?

It depends. Does that exist today, i.e.: is that supported with latest Express 5 beta release? If it is, there should already be docs for that (if there aren't, we should add them). If it's not, then there wouldn't be a specific place as it is simply not supposed to work with this release.

I guess what I'm trying to convey here is that express defines the features that path-to-regexp should provide. Not the other way around.

[dougwilson]

Perhaps the problem with my focus is that I may be relying a bit too much on the test suite here. I saw all 703 tests passing and I assumed everything was working as expected.

Right, as I said, the test suite is not a good indicator. It is very lacking. I have over time added a lot more tests, but there is still more to add. It doesn't always keep up, either, like this PR doesn't add any tests for all the new path matching added in the version upgrade.

It depends. Does that exist today, i.e.: is that supported with latest Express 5 beta release? If it is, there should already be docs for that (if there aren't, we should add them). If it's not, then there wouldn't be a specific place as it is simply not supposed to work with this release.

It doesn't exist in the current Express 5, as that is based off path-to-regexp 0.1.x. Your PR here is what is adding that feature, so it would not exist in any current documentation, which is why I'm saying should it be added somewhere as part of this upgrade which adds this feature.

I guess what I'm trying to convey here is that express defines the features that path-to-regexp should provide. Not the other way around.

That is an interesting take. I don't believe the path-to-regexp project is run like that, so maybe we need to bring that up with that project's maintainers. Edit: OR there should be logic added here that filters out the features from path-to-regexp to just the ones that we want to expose.

[nfantone]

Your PR here is what is adding that feature, so it would not exist in any current documentation,

My point is that it's really not adding it. path-to-regexp has that feature - router doesn't. It may in the near future, but currently it doesn't. Let's see how far we can go with this PR, though.

OR there should be logic added here that filters out the features from path-to-regexp to just the ones that we want to expose.

Right. This is what I was trying to say. It's completely fine to let path-to-regexp run its course - but adapting express to match its API feels backwards.

Anyway, regardless, I've added a bunch of new tests for all new features I could find on path-to-regexp@6.2.0. And a few that cover the breaking change you described before.

[dougwilson]

My point is that it's really not adding it. path-to-regexp has that feature - router doesn't. It may in the near future, but currently it doesn't.

But from a user's perspective who is using router, they wouldn't be able to make a route with those features, but then they upgraded router and now they can. As far as the user is concerned, router added that feature (because path-to-regexp is an internal implementation detail, not something the user would think about).

Right. This is what I was trying to say. It's completely fine to let path-to-regexp run its course - but adapting express to match its API feels backwards.

Gotcha. So what should we do instead?

[nfantone]

@dougwilson I see you've added test pipelines for very ancient runtimes. This is likely to make tests fail due to the lack of support for widely used language features, such as destructuring, in third-party dependencies. If we really need to support things like io.js, we would need to either use some kind of bundler (like webpack or rollup) or find older versions of our dependencies that would support it (if they exist).

[dougwilson]

I didn't add any new pipelines; this module has always ran tests against all the Node.js versions listed in the engines property in package.json.

If upgrading path-to-regexp no longer works on the Node.js versioned listed in the engines field, then we need to evaluate the impact of bumping the engines requirement in this module and it's consumers (mainly Express).

[nfantone]

Weird. No pipelines were run in any of my previous commits. It was only after I rebased your latest changes from 1.3.6 that they were triggered.

Also, the "engines" property only lists "node" - but GH Actions are run against io.js as well.

[dougwilson]

Weird. No pipelines were run in any of my previous commits. It was only after I rebased your latest changes from 1.3.6 that they were triggered.

The project was out of OSS credits in Travis CI at the time.

Also, the "engines" property only lists "node" - but GH Actions are ran against io.js as well.

The io.js platform was ths fork of Node.js that then became Node.js again with version 4 (Node.js 4 is io.js 4). It used the same field in engines. Effectively Node.js was called io.js for versions 1, 2, and 3.

[nfantone]

The project was out of OSS credits in Travis CI at the time.

👍🏼

It used the same field in engines.

Ok. I have oftentimes seen it as "iojs". Which really makes sense since the versioning has diverged.

[dougwilson]

Ok. I have oftentimes seen it as "iojs". Which really makes sense since their versioning has diverged.

I'm not aware of any tooling that used a separate field. Remember, io.js is Node.js now :) https://nodejs.org/en/blog/release/v4.0.0/

[nfantone]

I'm not aware of any tooling that used a separate field

I would know of a bunch. Just search "engines iojs" on all of Github and you'll see it popping out quite a few times.

If upgrading path-to-regexp no longer works on the Node.js versioned listed in the engines field, then we need to evaluate the impact of bumping the engines requirement in this module and it's consumers (mainly Express).

I couldn't find any tight info on this. path-to-regexp doesn't have an "engines" field and their changelog/releases don't specify anything in relation to the Node version they support (or, at least, I couldn't dig it out easily). Right now, the tests are failing due to a new dev dependency (semver) using destructuring - the same semver dep path-to-regexp is already using, BTW.

EDIT: According to their Travis CI config, they only run tests against node 10 and LTS.

IMHO, in this day and age, it makes little sense to keep supporting runtimes below LTS. The feature gap is no longer what it used to be and I like to think that we won't be having an angry io.js mob shaking fists at us anytime soon.

[dougwilson]

I would know of a bunch. Just search "engines iojs" on all of Github and you'll see it popping out quite a few times.

Can you be more specific? Which ones? I mean, this module has only ever supported being installed by npm and npm never used iojs field when installing on io.js (I even put in a feature request for npm at the time and it was rejected).

IMHO, in this day and age, it makes little sense to keep supporting runtimes below LTS.

I don't disagree. If this is a blocker for this PR, definitely open up a new issue to track this and you can link yo this PR as support for the reasoning 👍 . We want to make sure that users have a chance to see an upcoming change link this and have a forum to be able to voice their opinion on it. This is our process for dropping things like Node.js versions.

[nfantone]

Understood. On which repository should I open it?

[dougwilson]

The repo you are desiring to make the change on.

[nfantone]

RIght - but this would mean, as you pointed out, changing the min Node version for express as a package. So it would impact multiple repositories at once. Also, it'd probably have more visibility to the community if we post such a thing on https://github.com/expressjs/express. Objections?

[dougwilson]

There is already an open issue in the express repo, so you can just add to that. But unless express is the only possible repo that consumes this one (Edit: npm shows there are 215 public dependents), there should probably be one here too. I know you believe no one will care, but when you manage this many repos, you come to find out they do. And they of course come out immediately mad at you after you made the release. The purpose of this is mainly so we can point where they had an opportunity to raise their opinion before the change was made and reduces our burden managing these issues. I would expect them to remain upset if we pointed them to a completely different repo, though.

Also, apologies but I have to head off to work, so I won't be around for at least 10 hours.

[nfantone]

@dougwilson Here's the relevant issue: https://github.com/pillarjs/router/issues/103.

EDIT: Just read the related issue on https://github.com/expressjs/express/issues/2755. It seems to me the community has pretty much already voiced their opinion over there. Any reasons why you'd want me to open a new issue asking the same? I really think we should move forward and avoid the bike-shedding.

[dougwilson]

Any reasons why you'd want me to open a new issue asking the same?

I said not to open a new issue to ask that, as there was already one, as you saw :)

[nfantone]

You also said to open an issue where you "are desiring to make the change on" :). So I did.

[dougwilson]

You also said to open an issue where you "are desiring to make the change on" :). So I did.

Right. So where have the users of router (an independent project from express) voiced their opinion?

[dougwilson]

I looked into the failure and it looks like the only issue is just with the semver dependency that was added to the devDependencies of this module requires Node.js >= 10. I added the older semver to the older Node.js versions so they can use the relevant version of that devDependency and tests are now passing across all versions Node.js 0.10+

[jimmywarting]

I don't know if you have heard about this new URLPattern and a explainer (edit: funny they maintaining your path-to-regexp) It's pretty sweet if you ask me, would be cool if express used it instead

there is a polyfill floating around in npms user land

[nfantone]

@dougwilson Do you think we could merge this? Or do you feel like there's more work to be done here? I would really like to move forward with express v5 tasks.

[dougwilson]

Hi @nfantone I am away for this week for a US holiday and will be back again working on open source after that. I believe there is still an on-going conversion from last I interacted with this PR, including you double checked my thought that part of the history in this PR was incorrect and needed to be updated. We also identified there were no tests around the feature mentioned in the change log, which we could add, though they could just be in their own pr. I know you aksed another question above, but I left already, so I didn't have a chance to evaluate it. I will get back after the holiday 👍

[nfantone]

@dougwilson Ping?

[dougwilson]

Hi @nfantone thank you for the ping. I just got back today and am currently triaging a security vulnerability report that was given over the holiday. I just need to look in to that ASAP and determine what needs to be done and than I will get back to here 👍 , especially as I'm going to be away again this weekend and the beginning of next week. It does seem this is the time of year (along with March-ish) that a lot of security reports seem to come in for whatever reason.

[dougwilson]

In the meantime, please feel free to push the changes discussed above, as we'd need those regardless.

[nfantone]

@dougwilson Welcome back. Hope you had a good time.

including you double checked my thought that part of the history in this PR was incorrect and needed to be updated

I believe I answered that already in the thread above. Let me know if I'm mistaken.

We also identified there were no tests around the feature mentioned in the change log

Which feature is that again? Think we've covered them all. Which one did we miss?

[dougwilson]

So the changes are that the inaccurate changelog in that discussion above needs to be fixed in this PR, as you listed a breaking change under this PR that didn't change at all in this PR. The tests are for that same feature. https://github.com/pillarjs/router/pull/102#discussion_r751635292

[dougwilson]

Oh, and for the tests, sorry, we don't need to add tests for new features that were added before this specific upgrade, as those would just be "misses" from previous PRs and shouldn't hold this one up, but of course you're welcome to do so if you like. The parenthesis stuff was a change from a previous PR; the {} matching stuff and the named groups where the only new things here and you already added tests for them 👍

[nfantone]

Ah, I see. I can move the line in the changelog, that's no problem. But not sure I see what you're referring to as a "feature". More like a change of how things should be done for stuff that was already there. And, for that, all tests have been already accommodated and we have a couple targeting that change specifically. Do you think we need more?

[nfantone]

(Welp, I didn't realize I had already committed the semver removal change last week - so it went together with the changelog update. Sorry for the confusion - should do the same trick).

[nfantone]

@dougwilson Are you happy to merge this? Or is there anything else you'd like to include here?

[nfantone]

@dougwilson Pong?

[dougwilson]

Hi, sorry I was out the beginning of this week, and unfortunately trying to now recover from a very bad sickness. I will take a look at what I can as fast as I can.

[nfantone]

unfortunately trying to now recover from a very bad sickness.

Hope you're feeling better! I'll take a look at your comment above now.

[nfantone]

@dougwilson Anything else you'd like me to address?

[nfantone]

@dougwilson Happy new year! Do you think you could take a look at this PR in the near future? If not, would there be anybody else that could take over from you? Really eager to merge this, help the community and get the release finally going.

[dougwilson]

Hi @nfantone I should be out of the hospital soon. @xjamundx has been helping out.

[nfantone]

Hey @dougwilson, sorry for pressuring, I did not know you were hospitalized. Hope you're feeling better.

[euoia]

I have created a pull request in the documentation repo to document these changes in the Express v5 migration guide: https://github.com/expressjs/expressjs.com/pull/1316

[nfantone]

@dougwilson Any chance of reviving this? What's the current holdup?

[silverwind]

Bump, this seems to be the last blocker for 2.0.0.