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

Use Closure Compiler for UMD/Node bundles instead of Uglify #10236

Merged
merged 12 commits into from Aug 10, 2017

Conversation

Projects
None yet
7 participants
@trueadm
Contributor

trueadm commented Jul 20, 2017

This change introduces Google Closure Compiler (JS version) in the build pipeline instead of Uglify for the minifcation process for UMD/Node bundles. Locally, I'm seeing positive bundle size wins:

screen shot 2017-07-20 at 17 15 51

We need to thoroughly test these new builds before this can be merged however.

@trueadm trueadm requested a review from gaearon Jul 20, 2017

@trueadm trueadm requested a review from sophiebits Jul 20, 2017

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 20, 2017

Member

@trueadm Do you want to set up a Jest project for flat bundles and figure out a way to opt in a few tests into it? I imagine it working the following way:

  • It’s a separate Jest project configuration (see configurations @sebmarkbage added in #10214).
  • It should not run by default on local npm test but should run on CI after the build step.
  • It should only run for whitelisted tests. We can probably start with an explicit list of filenames. Later we can include whole directories or can use a folder name convention.
  • It should run both for DEV and PROD bundles. These can be separate “projects”, and we can start with PROD. In PROD, expectDev() calls should be ignored.
Member

gaearon commented Jul 20, 2017

@trueadm Do you want to set up a Jest project for flat bundles and figure out a way to opt in a few tests into it? I imagine it working the following way:

  • It’s a separate Jest project configuration (see configurations @sebmarkbage added in #10214).
  • It should not run by default on local npm test but should run on CI after the build step.
  • It should only run for whitelisted tests. We can probably start with an explicit list of filenames. Later we can include whole directories or can use a folder name convention.
  • It should run both for DEV and PROD bundles. These can be separate “projects”, and we can start with PROD. In PROD, expectDev() calls should be ignored.
@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Jul 21, 2017

Contributor

Seems to be something odd going on with Prettier, as it keeps failing on CI yet running Prettier locally results in no file changes. @vjeux is there something I'm missing?

Contributor

trueadm commented Jul 21, 2017

Seems to be something odd going on with Prettier, as it keeps failing on CI yet running Prettier locally results in no file changes. @vjeux is there something I'm missing?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 21, 2017

Member

Did you run yarn locally?

Member

gaearon commented Jul 21, 2017

Did you run yarn locally?

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Jul 21, 2017

Contributor

@gaearon Yes, it didn't fix the problems I was having :/

Contributor

trueadm commented Jul 21, 2017

@gaearon Yes, it didn't fix the problems I was having :/

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 21, 2017

Member

Can you separate Prettier related changes into another PR? I don't quite understand why they are related to this PR. Maybe separating will also make it easier to understand what went wrong with the branch.

Member

gaearon commented Jul 21, 2017

Can you separate Prettier related changes into another PR? I don't quite understand why they are related to this PR. Maybe separating will also make it easier to understand what went wrong with the branch.

@vjeux

I'd be happy to look into it but I need more details. What exactly is conflicting, what is the before/after...

In my experience, it's usually a problem where the two versions are not in sync.

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Aug 9, 2017

Contributor

@flarnie I'm really eager to get this PR landed for the next release (beta?).

I'll do more manual tests tomorrow and locally test the UMD bundles but it kind of needs public support as the bundles (UMD/CJS) aren't used on FB www.

What do you think?

Contributor

trueadm commented Aug 9, 2017

@flarnie I'm really eager to get this PR landed for the next release (beta?).

I'll do more manual tests tomorrow and locally test the UMD bundles but it kind of needs public support as the bundles (UMD/CJS) aren't used on FB www.

What do you think?

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 10, 2017

Contributor

Thanks for tagging me @trueadm :)

Here is my thought - if most folks in open source are waiting for the RC or final 16.0 release to try the latest React, then the UMD flat bundles have not been well tested by the community any way at this point. So they will need public support after each beta/RC release either way, regardless of whether we merge this.

If we wait, we may end up fixing bugs that are present with Uglify and not with the GCC (Google Closure Compiler) and so we might as well merge this.

I can try to keep an eye on issues - is there anything in particular that we know to be risky about this change?

Contributor

flarnie commented Aug 10, 2017

Thanks for tagging me @trueadm :)

Here is my thought - if most folks in open source are waiting for the RC or final 16.0 release to try the latest React, then the UMD flat bundles have not been well tested by the community any way at this point. So they will need public support after each beta/RC release either way, regardless of whether we merge this.

If we wait, we may end up fixing bugs that are present with Uglify and not with the GCC (Google Closure Compiler) and so we might as well merge this.

I can try to keep an eye on issues - is there anything in particular that we know to be risky about this change?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 10, 2017

Member

I think this is OK to merge. Most likely syntax errors in old browsers, I guess? It doesn't feel super likely that closure will introduce logic errors.

Member

sophiebits commented Aug 10, 2017

I think this is OK to merge. Most likely syntax errors in old browsers, I guess? It doesn't feel super likely that closure will introduce logic errors.

@flarnie flarnie self-requested a review Aug 10, 2017

@flarnie

🥅💨

Show outdated Hide outdated scripts/rollup/results.json Outdated
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 10, 2017

Member

I'm curious... will it get larger again when people pass it through Uglify? Since normally people do it as a build step. It's still nice to have smaller official UMD bundles but seems like people can't take advantage of them unless they explicitly load them as a separate file we packaged.

Member

gaearon commented Aug 10, 2017

I'm curious... will it get larger again when people pass it through Uglify? Since normally people do it as a build step. It's still nice to have smaller official UMD bundles but seems like people can't take advantage of them unless they explicitly load them as a separate file we packaged.

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Aug 10, 2017

Contributor

@gaearon That's a good point. I tried it locally and it doesn't seem to make the bundles any bigger for me – which is the good news! I'm merging this as per comments above.

Contributor

trueadm commented Aug 10, 2017

@gaearon That's a good point. I tried it locally and it doesn't seem to make the bundles any bigger for me – which is the good news! I'm merging this as per comments above.

@trueadm trueadm merged commit e97143c into facebook:master Aug 10, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details
@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 10, 2017

Member
Member

sophiebits commented Aug 10, 2017

@MAPESO

This comment has been minimized.

Show comment
Hide comment
@MAPESO

MAPESO Aug 10, 2017

@spicyj Refers to the compression of files JS

MAPESO commented Aug 10, 2017

@spicyj Refers to the compression of files JS

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Aug 10, 2017

Contributor

@spicyj Sorry, I wasn't very clear.

React: 6kb (GCC) and 6kb (GCC + Uglify)
ReactDOM: 115kb (GCC) and 115kb (GCC + Uglify)

Hope that explains things better.

Contributor

trueadm commented Aug 10, 2017

@spicyj Sorry, I wasn't very clear.

React: 6kb (GCC) and 6kb (GCC + Uglify)
ReactDOM: 115kb (GCC) and 115kb (GCC + Uglify)

Hope that explains things better.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 10, 2017

Member

Have you verified using them via Webpack+Uglify doesn’t make them worse?

Member

gaearon commented Aug 10, 2017

Have you verified using them via Webpack+Uglify doesn’t make them worse?

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Aug 11, 2017

Contributor

@gaearon I get 120kb for React+ReactDOM bundle using webpack in production mode. So it looks like we're all good here.

Contributor

trueadm commented Aug 11, 2017

@gaearon I get 120kb for React+ReactDOM bundle using webpack in production mode. So it looks like we're all good here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment