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 package.json file to support node 9.x #1508

Merged
merged 2 commits into from Sep 5, 2018
Merged

update package.json file to support node 9.x #1508

merged 2 commits into from Sep 5, 2018

Conversation

simcoder
Copy link
Contributor

@simcoder simcoder commented Sep 5, 2018

Some of use are using node 9.x

Some of use are using node 9.x
@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@IvanGoncharov
Copy link
Member

Additional discussion here: #1445 (comment)

@mjmahone I think we should merge this PR and since master doesn't contain any breaking/feature changes on top of 14.0.0 we can release this fix as 14.0.1.

And I will try to either remove sane dependency or revert it back to the version that supports node v9.

@leebyron
Copy link
Contributor

leebyron commented Sep 5, 2018

I actually think this should change to “>= 6.x”

The engines field suggests minimum support. I don’t think it’s appropriate to remove the odd versions and create install warnings just because we stop testing them in Travis.

Travis should test all currently supported versions of node. We don’t need to test older odd versions.

There’s no need to change the version of sane. That’s a dev dependency and we only expect those to apply in our Travis builds

@leebyron
Copy link
Contributor

leebyron commented Sep 5, 2018

LTS only means the node versions will get security updates in the future. While a node version out of support may be unwise to use in production, that’s not really our concern or our place to enforce. As long as we’re testing the oldest node version we claim to support and the newest, then we have a very high confidence of coverage for all versions in between. That’s especially true for this library since we do not rely on Node API which can change from version to version. We only rely on JS core API and that’s always backwards compatible.

@leebyron
Copy link
Contributor

leebyron commented Sep 5, 2018

TL;DR, we should only use the “engines” field to exclude node versions if we think it is unsafe to run this library on those versions. If node < 6 is lacking core JS API or syntax support, then “engines” is an appropriate tool to exclude those.

@IvanGoncharov IvanGoncharov merged commit 1f44c59 into graphql:master Sep 5, 2018
@IvanGoncharov
Copy link
Member

@leebyron Agree 👍 I updated PR with your suggestion and merged it.

Since we don't use any Node API or 3rd-party modules (except iterall which doesn't have any dependencies) we depend only on JS feature set which should be stay the same for higher version. Plus we are limited by IE anyway 😞

@IvanGoncharov
Copy link
Member

@mjmahone @leebyron Can you please release the new version?

@IvanGoncharov
Copy link
Member

I think it should be 14.0.1.

@leebyron
Copy link
Contributor

leebyron commented Sep 5, 2018

Yep, I'll start that release now

@jure
Copy link

jure commented Sep 6, 2018

Thank you! Also, in terms of continuing to test in v9, it should also be possible to simply ignore sane's engines configuration.

Edit: Anyone know what happened with the 14.0.1 release? It isn't on npm, but the tag here exists.

michaelbromley added a commit to vendure-ecommerce/vendure that referenced this pull request Sep 6, 2018
Yarn is failing with node 9 because graphql-js v14.0.0 removed support for node 9 in the "engines" field of package.json.

Looks like this will be sorted out in the next release (graphql/graphql-js#1508) but for now it is easiest to use node v8 to make the build pass.
IvanGoncharov added a commit to graphql/express-graphql that referenced this pull request Sep 6, 2018
junminstorage pushed a commit to junminstorage/express-graphql that referenced this pull request Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants