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

Drop support for Node < 10 #103

Closed
nfantone opened this issue Nov 17, 2021 · 8 comments
Closed

Drop support for Node < 10 #103

nfantone opened this issue Nov 17, 2021 · 8 comments
Labels

Comments

@nfantone
Copy link
Contributor

nfantone commented Nov 17, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

nfantone commented Nov 17, 2021

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
Copy link
Contributor

dougwilson commented Nov 17, 2021

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
Copy link
Contributor

dougwilson commented Nov 17, 2021

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
Copy link
Contributor Author

nfantone commented Nov 17, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

nfantone commented Nov 18, 2021

Fair enough. I just tested out [email protected] 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.

@jonkoops
Copy link
Contributor

This issue can be closed as support for older Node.js versions has been dropped in 2.0

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

4 participants