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

fix: require Node.js 6+ for all packages #7258

Merged
merged 4 commits into from
Oct 25, 2018
Merged

fix: require Node.js 6+ for all packages #7258

merged 4 commits into from
Oct 25, 2018

Conversation

DanielRuf
Copy link
Contributor

@DanielRuf DanielRuf commented Oct 24, 2018

Summary

Changes in Jest packages require at least Node.js 6.5+ so there is also no compatibility with older releases of Node.js 6.

Motivation: Node.js 4 support was officially dropped in Jest 22 and we test only 6, 8 and 10 so it makes sense to enforce node >= 6 on all packages to prevent breaking builds due to incompatible Node.js versions as this is a hard breaking change.

See https://jestjs.io/blog/2017/12/18/jest-22

See #7253

Test plan

@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

We already have it, it seems: https://github.com/facebook/jest/blob/319329f7942ed33f0be706a19a2b23d08eac0ea0/packages/jest/package.json#L14
https://github.com/facebook/jest/blob/319329f7942ed33f0be706a19a2b23d08eac0ea0/packages/jest-cli/package.json#L48

What package did you install that did not warn?
We can update it to a newer version of 6 if array.includes was not in 6.0.0, but I'm unable to figure out what version it was introduced in...

@DanielRuf
Copy link
Contributor Author

See http://kangax.github.io/compat-table/es2016plus/#node6_5 for Array.prototype.includes.

It was in jest-validate with blacklist.includes (and possibly more as there is also the spread operator).

@DanielRuf
Copy link
Contributor Author

DanielRuf commented Oct 24, 2018

Well, but in our case it did not warn that Node.js 6 is required for Jest. See https://travis-ci.org/docker/kitematic/builds/443616745#L475

https://node.green/#ES2016-features-Array-prototype-includes

@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

We might want to add it to all packages, then. Didn't think about the case where people use the other parts of jest, silly to overlook.

Well, but in our case it did not warn that Node.js 6 is required for Jest.

That seems like an npm bug. Maybe because it's fetched from cache?

See http://kangax.github.io/compat-table/es2016plus/#node6_5 for Array.prototype.includes.

Yes, but it doesn't mention anything about 6.0.0 not having it.

Node 6.0.0 has it:

image

@DanielRuf
Copy link
Contributor Author

We might want to add it to all packages, then. Didn't think about the case where people use the other parts of jest, silly to overlook.

What can I do here to help?

That seems like an npm bug. Maybe because it's fetched from cache?

Not sure as this is a very old npm version which was shipped with Node.js 4.

@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

What can I do here to help?

If you update all packages/*/package.json to include the engine part, that'd be awesome 🙂

That seems like an npm bug. Maybe because it's fetched from cache?

Not sure as this is a very old npm version which was shipped with Node.js 4.

🤷‍♂️ Not much we can do here though, as we have had that engine field since #4769 was merged (2 days less than a year ago).

@DanielRuf
Copy link
Contributor Author

DanielRuf commented Oct 24, 2018

Ok I'll change it to >= 6 and update the other package.json files too to ensure that all packages enforce the same minimum requirement so we can assure that no old Node.js release line tries to run unsupported code.

So it seems the table is not exact.
Mh, is there a better table just for Node.js? https://node.green/#ES2016-features-Array-prototype-includes is not more helpful as it only shows 6.4.0+ for 6.x.

Not sure if exact:
bildschirmfoto 2018-10-24 um 10 38 04

@DanielRuf DanielRuf changed the title fix: require Node.js 6.5+ by default fix: require Node.js 6+ for all packages Oct 24, 2018
@codecov-io
Copy link

Codecov Report

Merging #7258 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7258   +/-   ##
=======================================
  Coverage   66.55%   66.55%           
=======================================
  Files         237      237           
  Lines        9317     9317           
  Branches        4        3    -1     
=======================================
  Hits         6201     6201           
  Misses       3115     3115           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 319329f...4570fcc. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the changelog as well?

@DanielRuf
Copy link
Contributor Author

Could you update the changelog as well?

How exactly? I never did this in this repo before so I am open to any code suggestions (and it is open for edits by maintainers).

https://blog.github.com/2018-10-16-future-of-software/#suggested-changes-public-beta

@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

Add an entry under "Chore" here: https://github.com/facebook/jest/blob/master/CHANGELOG.md

@DanielRuf
Copy link
Contributor Author

Add an entry under "Chore" here: /CHANGELOG.md@master

Done.

@SimenB
Copy link
Member

SimenB commented Oct 25, 2018

Thanks!

@SimenB SimenB merged commit c5e3683 into jestjs:master Oct 25, 2018
@DanielRuf DanielRuf deleted the fix/require-nodejs-6 branch October 25, 2018 07:43
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants