Compile the react bundle with GCC advanced mode by banga · Pull Request #11995 · facebook/react

banga · 2018-01-08T22:26:45Z

Follow-up from #11967 to only compile the react bundle with GCC advanced mode. I also cleaned up the externs here to be consistent with the current public API. I also removed the type annotations from the externs file, since we don't use GCC for type-checking anyway.

gaearon · 2018-01-08T22:36:51Z

externs/react.ext.js

+
+React.PureComponent.prototype.isPureReactComponent;
+
+React.SyntheticEvent;


I wouldn't expect this to be in public API of React package

gaearon · 2018-01-08T22:36:56Z

externs/react.ext.js

+React.SyntheticEvent.prototype._dispatchInstances;
+
+/** @suppress {duplicate} */
+var DispatchConfig;


gaearon · 2018-01-08T22:37:22Z

externs/react.ext.js

+DispatchConfig.registrationName;
+
+/** @suppress {duplicate} */
+var FiberNode = {};


Same, this shouldn't be in React package

gaearon · 2018-01-08T22:37:28Z

externs/react.ext.js

+FiberNode.prototype._debugIsCurrentlyTiming;
+
+var Deadline = {};
+Deadline.prototype.timeRemaining;


gaearon · 2018-01-08T22:37:31Z

externs/react.ext.js

+var Deadline = {};
+Deadline.prototype.timeRemaining;
+
+var Reconciler = {};


gaearon · 2018-01-08T22:38:14Z

externs/react.ext.js

+React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = {};
+React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.assign;
+React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentOwner;
+React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.EventPluginHub;


This shouldn't be here (same for all of them below). It's on ReactDOM's field, not React's.

Most of these are needed to build react-dom

Some of these were outdated from previous runs

koba04 · 2018-01-11T03:21:18Z

externs/fbjs.ext.js

@@ -0,0 +1,30 @@
+/** @suppress {duplicate} */


Why is this file required?

This is only needed for the node bundles because the contents of fbjs are not included in those bundles, so properties like emptyFunction.thatReturns get renamed when they shouldn't be. I also remember hitting a problem with canUseDOM not being set to true when it should be, but I can't repro that right now. I was being defensive when I created this file by including everything from fbjs that I saw being used, but we can cut it down to a smaller set of things if that would be preferable.

Forgot to mention — this file is not pulled in for UMD bundles, because fbjs is not part of externals for those, which are checked in getClosureExterns to determine which externs files to look for.

I got it. Thank you!

banga · 2018-01-28T02:06:34Z

Hey @gaearon, just checking if you got a chance to look at this.

banga · 2018-01-28T02:07:09Z

(Whoops, tapped the wrong button)

gaearon · 2018-01-28T12:51:33Z

Hey @banga,

This is very valuable but my focus has shifted from working on the repo infrastructure to React app compilation experiments so I won't have time to fully review/merge/support this in the near future.

I think it's safe to assume that whenever we decide to get back to this, the changes won't be very different (aside from fixing up the whitelists). I'll happily do that once I feel comfortable with spending more time on this. Please be assured this work was not in vain. This is the first time we actually had a complete PR that implements this, and it shows exactly the parts we need to fix first.

(e.g. for ReactDOM bundle, I'm not very happy with whitelists like internal variable names having to be whitelisted—this means we need to rewrite those parts in a way that doesn't rely on those names. And I don't want to change how React is bundled before we're ready to start converting the others—although I very much appreciate a demo of how it could be done.)

Does this make sense? I think we can revisit in a few months.

banga · 2018-01-28T23:38:27Z

Yeah, that makes sense. Completely agree about rewriting the parts that share internal names. Ideally the whitelist should exactly match the public react API.
Whenever you get back to this, another thing that might be worth thinking about is explicitly supporting compiling React with other codebases in advanced mode. That’s one place where most of the externs (except for some DOM and css property names) are not needed, so you get the most wins. We do this at @quip, but we end up having to fix the things that break in advanced mode, which also makes version updates painful.
Thanks!

phistuck · 2018-02-11T08:01:54Z

externs/react.ext.js

@@ -0,0 +1,76 @@
+/** @suppress {duplicate} */


While there is some value in having a clearly defined API somewhere (which can also be used as an externs file for other project, even better if it had type annotations), why are you not using the @expose annotation instead?

Deraen · 2018-04-04T16:01:26Z

packages/react/src/ReactElement.js

-const hasOwnProperty = Object.prototype.hasOwnProperty;
+const objHasOwnProperty = Object.prototype.hasOwnProperty;

-const RESERVED_PROPS = {


Related to #12368 #12370 #12530

This RESERVED_PROPS change is no longer required because Closure-compiler was fixed to not collapse properties of objects which are accessed using hasOwnProperty: google/closure-compiler@c13cf48

This fix is included in release v20180319

Based on discussions on the mentioned PR's I think the change should be reverted.

epicfaace · 2019-07-29T00:01:56Z

Any plans to revisit this any time soon?

darkleaf · 2020-04-22T16:57:29Z

+1

Compile the react bundle with GCC advanced mode

6ec63d9

gaearon reviewed Jan 8, 2018

View reviewed changes

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

Shrey Banga added 2 commits January 8, 2018 15:39

Remove unnecessary externs

c1872b2

Most of these are needed to build react-dom

Update build results.json

ff42409

Some of these were outdated from previous runs

koba04 reviewed Jan 11, 2018

View reviewed changes

banga closed this Jan 28, 2018

banga reopened this Jan 28, 2018

phistuck reviewed Feb 11, 2018

View reviewed changes

Deraen mentioned this pull request Mar 13, 2018

RESERVED_PROPS is optimized away by Google Closure Compiler #12368

Closed

gaearon mentioned this pull request Apr 3, 2018

Dynamically initialize RESERVED_PROPS #12370

Closed

Deraen reviewed Apr 4, 2018

View reviewed changes

banga closed this Feb 22, 2019


		React.PureComponent.prototype.isPureReactComponent;

		React.SyntheticEvent;

Conversation

banga commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

banga commented Jan 28, 2018

Uh oh!

banga commented Jan 28, 2018

Uh oh!

gaearon commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

banga commented Jan 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deraen Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epicfaace commented Jul 29, 2019

Uh oh!

darkleaf commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gaearon commented Jan 28, 2018 •

edited

Loading

Deraen Apr 4, 2018 •

edited

Loading

darkleaf commented Apr 22, 2020 •

edited

Loading