pillarjs / router

Simple middleware-style router
MIT License
407 stars 84 forks source link

Drop support for Node < 10 #103

Open nfantone opened 2 years ago

nfantone commented 2 years ago

As part of the efforts for releasing express v5, a PR was opened to update path-to-regexp to 6.2.0. Since path-to-regexp is currently targeting node > 10 explicitly, it'd make sense for router to deprecate support for node versions that upstream core libraries do not.

This, of course, would mean that express as a whole would stop supporting node < 10 from v5. Moving forward, supported node versions across pillarjs components should be kept in sync to avoid divergences.

I'm opening the discussion to the entire community to gather feedback and see if they have any input or objections to this.

dougwilson commented 2 years ago

currently targeting node > 10 explicitly

I'm not sure if we should interpret the versions tested against as a project's supported versions, otherwise you could say that path-to-regexp does not support 11, 12, 13, 14, 15, 16, etc. since it is not tested there. Normally if there is no engines in the package.json the module is meant to work on whatever version, but in reality it just means it is undefined. I don't see support mentioned in the README, either. We should probably engage the path-to-regexp team to clarify with the version support is if we are going to base this module on that one 👍 .

nfantone commented 2 years ago

True. And we should def engage with @blakeembrey and the path-to-regexp folks. However, I think it's pretty safe to say that the intention with what I linked is to set a minimum supported node version. It's even phrase like that in their commit messages.

dougwilson commented 2 years ago

To also help, here is a list of our public dependencies that depend on this module that would need to break in order to upgrade (list includes what their current Node.js version support is indicated to be):

sails >= 0.10.0
superstatic >= 8.6.0 (Edit: it is now >= 10.13 in master branch)
vulcain-corejs >= 6.0
rocky >= 4
box-sdk >= 0.10.0
ruo >= 6.4
agentstack-router >= 6.0.0
json-http-proxy >= 6.0.0
nestjs-client >= 8.9.0
toxy >= 4
veloce >= 8.0.0
mixtrack-client >= 8
pleasant >= 8.5.0
coffeebeans >= 0.10.0
ramljs >= 8.0
dougwilson commented 2 years ago

However, I think it's pretty safe to say that the intention with what I linked is to set a minimum supported node version. It's even phrase like that in their commit messages.

It's hard to really know what it means, but the literal interpretation of that commit message is just to run the tests on that version, not what version consumers can use. Since the module is transpiled from TypeScript into JavaScript with a target of es5 it would seem to reason the consumer support would be wider than Node.js 10, as there wouldn't be a need to go that low for Node.js 10. The path-to-regexp also has zero dependencies so there isn't a way to interpret based on that, either.

nfantone commented 2 years ago

I understand - but seeing it though that lens, since code is always targeting ES5, it'd make little sense to ever update a "min node version" (unless they have a different spec for their tests), right? Also, since 6.2.0, they support named capturing groups which were introduced in node 10.0.0.

Anyway, we digress. Let's wait for them to confirm.

dougwilson commented 2 years ago

it'd make little sense to ever update a "min node version" (unless they have a different spec for their tests), right?

Not really, as they have to run typescript and other software to perform the trandpiling and testing, which has it's own node.js requirements independent of the finished product. The CI needs to be able to run that supporting software (like the typescript lanuage engine).

Also, since 6.2.0, they support named capturing groups which were introduced in node 10.0.0.

Anyone can have a module that provides support for features added in Node.js versions thag are not a 1:1 to their support map. Just take this module for instance: it (currently) works great on Node.js 0.10 and higher, but it also supports using features like Promises which don't exist in Node.js 0.10.

nfantone commented 2 years ago

Fair enough. I just tested out path-to-regexp@6.2.0 with node versions 0.10, 0.12, 4.9.1, 6.17.1, 9.11.2, 10.24.1 and latest LTS and, if not using RegExp with named groups, it seems to work fine for the most basic of cases (i.e.: /foo/:bar). So not a confirmation, but a strong indication.

Still, we don't really need to follow the path of path-to-regexp. Even if we don't get an answer from them, we should still be able to make a decision.