Switch to babel preset env + async/await/generator support #1668

ndelangen · 2017-08-16T21:48:18Z

Possible fix for: #1665, #489
Possible fix for: #1570

What I did

I changed all usages of babel-preset-es20xx to babel-preset-env
I also added babel-minify
And I enabled webpack tree-shaking

How to test

Run the examples
Build the static versions of the examples

joscha

General gist looks good, not sure about enabling mangle

joscha · 2017-08-16T21:55:04Z

app/react/src/server/config/webpack.config.prod.js

@@ -29,7 +29,7 @@ export default function() {
          screw_ie8: true,
          warnings: false,
        },
-        mangle: false,
+        mangle: true,


This would also affect code written by consumers of storybook, right?

Yes, should be safe right?

I think this is okay because its the prod config that compiles a static bundle. Presumably, users wouldn't be debugging from a static file served on an http host somewhere.

right, and they could add sourcemaps if required using custom webpack config.

It's not about debugging, it's about the prod deployment of as storybook being broken. I don't think enabling mangle is safe and it would mean that in Dev mode everything might be fine but the storybook deploy could be broken.

Agreed. I think we need to be conservative in these things.

Do you have an actual non-theoretical example @joscha ?

This saves A LOT of bytes in the production bundle, which is one of the issues I need to resolve to get automatic example deployment working.

I'm gonna go ahead with this unless someone can provide me a non-theoretical reason not to do this.

My example is not theoretical?! We have these problems a the time with Closure Compiler before we write externs for it, that prevent Closure from thinking it can just mangle names. The last one we had was the defaultProps of React components. Another one would be CSS modules - renaming props on there means that object access won't work any more, e.g. where before you could do styles[variable] it would break after. Also anything with eval assuming state being accessible by a named property will break. It saves a lot of bytes, but it's not a safe transformation, that's why it is an option and not on by default. Not making the assumption that just because it will work inside storybook (which we not even know for sure, because the tests don't run against the prod build) for a lot of people using storybook will save these people and the maintainers a lot of time and debugging pain in the future at the expense of a few bytes.

agree with @joscha on this. let's be conservative here and reduce our bundle size by eliminating bogus dependencies. we can also provide a recipe for how to opt-in on this for people who want it.

danielduan · 2017-08-16T21:52:31Z

