Skip to content

Bundle umd with rollup #681

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 1 commit into from
May 18, 2017
Merged

Bundle umd with rollup #681

merged 1 commit into from
May 18, 2017

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Apr 15, 2017

82 798 -> 65 835
19 181 -> 12 773

Ref #680

@TrySound
Copy link
Contributor Author

Any update on this?
/cc @timdorr

@timdorr
Copy link
Member

timdorr commented Apr 24, 2017

Sorry, I've been sidetracked. I'll be looking into this soon!

"rollup": {
"plugins": [
"./build/use-lodash-es",
"external-helpers"
Copy link
Member

Choose a reason for hiding this comment

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

So, do we have to externalize Babel's helpers? This introduces a rather opaque dependency, since it's not explicitly listed in package.json or anything. What if we use transform-runtime instead to centralize helpers within the build? Does that bloat things a lot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And then tree shaking removes unnecessary helpers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, confused by github reviews.
rollup-plugin-babel calls buildExternalHelpers, adds it as virtual module and then tree shake unnecessary code. So all in the same bundle, not external package

Copy link
Member

Choose a reason for hiding this comment

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

Well, transform-runtime should only add used helpers by itself. But there can be a tree of helpers using other helpers underneath that. I would just ensure the { polyfills: false } option is used so it doesn't try to polyfill everything. That can be left up to the user.

Copy link
Contributor Author

@TrySound TrySound May 17, 2017

Choose a reason for hiding this comment

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

It doesn't try to polyfill everything. Used is only what added by plugins in .babelrc. And it looks like runtime plugin adds core-js stuff under the hood. And it's common js afaik.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but I don't know how to best advertise to UMD users that they need this thing that isn't in the dependency list. It seems really error-prone.

I'll do some checking into this when I have a chance later tonight. I'm sure this stuff can be inlined without causing a ton of bloat. It's a babel issue, not webpack or rollup's job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you want one more dependency for umd users? Now helpers are builtin with umd bundle. You want to make life harder?

Copy link
Member

Choose a reason for hiding this comment

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

No, I'm saying don't make them external. If you do, that becomes a dependency. transform-runtime inlines and centralizes helpers, so they don't need to be imported by the user, but are still efficient because there is only ever one copy of a helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and that what rollup-plugin-babel do. External plugin just add namespace to helpers which is used by rollup-plugin-babel. But that happen without additional and not always efficient work by commonjs plugin and expliit dependency which developer should specify instead of what exactly plugins require.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I apparently have a fundamental misunderstanding of babel-plugin-external-helpers or rollup-plugin-babel does :P

external-helpers is supposed to tell the transpiled code to look at global for the babelHelpers property to find them. That means they should be external (like the name implies) from the build and included via a separate <script> tag. For whatever reason, rollup-plugin-babel actually treats them like an inline-able dependency, which is what transform-runtime is supposed to do.

Anyways, that's just my 2 cents. Since the naming is backwards, this is actually what we want and I was wrong about it originally :)

@timdorr
Copy link
Member

timdorr commented May 18, 2017

OK, this builds A-OK, so we're good to go. Here's what the output looks like for the curious: https://gist.github.com/timdorr/997620e0980f53087fdbef36d6a6f750#file-react-redux-js

@timdorr timdorr merged commit a7a11fc into reduxjs:master May 18, 2017
josepot pushed a commit to josepot/react-redux-lean that referenced this pull request Sep 21, 2018
albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants