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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

JamiesonRoberts
Copy link

@JamiesonRoberts JamiesonRoberts commented Sep 6, 2017

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

commit 1da342819f6366268c758ee2d5169c10b7c54039
Author: Jamieson Roberts <[email protected]>
Date:   Wed Sep 6 01:27:01 2017 -0400

    Updating to include never versions of node for testing as well as removing legacy version

commit 63895e167f0c26493460ef24e65d1f07cd469d00
Author: Jamieson Roberts <[email protected]>
Date:   Wed Sep 6 01:26:40 2017 -0400

    Updating specified version of node engine allowed
@googlebot
Copy link

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).

📝 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.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@JamiesonRoberts
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Sep 6, 2017
@JamiesonRoberts JamiesonRoberts changed the title Resolves Issue #950 Update Node Version tested for and allowed - Resolves #950 Sep 7, 2017
@Garbee
Copy link
Contributor

Garbee commented Sep 18, 2017

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

JamiesonRoberts commented Sep 19, 2017

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

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

Garbee commented Jan 10, 2018

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

@JamiesonRoberts
Copy link
Author

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

@gauntface gauntface closed this Jan 31, 2020
Jektorr7 referenced this pull request Feb 12, 2020
Changing Bullet to Horizontal Line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants