-
Notifications
You must be signed in to change notification settings - Fork 35
Conversation
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.
Looks like a clean update, following along with upgrade guides for babel/webpack. Asking a few questions.TODO : test after merge to ensure all is well (across all browsers)
plugins: [ | ||
new webpack.HotModuleReplacementPlugin(), | ||
new webpack.DefinePlugin({ | ||
'process.env': { | ||
NODE_ENV: JSON.stringify('development') |
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.
Okay, nice! https://webpack.js.org/concepts/mode/
['transform-runtime', { helpers: true, polyfill: false }] | ||
'@babel/plugin-proposal-object-rest-spread', | ||
'@babel/plugin-proposal-class-properties', | ||
['@babel/plugin-transform-runtime', { helpers: true }] |
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.
We no longer polyfill ?
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.
The polyfill option was removed https://babeljs.io/docs/en/babel-plugin-transform-runtime (defaulting to False, see https://github.com/babel/babel/blob/f6643d1804c977cc7e9f769ad24d7baeb8b9a5ed/packages/babel-plugin-transform-runtime/src/index.js#L100 the doc is misleading)
We still include @babel/polyfill
in index.jsx
that is reduced with babel-preset-env (useBuiltIns=entry)
Here we use transform-runtime with helpers: true
to reduce the bundle of some kB.
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.
Nice, good answer. Thank you, it's more clear now.
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.
LGTM. Thanks!
@@ -96,10 +98,6 @@ module.exports = { | |||
exclude: /node_modules/, | |||
use: 'graphql-tag/loader' | |||
}, | |||
{ |
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.
why don't we need this anymore?
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.
json-loader ? webpack handles json import natively since webpack 2
I can rebase and merge when the other big PRs are merged and no big branches are pending, in a few weeks. There is no hurry. |
I had to update storybook to latest 4.0.0-rc.0 which use webpack 4 and babel 7 to fix tests issue after the rebase. BUT storybook 4 now needs node >= 8.0.0, you can't run |
6e647d2
to
a1aef4b
Compare
39d6718
to
e639ece
Compare
Update to
mode: 'production'/'development'
(new UglifyJSPlugin({ sourceMap: true, parallel: true, cache: true })
is used by default in production modedevelop branch:
first build (without uglify cache): 1m54.120s
second build: 0m54.846s
without uglify: 0m38.620s
4M, 4102630 bytes with uglify
10M, 10410066 bytes without uglify
babel7 branch:
first build (without uglify cache): 1m58.283s
second build 0m59.051s
without uglify: 0m44.785s
3,9M 4033232 bytes with uglify
8,5M, 8886172 bytes without uglify