-
Notifications
You must be signed in to change notification settings - Fork 103
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
Drop Node 8. Support GraphQL 15. Update Babel 7 & ESLint 6.8 #271
Drop Node 8. Support GraphQL 15. Update Babel 7 & ESLint 6.8 #271
Conversation
While we will still have to reevaluate this in the future, we're testing for versions `>= 15` so this name matches up a bit more precisely.
…slist`. By default, if no `targets` option is passed, or `browserslist` isn't set in the `package.json`, `@babel/preset-env` only transpiles to ES2015/ES6. Since we're only testing Node.js 10+ and we've set the `engines` property to indicate our intention of only supporting Node.js 10+, I think we can transpile to a newer target safely. Not truly important to count bytes in this package, but this shaves off an unimpressive 8kB (but also a 20% reduction for this package).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for jumping into this! I pushed a few follow-up commits, but LGTM.
"husky": { | ||
"hooks": { | ||
"pre-commit": "pretty-quick --staged" | ||
} | ||
}, | ||
"engines": { | ||
"node": ">=6.0" | ||
"node": ">=10.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a note about dropping support for Node.js to the changelog after merging.
@@ -21,34 +22,54 @@ const envGraphQLValidatorNames = { | |||
apollo: without( | |||
allGraphQLValidatorNames, | |||
"KnownFragmentNames", | |||
"NoUnusedFragments" | |||
"NoUnusedFragments", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR didn't introduce it this, but worth noting that this technique of excluding by validator rules by the string-form of their name has broken down once before when subjected to minification since the functions in graphql
had been minified themselves. See apollographql/apollo-server#3335.
Because of its role in the ecosystem (in dev tooling) I don't suspect this package or its dependencies will be subject to minification, but just noting. Definitely not asking for a change. 😄
@@ -0,0 +1,4 @@ | |||
module.exports = { | |||
presets: ['@babel/preset-env'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3,6 +3,9 @@ | |||
### vNEXT | |||
|
|||
- Improve identity template literal tag docs. [PR #254](https://github.com/apollographql/eslint-plugin-graphql/pull/254) by [Jayden Seric](https://github.com/jaydenseric). | |||
- Add support for GraphQL 15. [PR #271](https://github.com/apollographql/eslint-plugin-graphql/pull/271) by [Scott Taylor](https://github.com/staylor). | |||
- Update all `devDependencies` - upgrades the project to use Babel 7 and ESLint 6. [PR #271](https://github.com/apollographql/eslint-plugin-graphql/pull/271) by [Scott Taylor](https://github.com/staylor). | |||
- **BREAKING**: Minimum supported Node.js version is now Node.js 10; Dropped support for Node.js 8. [PR #271](https://github.com/apollographql/eslint-plugin-graphql/pull/271) by [Scott Taylor](https://github.com/staylor). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explicitly made a call-out to dropping Node.js from engines
. Even if the relatively unenforced engines
was the only thing preventing it from working on new versions, I think it's okay for us to take a stance on it 3ecf7de 😄 .
Putting a `const` below the first place it was used was never an acceptable mechanism for using `const` since it has different dynamics than a `var`. However, prior to 3ecf7de, we were transpiling `const` into `var` statements, which are hoisted.
TODO: