Investigate Compiling React with Google Closure Compiler Advanced Mode

[gaearon]

Just creating this to track it. We already compile the bundles with GCC simple mode. There's a bunch of things that will break in advanced mode but we're gradually moving closer to being able to do this.

I think #9955 is a prerequisite since otherwise we can never be sure we're still being correct. Landed!

Open question is if we can still keep DevTools working. I wonder if Fiber could be an array with fixed indexes and then we wouldn't need any "sourcemapping".

[evmar]

I work in this space (spent a lot of time fighting with Closure Compiler over the last few years) so if you have any questions feel free to ask.

One big piece to be aware of: there are two main ways for property renaming to work in advanced mode, controlled by the "use_types_for_optimization" flag. When it's on, it relies on correct type annotations to know what to rename, which I think is maybe a non-starter for you. (To be honest I don't know what it does when it's on but you don't have type annotations; it might fall back to the 'off' state.)

If you turn the flag off, renaming works globally, in that if a given property name is known to be one to not mangle (e.g. 'getElementById', because it's a DOM API), then Closure will never mangle any instance of getElementById anywhere in the app. Because of this, one approach that might work for you (rather than adding all the quotes as done in #9293) is to make a single externs.js that just references all known properties off of some fake object.

You can see an example of this in this file (forgive the wacky code, it's in the test suite for a code generator), where it stuffs a 'foo' property on a long namespace that will never occur in the real code. This causes all 'foo' properties to be preserved.

Of course, the best thing is to be careful to never treat properties as strings, and otherwise use quotes when indexing into string-keyed maps, which then makes it safe to rename everything. But sometimes that is not possible; depends on your code style.

[gaearon]

This makes sense. Would you be interested to send a PR to try converting the “react” package to the advanved mode with an externs file? It’s a small package so easy to experiment with. I tried flipping the advanced mode on it before but there were some compile errors and I didn’t proceed.

[banga]

Here's a possible resource for extern files for react if it helps (h/t @mihaip for finding it): https://github.com/cljsjs/packages/tree/master/react/resources/cljsjs

[gaearon]

Nice! This issue would still very much benefit from someone motivated to try it. Now that we have bundle tests, it’s a great time to play with it.

[banga]

I'd love to take a stab at it in a couple of days if that's ok?

[gaearon]

Sounds good!

[banga]

Quick update from my attempts so far:

• The approach of using externs works pretty well in preventing property renaming. The exact contents of the externs have to vary across bundles because they expose and expect different things.
• GCC still complains about a couple of things:
  • __REACT_DEVTOOLS_GLOBAL_HOOK__ is not defined in the clsjs externs file, but something was looking for it so I added it.
  • It complains about escape being redefined on

    react/packages/react/src/ReactChildren.js

    Line 29 in faa4218

    function escape(key) {

    , so I renamed it to escapeKey.
  • I had to change the env to BROWSER from CUSTOM to get the default browser externs for the UMD bundle.
• I modified fixtures/packaging/babel-standalone/dev.html to use the production builds, but I was seeing a minified error that seemed to occurring because ReactDOM was trying to access a property on ReactElement that was getting renamed. It turned out to be $$typeof, so I also added it to externs. I saw similar errors with React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED getting renamed and not being available in ReactDOM, so I added it and its assign and ReactCurrentOwner properties to externs.
• With the success with the fixture, I just now started trying yarn test-build-prod to pass, but I'm just realizing that the compiled NODE bundle files don't export anything. I think I just need to properly setup the externs and maybe use the processCommonJsModules GCC parameter to get this to work correctly.

Let me know if you notice anything troubling here. My main concern from this attempt is any hidden property name accesses between React and ReactDOM being broken by renaming because they are compiled separately, but the hope is that yarn test-build-prod will catch those.

[gaearon]

All of this sounds great. Do you think you could start a PR to get things rolling?

[banga]

Sure. I got a little stuck trying to get the node bundles working because GCC seems to be stripping out the module.exports = part. I was planning to fix that and run some of the build-prod tests to determine how much more work is needed, but if it would be useful to see my progress so far, I can start a PR.

[gaearon]

I think PR would be helpful even if incomplete.

[banga]

Alright, I'll start one tonight.

[gaearon]

@banga has done some investigation. My conclusion so far is it'll be more trouble than it's worth at this point. We may revisit at some point later.