deps: [email protected]

[nfantone]

• Bump path-to-regexp to 6.2.0.
• Add tests for new features introduced since [email protected].
• 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 [email protected]. 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]

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.

[euoia]

I have created a pull request in the documentation repo to document these changes in the Express v5 migration guide: expressjs/expressjs.com#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.

[hypesystem]

If anyone can do a recap of what exactly needs to be done to land this, I'm very willing to put in the work 😄 But I'll be honest, I find it a bit hard to navigate what's going on right now...

[wesleytodd]

Hey @nfantone! I am the person who landed the first path-to-regexp update in this branch. I am both sad and happy to say that I am working to get the thing lined up to finally wrap up this long road toward landing this in main-line express. I wanted to check in with you on this work. If you are willing to work with me I can probably help you land this before our next release. If not, I will likely say we will need to wait for 6.0 (I promise it will not be 10 years). Let me know asap so I can line this up with the plans.

[nfantone]

Hey @wesleytodd — happy to help. Honestly, I barely remember what I did here, but seems like it was awaiting a review.

I'll follow your lead. Let me know what I can do to help.

[wesleytodd]

Awesome! So I think the first thing is to chat with @blakeembrey about this. We discussed on the TC call last week that we would follow his lead on which major version of path-to-regexp should land for the router@2/express@5 releases. I figured that since this PR was here that this would likely be the best choice, but would love to hear Blake's opinion on it.

[blakeembrey]

Latest should be good. I'd land this, and then maybe another minor that I need to produce to add back * support (since it's been standardized in URLPattern now).

Once we have the min node.js version support defined for node 5.x I can also set that up in path-to-regexp. Currently I'm producing ES5 code, so the library probably works all the way back to v4 or v5 of node.js, but I'll need to test for that which usually means having to downgrade to a much older test runner (most latest versions only do LTS onward).

[wesleytodd]

Hey, so an update on this. We are going a bit of a different direction now. I am probably going to need to subsume this PR into #117 (where applicaable) because we are entirely dropping the regular expression handling in path-to-regexp in favor of only supporting "bring your own regexp" for that subset of features.

Really sorry this never landed, as the original person to land an update like this many years ago, I feel the disappointment on doing all this work for it not to ever be used by users very keenly. That said, we are moving forward and should hopefully avoid this kind of stagnation in the future.