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

Replace UglifyJS with babel-minify #1733

Closed

Conversation

ndelangen
Copy link
Member

This came up in: #1501
A node_module we want to use has not been transpiled to es5 before publishing.

We could add the module to be run through babel, or we could do this:
Switch over to an alternative minifier. This PR swaps UglifyJS for Babel-Minify.

Why? Well the browsers we target actually support 90%+ of ES6+. Transpiling down

Related issue: #1005

This only affects static builds!

How to test

Run yarn build-storybook on cra-kitchen-sink
Serve the static site

What about bundle size?

1.396.924 bytes (1,4 MB on disk)
That's down considerably from before:
2.032.284 bytes (2 MB on disk)

UglifyJS does not support any ES6 whatsoever. We want to allow users to set their own target env.
@ndelangen
Copy link
Member Author

screen shot 2017-08-24 at 23 12 46

screen shot 2017-08-24 at 23 15 03

@ndelangen ndelangen changed the base branch from master to release/3.3 August 24, 2017 21:45
@codecov
Copy link

codecov bot commented Aug 24, 2017

Codecov Report

Merging #1733 into release/3.3 will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #1733      +/-   ##
===============================================
- Coverage        22.77%   22.77%   -0.01%     
===============================================
  Files              326      326              
  Lines             6753     6754       +1     
  Branches           829      841      +12     
===============================================
  Hits              1538     1538              
+ Misses            4621     4607      -14     
- Partials           594      609      +15
Impacted Files Coverage Δ
app/vue/src/server/config/webpack.config.prod.js 0% <ø> (ø) ⬆️
app/react/src/server/config/webpack.config.prod.js 0% <ø> (ø) ⬆️
...ct-native/src/server/config/webpack.config.prod.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Tabs.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/PropField.js 10.63% <0%> (ø) ⬆️
addons/viewport/src/components/SelectViewport.js 15% <0%> (ø) ⬆️
.../src/modules/ui/components/stories_panel/header.js 29.62% <0%> (ø) ⬆️
addons/info/src/components/Props.js 37.2% <0%> (ø) ⬆️
addons/viewport/src/components/viewportInfo.js 36.36% <0%> (ø) ⬆️
addons/info/src/components/markdown/code.js 24.13% <0%> (ø) ⬆️
... and 28 more

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 a52a91d...4280d32. Read the comment docs.

@usulpro
Copy link
Member

usulpro commented Aug 24, 2017

I tried to test it locally with my branch and it takes about 8 min to finish building
All except this works fine! 👍

(I will try to clean everything and run it again tomorrow)

@ndelangen
Copy link
Member Author

Wow, that's a long time.. For me it was about 80 seconds

@usulpro
Copy link
Member

usulpro commented Aug 25, 2017

got 87 sec today!

Copy link
Member

@usulpro usulpro left a comment

Choose a reason for hiding this comment

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

I'd love to have it merged

@ndelangen
Copy link
Member Author

I want someone to verify react-native other then me

@tmeasday
Copy link
Member

I tried it with RN-vanilla (just ran the storybook as usual), and it worked fine. How do I run a build with RN?

@tmeasday
Copy link
Member

I'm not sure that build is implemented yet for RN (https://github.com/storybooks/storybook/tree/master/app/react-native/src/bin), so I wonder if the prod babel config for RN serves any purpose?

@ndelangen
Copy link
Member Author

@tmeasday I've looked into this, and it would appear there is no production build possible for RN.
This kinda makes sense to me.

I'm hoping @mnmtanish can verify?

@Hypnosphi
Copy link
Member

So do those errors happen foryou in IE11 only, or in other browsers as well?

@igor-dv
Copy link
Member

igor-dv commented Oct 1, 2017

In Chrome and FF

@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 1, 2017

I can confirm this in Chrome on mac as well (when running build-storybook + http-server)

@Hypnosphi
Copy link
Member

The problem was the name mangling, plus the fact that we have to use eval in addon-actions

@igor-dv
Copy link
Member

igor-dv commented Oct 1, 2017

Nice! But what is the bundle size now ?

@Hypnosphi
Copy link
Member

For cra-kitchen-sink, it's 3.2M vs 2.9M on release/3.3 (but mangling is disabled there as well)

@danielduan
Copy link
Member

This feels like something that's valuable for us to get merged.

@evan-scott-zocdoc
Copy link

uglifyjs-webpack-plugin v1 is out, which has full ES6+ support btw. https://github.com/webpack-contrib/uglifyjs-webpack-plugin

@ndelangen
Copy link
Member Author

ndelangen commented Nov 16, 2017

Latest stats:

before 'mangle'

manager = 1.915.735 bytes (1,9 MB on disk)
preview = 206.408 bytes (209 KB on disk)
vendor = 966.175 bytes (967 KB on disk)

after 'mangle'

manager = 1.332.127 bytes (1,3 MB on disk)
preview = 118.647 bytes (119 KB on disk)
vendor = 698.993 bytes (700 KB on disk)

@danielduan
Copy link
Member

@ndelangen you mean mangle right?

I still don't think we should be using mangle but anything else we can do to get the bundle size down is a plus for me.

@Hypnosphi Hypnosphi added this to the v3.3.0 milestone Nov 17, 2017
@Hypnosphi
Copy link
Member

I'd prefer to keep avoiding mangling. Even in the case of static build, storybook is still a developer tool, and debugging experience should be among top priorities. And I believe we actually display some original variable and function names in addons like info and actions.

@Hypnosphi
Copy link
Member

@shilman is there anything else that should be done here in your opinion?

@Hypnosphi
Copy link
Member

@ndelangen we need to apply the same changes to angular app

@ndelangen
Copy link
Member Author

Putting this back into the freezer, gonna have a look at the uglify-webpack-plugin that was mentioned: #1733 (comment)

@Hypnosphi Hypnosphi removed this from the v3.3.0 milestone Nov 28, 2017
@ggarek
Copy link
Contributor

ggarek commented Dec 22, 2017

@ndelangen Thank you for looking into this issue! Since i am also interested in this fix and i had some free time, i tried to use uglify-es.

Please, check it when you have time

@ndelangen
Copy link
Member Author

Thank you @ggarek, I will close this PR in favour of yours!

@ndelangen ndelangen closed this Dec 22, 2017
@Hypnosphi Hypnosphi deleted the ndelangen/replace-uglify-with-babel-minify branch February 17, 2018 13:44
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.

10 participants