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

Drop Node 8. Support GraphQL 15. Update Babel 7 & ESLint 6.8 #271

Merged
merged 13 commits into from
May 29, 2020

Conversation

staylor
Copy link
Contributor

@staylor staylor commented May 28, 2020

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the README

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).
Copy link
Member

@abernix abernix left a 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"
Copy link
Member

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",
Copy link
Member

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'],
Copy link
Member

Choose a reason for hiding this comment

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

If we don't specify targets here or browserslist in the package.json, this transpiles to ES2015 (ES6) (Source). That's probably fine, but if we're specifying engines at >= 10, I guess we can go ahead and bump the transpilation target. I've done that in 3ecf7de.

@@ -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).
Copy link
Member

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.
@abernix abernix changed the title support GraphQL 15, Babel 7, ESLint 6.8 Drop Node 8. Support GraphQL 15. Update Babel 7 & ESLint 6.8 May 29, 2020
@abernix abernix merged commit 95aeda5 into apollographql:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants