-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[core] Bundle UMD with rollup #11360
Conversation
@TrySound Thanks for the pull-request. I have been busy with the v1-rc.0 release. I will have a look at the pull-request tomorrow :) |
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.
Thanks for the pull-request
import babel from 'rollup-plugin-babel'; | ||
import replace from 'rollup-plugin-replace'; | ||
import uglify from 'rollup-plugin-uglify'; | ||
import { sizeSnapshot } from 'rollup-plugin-size-snapshot'; |
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 are already using https://github.com/ai/size-limit. I believe it's answering the same problem. Shouldn't we use a single tool?
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.
size limit creates limits. My tool is about showing different sizes.
- size of bundle processed with bundler
- size of bundle loader with browser
- size of bundle parsed with browser
- treeshaked size for esm bundles
For it's more important to see these values than moving limits over and over again.
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.
Does it have a mode where it only outputs in the console and doesn't write in the filesystem? I'm wondering about constantly having to commit the snapshot.
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.
Isn't it the point to show users these numbers?
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.
I figured out that I can live with that 👍
|
||
const getBabelOptions = () => ({ | ||
exclude: /node_modules/, | ||
runtimeHelpers: 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.
What is this option for? I can't find it on React side https://github.com/facebook/react/blob/master/scripts/rollup/build.js.
Alternatively, if you know what you're doing, you can use the transform-runtime plugin. If you do this, use runtimeHelpers: true:
https://github.com/rollup/rollup-plugin-babel#helpers
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 you rely on react here? They has quite specific config. For libs it can be simpler. runtimeHelpers should allow to use babel-runtime instead of inlined helpers (which is default behaviour of this plugin).
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.
Should we be using babel-runtime instead?
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.
I'm surprised we have to provide an option to Rollup. Babel was taking care of it on its own with wepback. For the common js output, we only have to add the plugin in the .babelrc.
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.
Default behavior of rollup plugin is smarter than webpack loader which inlines helpers in every file. Rollup plugin inlines them once.
'react-dom': 'ReactDOM', | ||
}; | ||
|
||
const getBabelOptions = () => ({ |
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.
Something is off. Some babel plugins declared in .babelrc aren't applied. For instance, we still have the data-mui-test
properties.
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.
Yeah, current babelrc is a mess to be honest. It's better to specify targets in browserlist file and completely separate environments with only NODE_ENV.
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.
It seems NODE_ENV=production
isn't taken into account by rollup-babel. But I could be wrong.
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.
Its done by rollup plugin replace.
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.
That's why sizes of bundles are different
Sizes before and after webpack material-ui.development.js 2.54M material-ui.production.min.js 436.3K rollup material-ui.development.js 1.01M material-ui.production.min.js 370.1K Added size snapshot to track bundle size change on every commit.
9e281ef
to
d7600de
Compare
d7600de
to
fce1eb3
Compare
@TrySound It's a great first pull request on Material-UI 👌🏻. Thank you for giving it a shot! |
Sizes before and after
webpack
material-ui.development.js 2.54M
material-ui.production.min.js 436.3K
rollup
material-ui.development.js 1.01M
material-ui.production.min.js 370.1K
Added size snapshot to track bundle size change on every commit.