Initial work to support advanced compilation of UMD bundles using GCC #11967

banga · 2018-01-05T09:21:48Z

As described in #11092, this PR adds support for passing in externs to GCC to allow it to compile bundles in advanced mode without renaming properties that are part of the public API. Most of the externs were taken from https://github.com/cljsjs/packages/tree/master/react/resources/cljsjs/, although some more needed to be added. I've split them into various files with necessary comments so they should be reasonably easy to follow.

At this point, the node bundles do not build correctly (the module.exports part is stripped out), so we can't run tests just yet. So I just tested by pointing the babel-standalone fixture to the built prod files and loading it. The current fixture works successfully, and I was able to create a functional component as well. But when I tried creating a component by extending React.Component, I saw class constructor cannot be invoked without 'new', indicating that another property is getting renamed and causing the functional component codepath to execute for non-functional components.

(This is my first PR to an FB project, so I just submitted the CLA.)

Get all test-build-prod tests to pass
Don't use fbjs externs for UMD bundles for better minification
Clean up externs files to remove old methods that no longer exist
Try Splitting JS into modules to allow minifying react internal properties accessed across bundle boundaries

This makes closure options dynamic, based on bundle type, since UMD and Node bundles expect different things in their externs. At this point, this can build the UMD_PROD versions of core and dom-client and load the babel-standalone fixture with the compiled react and react-dom files.

The built bundle is not correct yet because it's missing module.exports.

sophiebits · 2018-01-05T18:28:07Z

That error is likely caused by renaming isReactComponent.

gaearon · 2018-01-05T18:52:51Z

externs/react.ext.js

+React.version;
+
+React.createClass = function(specification) {};
+React.createFactory = function(reactClass) {};


A lot of these are outdated. Once we get the tests passing we should trim these down significantly.

banga · 2018-01-05T23:22:22Z

@sophiebits yeah that was it. I got the node bundles to build correctly last night and got a handful of test-build-prod tests to pass. I'll clean-up my changes and push tonight. The most frequent error that was breaking tests now was Target Container is not a DOM Element, which might be too generic to debug just from that, but let me know if you have ideas.

gaearon · 2018-01-06T00:51:42Z

Can't wait 😍

appsforartists · 2018-01-06T03:42:56Z

The Angular team has tools (tsickle and clutz) to convert between TypeScript and Closure annotations. I haven't tested them myself yet, so I don't know how well they work, but I believe the goal is to enable TypeScript applications to use advanced GCC.

It may be worth exploring using tsickle to auto-gen your Closure externs. I'm not sure if it would work on Flow code, but I bet @evmar is amenable to PRs.

I suspect that automatically generating them is better in the long run than maintaining the externs by hand.

The missing isReactComponent was causing Component subclasses to be treated as functional components. The missing properties on ReactElement were breaking other type dependant behavior.

The browser environment helps ensure that dom attributes don't get renamed, for example. With processCommonJsModules set to true, gcc was removing the module.exports from the compiled bundles.

So we don't have to keep defining these for each bundle

Event -> FakeEvent hasOwnProperty -> objHasOwnProperty escape -> escapeKey Suppressed the error on ReactART.Text since it's exported

Also suppress duplicate warnings because now that we don't use processCommonJsModules, these are treated as global variables.

This is needed by both UMD and NODE bundles, even though the NODE bundle generates a duplicate warning.

These are treated as external dependencies in node bundles, so have to prevent renaming them. We should eventually still rename these in UMD bundles.

gaearon · 2018-01-06T03:44:56Z

One thing that’s nice in our case is that the API surface is relatively small. So once we get them right for the first time they’re not as much pain to maintain.

GCC seems to incorrectly remove all of the properties from `RESERVED_PROPS` because they are never accessed by name. Replaced this with a Set since that was the intent anyway.

This allows us to run prettier even after compiling in ADVANCED mode, which can be really useful when debugging problems with the minified output.

banga · 2018-01-06T06:03:59Z

Alright after today's work, we have 222 out of 244 tests passing in the react package:
.

evmar · 2018-01-06T08:17:36Z

@appsforartists Within Google we do generate externs for React from the DefinitelyTyped React d.ts files. The API surface is a bit larger than you'd expect due to including all the CSS properties etc.