app/react-native/src/server/config/babel.prod.js

  ],
  plugins: [
-    // function x(a, b, c,) { }


these comments are kinda helpful to figuring out what these transforms do.

Mah, I find the package names pretty self-explanatory. I wonder if I can just switch this to babel-preset-stage-n?

that might make everyone's lives easier.

I actually see these things as an custom babel config templates for users as well. we really don't have any documentation on what sort of plugins users would need to correctly compile storybook with their own configs.

That's because we're not maintaining/documenting babel..

It's just a .babelrc. would you expect us to document/instruct people how to config that?

danielduan · 2017-08-16T21:58:08Z

app/react/src/server/config/babel.js

+      require.resolve('babel-preset-env'),
+      {
+        targets: {
+          browsers: ['last 2 versions', 'safari >= 7'],


we're targeting every browser > 1% in the style prefixer:
https://github.com/storybooks/storybook/blob/master/app/react/src/server/config/defaults/webpack.config.js#L30
I think whichever one we decide on, we should keep the two uniform.

my opinion is that storybook should be on as wide of a platform as we can effortlessly support so users can view what components look like in say IE10 or FF ESR releases. last 2 versions seem really restrictive to me.

Thanks for this remark, what do you suggest we use?

I like what the other config is targeting. maybe we can merge the safari >= 7 in there as well. seems to basically support most browsers in the last 5 years

so users can view what components look like in say IE10

https://www.xfive.co/blog/stop-supporting-ie10-ie9-ie8/

@Hypnosphi we can't make these kind of decisions for developers.

https://www.netmarketshare.com/browser-market-share.aspx?qprid=2&qpcustomd=0

Every tool for or -working in- browsers has made some consideration on how backwards compatible they are.

We can provide what we think is a sane default. We already provide developers with ways to change this.

I vote we target:

IE 11

FF ESR

last 4 versions of Edge, Safari, Chrome, Firefox

danielduan · 2017-08-16T22:00:34Z

app/react/src/server/config/babel.js

    require.resolve('babel-preset-stage-0'),
    require.resolve('babel-preset-react'),
  ],
+  plugins: [
+    require.resolve('babel-plugin-transform-regenerator'),


I'm pretty opposed to this plugin because we don't have it in our code base and I would hope we wouldn't need to polyfill it in the future because of the compiled bundle size and performance on browsers that don't natively support generators.

see generators section:
http://incaseofstairs.com/2015/06/es6-feature-performance/

it should probably be a custom babel config if users are looking for this plugin.

You think we should prevent people from using async/await/generators if not supported by their browser?

On the other hand we have people opening issues expecting support for it.

And in this same PR I'm asked to support browsers from ±5 years ago.

I feel like I'm, in a no-win situation here 😃

I get where you're coming from. enabling this means enabling it for all browsers even the ones that DO support it right?
I agree that's bed considering how much lower the performance is of the transpiled code vs native implementation.

seems like people are using async in their code and are expecting this. i'd vote that we support out of the box it even if it leads to a little bloat. we can also provide a recipe for how to opt out with custom .babelrc?

My gripe with polyfills is that that the older and less powerful the system is, the more performance hit you get. I'm okay with single digit multiples to a certain extent, but when generators polyfill to be 100x-700x slower, an operation that takes 10ms to complete will now take 1s-7s on the thread in FF, IE, and old mobile browsers? you pretty much get a frozen window at that point :(

It does look like a popular request so I'm gonna give in and support this one. I really hope people who are using these polyfills know the performance implication of what they're doing too.

codecov · 2017-08-16T22:32:03Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2392767). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1668   +/-   ##
=========================================
  Coverage          ?   21.13%           
=========================================
  Files             ?      247           
  Lines             ?     5582           
  Branches          ?      668           
=========================================
  Hits              ?     1180           
  Misses            ?     3910           
  Partials          ?      492

Impacted Files	Coverage Δ
app/vue/src/server/config/babel.js	`100% <ø> (ø)`
app/react/src/server/config/babel.prod.js	`0% <ø> (ø)`
app/vue/src/server/config/babel.prod.js	`0% <ø> (ø)`
app/vue/src/server/config/webpack.config.prod.js	`0% <ø> (ø)`
lib/ui/example/webpack.config.js	`0% <ø> (ø)`
app/react/src/server/config/webpack.config.prod.js	`0% <ø> (ø)`
app/react-native/src/server/config/babel.js	`0% <ø> (ø)`
app/react-native/src/server/config/babel.prod.js	`0% <ø> (ø)`
app/react/src/server/config/babel.js	`100% <100%> (ø)`

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 2392767...1f8e843. Read the comment docs.

Hypnosphi · 2017-08-16T22:57:08Z

So do we use both babel-minify and UglifyJS at the same time? Why?

ndelangen · 2017-08-16T22:59:46Z

babel-minify operates per file, and removes dead code
uglify minifies the overall output.

webpack babili plugin would be a replacement for uglify plugin, but i want to wait till that's final.
babili just got renamed to babel-minify, waiting to see what impact this has on the webpack plugin side of things.

ndelangen · 2017-08-16T23:00:39Z

I'd be interested in running prepack on the bundle too.

Hypnosphi · 2017-08-16T23:32:58Z

babel-minify operates per file, and removes dead code

So it's only for dead code in terms of file, unused exports can't be eliminated that way because it runs before webpack, right?

shilman

see my comment about mangle: true

ndelangen · 2017-08-17T16:35:30Z

Thanks everyone for reviewing.

I hope you also feel this PR is an improvement. maybe not quite perfect.
We should improve the method of configuring webpack and babel.

WIP

58b5d6d

ndelangen self-assigned this Aug 16, 2017

ndelangen requested review from shilman, danielduan and a team August 16, 2017 21:48

joscha reviewed Aug 16, 2017

View reviewed changes

danielduan reviewed Aug 16, 2017

View reviewed changes

ADD minify

a9b55dc

ndelangen force-pushed the switchto-babel-preset-env branch from 41fd3a5 to a9b55dc Compare August 16, 2017 22:31

danielduan changed the title ~~Switchto babel preset env~~ Switch to babel preset env Aug 17, 2017

shilman requested changes Aug 17, 2017

View reviewed changes

REVERT mangle setting

39140d8

shilman approved these changes Aug 17, 2017

View reviewed changes

Merge branch 'master' into switchto-babel-preset-env

1f8e843

ndelangen changed the title ~~Switch to babel preset env~~ Switch to babel preset env + async/await/generator support Aug 17, 2017

ndelangen merged commit e86a615 into master Aug 17, 2017

ndelangen deleted the switchto-babel-preset-env branch August 17, 2017 16:35

ndelangen added feature request maintenance User-facing maintenance tasks labels Aug 17, 2017

This was referenced Aug 26, 2017

Support es modules syntax in .config & webpack.config #155

Closed

build-storybook failing: Unexpected token: name (Service) #1402

Closed

vvo mentioned this pull request Aug 28, 2017

Webpack / UglifyJS bundling process issue algolia/react-element-to-jsx-string#140

Closed

valscion mentioned this pull request Sep 14, 2017

babel-plugin-transform-regenerator and babel-plugin-transform-runtime missing from app/react/package.json #1729

Closed

alloy mentioned this pull request Oct 12, 2017

@kanaabe: Refactored article component to fix fullscreen slideshow artsy/reaction#286

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to babel preset env + async/await/generator support #1668

Switch to babel preset env + async/await/generator support #1668

ndelangen commented Aug 16, 2017 •

edited by danielduan

Loading

joscha left a comment

joscha Aug 16, 2017

ndelangen Aug 16, 2017

danielduan Aug 16, 2017

ndelangen Aug 16, 2017

joscha Aug 16, 2017

tmeasday Aug 16, 2017

ndelangen Aug 17, 2017

joscha Aug 17, 2017

shilman Aug 17, 2017

ndelangen Aug 17, 2017

danielduan Aug 16, 2017

ndelangen Aug 16, 2017

danielduan Aug 16, 2017 •

edited

Loading

ndelangen Aug 17, 2017

danielduan Aug 16, 2017 •

edited

Loading

ndelangen Aug 16, 2017

danielduan Aug 16, 2017

Hypnosphi Aug 16, 2017 •

edited

Loading

tmeasday Aug 16, 2017

ndelangen Aug 17, 2017

ndelangen Aug 17, 2017

danielduan Aug 17, 2017

danielduan Aug 16, 2017 •

edited

Loading

ndelangen Aug 17, 2017

shilman Aug 17, 2017

danielduan Aug 17, 2017 •

edited

Loading

codecov bot commented Aug 16, 2017 •

edited

Loading

Hypnosphi commented Aug 16, 2017

ndelangen commented Aug 16, 2017

ndelangen commented Aug 16, 2017

Hypnosphi commented Aug 16, 2017

shilman left a comment

ndelangen commented Aug 17, 2017

Switch to babel preset env + async/await/generator support #1668

Switch to babel preset env + async/await/generator support #1668

Conversation

ndelangen commented Aug 16, 2017 • edited by danielduan Loading

What I did

How to test

joscha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduan Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduan Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduan Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduan Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2017 • edited Loading

Codecov Report

Hypnosphi commented Aug 16, 2017

ndelangen commented Aug 16, 2017

ndelangen commented Aug 16, 2017

Hypnosphi commented Aug 16, 2017

shilman left a comment

Choose a reason for hiding this comment

ndelangen commented Aug 17, 2017

ndelangen commented Aug 16, 2017 •

edited by danielduan

Loading

danielduan Aug 16, 2017 •

edited

Loading

danielduan Aug 16, 2017 •

edited

Loading

Hypnosphi Aug 16, 2017 •

edited

Loading

danielduan Aug 16, 2017 •

edited

Loading

danielduan Aug 17, 2017 •

edited

Loading

codecov bot commented Aug 16, 2017 •

edited

Loading