google / web-starter-kit

Web Starter Kit - a workflow for multi-device websites
http://developers.google.com/web/starter-kit
Apache License 2.0
18.43k stars 3.02k forks source link

Update Node Version tested for and allowed - Resolves #950 #951

Closed JamiesonRoberts closed 4 years ago

JamiesonRoberts commented 6 years ago

This patch removes the oldest versions of Node and only includes those that have LTS support or have existed while node.js has implemented LTS. While its potentially not a good idea to test for non LTS versions from a long term support perspective, there could be users more likely to have those interim version installed and not updated yet.

Resolves - #950

googlebot commented 6 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


JamiesonRoberts commented 6 years ago

I signed it!

googlebot commented 6 years ago

CLAs look good, thanks!

Garbee commented 6 years ago

There are still LTS OSes people use that bundle the old node by default and some people are locked to that. I'd rather we not upgrade the engine requirement until there is some tangible benefit in the project being added to justify the move. If we move without adding anything for developers we are only prematurely forcing them to upgrade systems (some of which can't be due to business rules) without any justification for it beyond security. Which, depending on the system they could not have any issue if it is being ran on isolated internal things.

Beyond the theoretical security benefits, what justification is there for making the forced upgrade on the NodeJS engine?

JamiesonRoberts commented 6 years ago

The biggest one that has a requirement of NodeJS 4 as a minimum is I would like to propose and bring forward the use of BrowserList 2.0 and its usage across Autoprefixer, Babel, ESLinting, and Stylelinting.

It would mean that all of the transpiring and polyfilling would be run off of a single declaration within the package.json, as well as allows others to change it to fit their needs. Also it means that you can work to reduce compiled asset sizes by only prefixing, or polyfilling, what is actually needed based on what your singular declaration dictates and means you have a single source of truth. Yes, it could be a relatively small performance increase, but if you also then declare something that only covers off the most recent versions of browsers, it could drastically reduce size of bundles for better performance.

I could work up a PR that would accomplish this fairly easily if you wanted to see an example of what I'm meaning.

For reference: https://evilmartians.com/chronicles/autoprefixer-7-browserslist-2-released

UPDATE: I do see your concerns about those having to use legacy versions for systems, but would that not then mean something like this could be flagged as a major semver release so that systems wouldn't force the upgrade?

gauntface commented 6 years ago

@Garbee I doubt there is a customer of WSK that is relying on node 0.12 AND unable to update and as stated - there will be modules that sooner or later will rely on newer node versions.

Garbee commented 6 years ago

SGTM then, Especially since there is active thought and work going into the next revision.

JamiesonRoberts commented 6 years ago

I'll work to get the checks resolved for it and then it should be good to go!