But it's also probably more complicated than what you want because:

the d.ts files include lots of type definitions (including fancy generics to model setState() etc)
they ended up needing some fixing by hand at the end due to various bugs (in how we handle said fancy generics -- Closure can't model some of them)
if you're just worrying about the untyped Closure optimizations you don't need the types.

Oh, and now that I think harder about it, these d.ts files are for clients of React to use the minified bundle, while here you are concerned about preventing bits of React's internal API from getting renamed. (You might find this section of the docs useful in that respect.)

banga · 2018-01-06T08:26:37Z

Yeah, the more challenging part seems to be the non-public API shared between the bundles (e.g. FiberNode and updater). We need to either prevent them from being renamed, or find a way to compile everything together and split the bundles so they are renamed consistently. So far I'm just manually doing that, with some hints from exported types.

stereobooster · 2018-01-06T08:39:41Z

When Dan twitted about using GCC for minification I thought about "GNU Compiler Collection", but this was about "Google Closure Compiler" 🤣

…_FIRED These are used extensively by react-dom etc., so best to keep them all in externs instead of trying to whack-a-mole.

These are needed in both UMD and NODE bundles, plus these were outdated.

These are useful for understanding why a test is failing in prod.

Removed unnecessary annotations and updated the externs for React 16

banga · 2018-01-07T05:10:55Z

Ok now all react and react-dom tests are passing!

gaearon · 2018-01-07T16:48:53Z

We probably don’t need to apply advanced mode to any bundles except React and ReactDOM. Others’ size is less critical / important.

gaearon · 2018-01-07T16:50:35Z

It’s also worth exploring a subset of this PR that only applies advanced to the React bundle. It’s not as important as ReactDOM but it’s a good way to get started since it’s fairly small and won’t need a ton of externs. That would be relatively safe to merge while we continue to iterate on ReactDOM.

sophiebits · 2018-01-07T17:56:12Z

If we do run it on all bundles, let’s have separate externs for each bundle. There are some names we want to munge in react-dom but not react-reconciler, for instance.

banga · 2018-01-07T21:13:14Z

@gaearon sounds good. Should I start a separate PR for just compiling the react bundle?

@sophiebits yup, I’ve been keeping the externs separate by bundle type, although some bundles need to pull in externs for others (eg react-dom needs react externs)

gaearon · 2018-01-08T02:29:14Z

Yes, a separate more scoped PR that can pass a CI would be great. Thank you for working on this!

facebook-github-bot added the CLA Signed label Jan 5, 2018

Allow building node bundles

acbf6ae

The built bundle is not correct yet because it's missing module.exports.

gaearon reviewed Jan 5, 2018

View reviewed changes

Shrey Banga added 10 commits January 5, 2018 19:44

Add more missing externs

706e247

The missing isReactComponent was causing Component subclasses to be treated as functional components. The missing properties on ReactElement were breaking other type dependant behavior.

Switch to BROWSER env and don't process commonjs modules

8f6eb00

The browser environment helps ensure that dom attributes don't get renamed, for example. With processCommonJsModules set to true, gcc was removing the module.exports from the compiled bundles.

Move common externs to react.ext.js

0edde2d

So we don't have to keep defining these for each bundle

Fix some "duplicate name" errors from GCC

1ec3e29

Event -> FakeEvent hasOwnProperty -> objHasOwnProperty escape -> escapeKey Suppressed the error on ReactART.Text since it's exported

Remove externs obviated by BROWSER

ca08db9

Also suppress duplicate warnings because now that we don't use processCommonJsModules, these are treated as global variables.

Move plugins to react-dom.ext.js

d828f31

This is needed by both UMD and NODE bundles, even though the NODE bundle generates a duplicate warning.

Add more externs to get more tests to work

9a43caf

Add externs for names used from fbjs

0f6dca5

These are treated as external dependencies in node bundles, so have to prevent renaming them. We should eventually still rename these in UMD bundles.

Add externs to get remaining bundles to compile

5d89dff

Check-in compilation results

923dc07

Shrey Banga added 6 commits January 5, 2018 19:55

run prettier

8508a5e

Prevent GCC from emptying RESERVED_PROPS

7d50185

GCC seems to incorrectly remove all of the properties from `RESERVED_PROPS` because they are never accessed by name. Replaced this with a Set since that was the intent anyway.

Add externs for FiberNode

c8a14d3

Add missing extern for Component.defaultProps

0aca7a7

Decouple forcePrettyOutput from shouldStayReadable

2b23c2b

This allows us to run prettier even after compiling in ADVANCED mode, which can be really useful when debugging problems with the minified output.

run prettier

d0b197b

Shrey Banga added 3 commits January 6, 2018 00:15

Add a missing extern on ReactDOM

1930287

Add more fiber externs

7b01e20

Add externs for react-call-return

67b2f3b

stereobooster mentioned this pull request Jan 6, 2018

Switch to ES Modules with Rollup mapbox/mapbox-gl-js#5939

Closed

Shrey Banga added 13 commits January 6, 2018 02:00

Add externs for ReactDOMServer and stream.Readable

ed620f2

Fix test failures because of suppressReactErrorLogging getting renamed

505945b

Update externs for SyntheticEvent

9146ac4

Add complete externs for __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE…

1b98ddd

…_FIRED These are used extensively by react-dom etc., so best to keep them all in externs instead of trying to whack-a-mole.

Fix externs for BrowserEventConstants

8bfc5e4

These are needed in both UMD and NODE bundles, plus these were outdated.

Add missing fbjs extern for ExecutionEnvironment.canUseDOM

4ddec66

Avoid duplicating react-dom externs

af16528

Add debug-test-build and debug-test-build-prod

4b2234f

These are useful for understanding why a test is failing in prod.

Clean-up ReactDOM externs

6fb8cce

Removed unnecessary annotations and updated the externs for React 16

Make ReactInstanceMap work correctly

436c08c

Stop renaming mockImplementation for tests

629dbc2

Add externs for CSSProperty

f14d054

Add externs for test-renderer

23ef5a6

banga mentioned this pull request Jan 8, 2018

Compile the react bundle with GCC advanced mode #11995

Closed

gaearon mentioned this pull request Jan 10, 2018

Run 90% of tests on compiled bundles (both development and production) #11633

Merged

Deraen mentioned this pull request Mar 13, 2018

RESERVED_PROPS is optimized away by Google Closure Compiler #12368

Closed

banga closed this Feb 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial work to support advanced compilation of UMD bundles using GCC #11967

Initial work to support advanced compilation of UMD bundles using GCC #11967

banga commented Jan 5, 2018 •

edited

Loading

sophiebits commented Jan 5, 2018

gaearon Jan 5, 2018

banga commented Jan 5, 2018

gaearon commented Jan 6, 2018

appsforartists commented Jan 6, 2018

gaearon commented Jan 6, 2018

banga commented Jan 6, 2018

evmar commented Jan 6, 2018 •

edited

Loading

banga commented Jan 6, 2018

stereobooster commented Jan 6, 2018

banga commented Jan 7, 2018 •

edited

Loading

gaearon commented Jan 7, 2018 •

edited

Loading

gaearon commented Jan 7, 2018

sophiebits commented Jan 7, 2018

banga commented Jan 7, 2018

gaearon commented Jan 8, 2018

Initial work to support advanced compilation of UMD bundles using GCC #11967

Initial work to support advanced compilation of UMD bundles using GCC #11967

Conversation

banga commented Jan 5, 2018 • edited Loading

sophiebits commented Jan 5, 2018

gaearon Jan 5, 2018

Choose a reason for hiding this comment

banga commented Jan 5, 2018

gaearon commented Jan 6, 2018

appsforartists commented Jan 6, 2018

gaearon commented Jan 6, 2018

banga commented Jan 6, 2018

evmar commented Jan 6, 2018 • edited Loading

banga commented Jan 6, 2018

stereobooster commented Jan 6, 2018

banga commented Jan 7, 2018 • edited Loading

gaearon commented Jan 7, 2018 • edited Loading

gaearon commented Jan 7, 2018

sophiebits commented Jan 7, 2018

banga commented Jan 7, 2018

gaearon commented Jan 8, 2018

banga commented Jan 5, 2018 •

edited

Loading

evmar commented Jan 6, 2018 •

edited

Loading

banga commented Jan 7, 2018 •

edited

Loading

gaearon commented Jan 7, 2018 •

edited

Loading