search

shellscape / webpack-manifest-plugin

webpack plugin for generating asset manifests
MIT License
1.43k stars 186 forks source link

Allow node 10.19 #247

Closed nicolas-grekas closed 3 years ago

nicolas-grekas commented 3 years ago

This PR contains:

Are tests included?

Breaking Changes?

Description

The minimum node version was bumped in #223, which intended to bump to node >=10, but didn't mention which minor of it.

10.19 is the version shipped with Ubuntu 20.04LTS.

It would be great to allow it, to save ppl from installing yet another ppa if this is not strictly needed.

shellscape commented 3 years ago

Please don't remove our issue and pr templates. I'll be happy to review this once the template is filled out.

nicolas-grekas commented 3 years ago

Oops, sorry about that. Description updated.

shellscape commented 3 years ago

@nicolas-grekas thanks for updating that. We tend to update that version based on the LTS schedule (https://nodejs.org/en/about/releases/), since v10 is in maintenance-only mode, and new versions on the channel tend to be security and stability updates. Can you verify that the difference between 10.19 and 10.22.1 doesn't contain any security or stability fixes? If it doesn't, I'm good to merge this. If it does, I'm afraid that I'll have to decline - encouraging use of a less-stable or less-secure version of a maintenance version channel isn't something I think is good practice.

nicolas-grekas commented 3 years ago

Apparently, node 10.23.1 fixed some issues labeled as security: https://nodejs.org/en/blog/release/v10.23.1/

Note that even if >=10.22.1 is used currently, I don't think that the job of a nodejs lib is to engage into "politics" to somehow force ppl to upgrade their node instance. Thus I don't think you should bump to >=10.23 nor any higher unless you have a technical reason to do so, for the lib to work correctly.

Taking the example of node on Ubuntu LTS, ppl from Ubuntu will apply the security fixes on their "10.19" version. That's part of their mission statement and what they do for years.

That's why I think that lowering this number would be friendly to the community that uses this lib: ppl on Ubuntu don't have to be forced to install nodejs using extra means (nvm, etc.) if what they already have is good enough for the lib to run as expected.

I hope you'll accept my arguments :)

shellscape commented 3 years ago

Note that even if >=10.22.1 is used currently, I don't think that the job of a nodejs lib is to engage into "politics" to somehow force ppl to upgrade their node instance. Thus I don't think you should bump to >=10.23 nor any higher unless you have a technical reason to do so, for the lib to work correctly.

I don't believe any politics apply here.

That's why I think that lowering this number would be friendly to the community that uses this lib: ppl on Ubuntu don't have to be forced to install nodejs using extra means (nvm, etc.) if what they already have is good enough for the lib to run as expected.

I think encouraging folks to use a Node version that's lacking security fixes, let alone a version that's in maintenance mode, is irresponsible. Let alone one that's due to cease support altogether in 3 months. This is of course, personal preference, and being that, not all will agree. Ubuntu would do a great service to users to keep up with Node releases, but that's probably asking too much of such a large project. I don't believe it's burdensome for users to update to a more secure version of a thing, and I believe that applies to any library. For what it's worth, come April when v10 is out of LTS, I'll be updating all of my packages and no longer supporting v10. In terms of where energy is best spent at the moment, I'd argue that an update to Node v12 is long overdue and will buy you a full year before having to update to v14. That is of course, your personal preference, and my suggestion is nothing more than a suggestion.

I appreciate your willingness to discuss this and opening a PR for it, but I can't feel good about lowering the version range at the moment, citing the reasons above.

nicolas-grekas commented 3 years ago

I think encouraging folks to use a Node version that's lacking security fixes, let alone a version that's in maintenance mode, is irresponsible.

That's what I mean by "politics": this bump is not required by this very lib, from a technical perspective :) But I also don't think that lowering the version number would encourage anyone to use a legacy version of it. Just publishing node v10.23 with CVE attached is a strong enough incentive for ppl to upgrade, when they care.

In my case, I use node to build assets, so I don't really care about the latest vulnerabilities, as node never runs as a long-running process. That's why I would appreciate being able to just run yarn install on my default node shipped by Ubuntu, and free myself from figuring out how to upgrade node. I know it's relatively easy, but I also think this would help other ppl. That's why I opened this PR: to smoothen the DX a bit for others too.

April when v10 is out of LTS, I'll be updating all of my packages and no longer supporting v10

That's fair and that might give you some opportunities to clean up the codebase a bit by leveraging newer features (if that makes sense). In my view, this is when bumping makes the most sense.

I also appreciate that you took the time to answer, read my argument and care about my answers.

Feel free to close or merge :pray: :)

nicolas-grekas commented 3 years ago

Oh, and the most important: thank you for maintaining this package!

shellscape commented 3 years ago

Thanks for the kind words, much appreciated. I sat on this one for a while but unfortunately I'm not compelled to merge it. Thanks again for the discussion and for opening the PR.