Skip to content

Use babel-preset-env #880

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

Merged
merged 3 commits into from
Jul 3, 2017
Merged

Use babel-preset-env #880

merged 3 commits into from
Jul 3, 2017

Conversation

baer
Copy link
Contributor

@baer baer commented May 25, 2017

This PR updates the .babelrc file to follow the guidelines set by the Babel team. The following changes were made:

  • Use babel-preset-env to target environments rather than trying to target a spec which may be in various states of completion depending on the environment.
  • Remove all hand-added transformation for and syntax for ES2015 features from the .babelrc and package.json
  • Add missing transform-class-properties dep to the package.json

The fallout from this is that the .babelrc file is much cleaner and, much more importantly, the file that is shipped to clients should be much leaner since it only applies the presets that a required for that platform.

@baer baer changed the title Feature/use babel preset env [WIP] use babel preset env May 25, 2017
@baer baer changed the title [WIP] use babel preset env [WIP] use babel-preset-env May 25, 2017
package.json Outdated
"babel-plugin-transform-es2015-block-scoping": "6.24.1",
"babel-plugin-transform-es2015-classes": "6.24.1",
"babel-plugin-transform-es2015-computed-properties": "6.24.1",
"babel-plugin-transform-class-properties": "^6.24.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was missing from the package.json and was only working through an accidental transitive dep.

.babelrc Outdated
"transform-es2015-modules-commonjs",
"transform-regenerator",
"transform-es3-property-literals"
["transform-es2015-spread", {"loose": true}],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was defined twice. I kept the loose mode dep but if loose can be turned off, both this and the destructuring transform could be removed since they are already applied in the Node4 preset.

@baer baer changed the title [WIP] use babel-preset-env Use babel-preset-env May 25, 2017
Copy link
Contributor

@leebyron leebyron left a comment

Choose a reason for hiding this comment

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

I'm not sure about this. We're not compiling for Node v4, we support lots of browsers as well. I'm concerned that lying about our compile target will result in difficult to detect issues for slightly older browsers.

What's the value this provides us over an explicit list of transforms?

