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

Update Rollup #11427

Merged
merged 3 commits into from Nov 2, 2017
Merged

Update Rollup #11427

merged 3 commits into from Nov 2, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 2, 2017

Seems like we'll need to do this to get a new option that was added in 0.43. I updated to 0.49 as it was out for a while, and doesn't have the regressions from more aggressive 0.50.

The size has increased but the reason seems to be mostly duplication of 'use strict' that's now added in the bundle many times (for every module?). I don't understand why yet. We'll get rid of them with ES modules though (I think) so maybe not a big deal.

screen shot 2017-11-02 at 01 03 32

screen shot 2017-11-02 at 01 05 17

Diff: https://gist.github.com/gaearon/8de8c9d5f47c76061a900e16c938872c

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

I agree that the "use-strict" duplication is fine. 👍

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 2, 2017

It's not great to inflate bundle size because of it though. I'll land it only if I verify the ES modules PR gets rid of it when laid on top.

@nhunzaker
Copy link
Contributor

Ah, that was where I was at. That the ES Module update would collapse the closures around the CJS modules and Uglify (or Rollup) would clean up.

Assuming those assumptions are correct...

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 2, 2017

It doesn't help by itself, no. But I was hoping it will let us delete use strict in our source (since it's implicitly strict in modules) and hopefully there'll then be nothing for Rollup to duplicate.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 2, 2017

Regarding use strict, seems intentional: rollup/rollup#1591.

Maybe we should just do a pass and remove it ourselves since we know the whole bundle has it anyway.

@nhunzaker
Copy link
Contributor

Sounds laborious, but maybe just a project replace. Okay if I take a stab? Should I base that off of this branch, or the ES Module branch?

@nhunzaker
Copy link
Contributor

Sent out a PR with a basic script to do this en-mass, happy to pick it up again tomorrow. gaearon#2

@gaearon gaearon merged commit fbf617a into facebook:master Nov 2, 2017
Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
* Update Rollup

* Strip "use strict" in individual modules

* Record sizes
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.

None yet

3 participants