.babelrc Outdated
@@ -1,27 +1,16 @@
{
"presets": [["env", {
"targets": {
"node": 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this guarantee the same scope of transforms as below?

package.json Outdated
"babel-plugin-syntax-async-generators": "6.13.0",
"babel-plugin-transform-class-properties": "6.24.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's defined here - your change just caused the line to appear added since a ^ was added to the version

@baer
Copy link
Contributor Author

baer commented May 26, 2017

tl;dr I think there is value in doing this but the PR, as it is now, is not correct as you pointed out. I'd love to work together to get it sorted out.


We're not compiling for Node v4, we support lots of browsers as well

I had been thinking of this as a set of server side modules but you're right, some parts of this will be run on both the client and on the server in things like Relay-Modern, Apollo and others.

I'm concerned that lying about our compile target will result in difficult to detect issues for slightly older browsers.

The intention was not to lie about the compile target (although, yes, I took care to make sure the same transformations were applied) the intention was to be explicit about what you are compiling for. It was my mistake to assume Node4 as a compile target. The right thing would be to re-work this PR so that the target is something like the following.

        "targets": {
          "node": 4,
          "browsers": [
            "last 2 versions", 
            "ie >= 9",
            "safari >= 7"
          ],
          "uglify": true
        }

What's the value this provides us over an explicit list of transforms?

  • It's what the babel team has been pushing for the last few months. That's less of a benefit and more of an endorsement but ¯_(ツ)_/¯.
  • It makes the support matrix for the project very explicit.
  • It makes compilation as minimal as possible which generally makes debugging in Node nicer
  • It lowers maintenance overhead associated with keeping the .babelrc, package.json JS features in the code and implicit browser support in sync.
  • It opens the door to doing jsnext:main types of things very simply which is a dramatically better debugging experience for Node

Full disclosure, I wrote some of babel-preset-env and am in the Babel org.

@baer
Copy link
Contributor Author

baer commented May 26, 2017

Also, thanks for all the work you've done on this project and specifically for the quick review!

@wincent
Copy link
Contributor

wincent commented May 26, 2017

I'm not sure about this. We're not compiling for Node v4, we support lots of browsers as well. I'm concerned that lying about our compile target will result in difficult to detect issues for slightly older browsers.

We could (and maybe should) consider adding a "browsers" target as well. The docs don't make it explicit, but I would assume that in the presence of multiple targets babel-preset-env will put together a lowest-common-denominator list.

Actually, now that I write it, I think perhaps we should specify a "browsers" target instead of the "node" one, not in addition to it. The CI should let us know if we ever break things in Node, but having Babel automatically configured to work on "not ie <= 8" would be pretty nice (other examples here).

@baer
Copy link
Contributor Author

baer commented May 26, 2017

Another quick note w/r to browser support is that often Uglify2 is the limiting factor. Most folks do compression on their full bundle as a post-build step and unfortunately Uglify2/3 does not support any ES-next syntax. This is changing with things like uglify-es and Babili but that's far from the norm. You'll notice that in included the "uglify": true flag in the above example config to reflect this.

I think the strongest case for this plugin is an explicit statement of support for some environments without having to manage the union(nodeReqs, chromeReqs, etc) yourself.

@leebyron
Copy link
Contributor

I think the strongest case for this plugin is an explicit statement of support for some environments without having to manage the union(nodeReqs, chromeReqs, etc) yourself.

I agree this is pretty compelling. If updated to list all of this out (we should support IE9+, old chrome/ff, etc) while maintaining the "loose mode" transforms to preserve performance, and to guarantee avoiding bundling in polyfills, then this could be merged.

Otherwise, my primary concern is that some future version of babel-preset-env may change behavior and cause builds to either break or balloon in size. Specifying the exact set of transforms currently gives us full control over the output to avoid this.

@baer
Copy link
Contributor Author

baer commented Jun 30, 2017

Okay, all done! After this diff, the transformations will be identical to what is in master but fully managed by babel-preset-env with a way smaller list of deps in package.json!

caveat: The following transformations are new. Below is a list of transforms and a comma-separated list indicating the browsers that triggered babel-preset-env to include them. ios === iOS Safari

transform-es2015-for-of { edge:14, ie:9, ios:9, node:4}
transform-es2015-sticky-regex {ie:9, ios:9, node:4}
transform-es2015-typeof-symbol {ie:9}
transform-es2015-unicode-regex {ie:9, ios:9, node:4}
transform-exponentiation-operator {ie:9, ios:9, node:4}
transform-async-to-generator {edge:14, ie:9, ios:9, node:4}
syntax-trailing-function-commas {ie:9, ios:9, node:4}

"my primary concern is that some future version of babel-preset-env may change behavior and cause builds to either break or balloon in size. Specifying the exact set of transforms currently gives us full control over the output to avoid this."

babel-preset-env is completely data driven (kangax/compat-table and ai/browserlist). Future versions will strive to make the API easier to use but there are no changes that I can think of that could have an impact on bundle size. That said, you are right, if you maintain 100% control you mitigate 100% of the risk. IMO specifying support is a clear win for users and the reduction of deps + minimal compilation is a win for developers but I understand that the scope of this projects makes even small risks harder to stomach. ¯_(ツ)_/¯ Your call on that.

"while maintaining the "loose mode" transforms to preserve performance"

babel-preset-env does not apply loose mode by default. To match the configuration in master, you'll notice the following overrides in the config. I'm not sure that this gains much so these may be candidates for removing (from the config) in the future. If performance is something you're after you may want to consider using jsnext:main or using Rollup/Webpack to do some pre-compile DCE. Happy to hook that up in a separate PR.

transform-es2015-classes
transform-es2015-destructuring
transform-es2015-spread

@wincent
Copy link
Contributor

wincent commented Jul 3, 2017

Thanks for your work on this @baer. I think what you've got here is reasonable at this point. Let's ship it and revise it as necessary if we discover some unanticipated downside.

@wincent wincent merged commit 68b5d22 into graphql:master Jul 3, 2017
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.

4 participants