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

Add reactProdInvariant and corresponding babel rewrite pass #6948

Merged
merged 11 commits into from
Jun 8, 2016

Conversation

keyz
Copy link
Contributor

@keyz keyz commented Jun 2, 2016

Implements #6874. Thanks for reviewing!

var buildRequire = babel.template(`var IMPORT_NAME = require(SOURCE);`);

var REQUIRE_PROD_INVARIANT = buildRequire({
IMPORT_NAME: t.identifier(prodInvariantName),
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider doing this differently. Right now we don't have a guarantee that this identifier doesn't already exist (practically it probably won't matter but just in case…). We can use babel helpers to generate a unique identifier to use though and it'll be tracked on a per-file basis. This is how I did it for the object-assign transform: https://github.com/facebook/fbjs/blob/7c77c3c2ebcf0f1174512a66a08998dcff3d4cc5/babel-preset/plugins/object-assign.js#L15-L27

@ghost
Copy link

ghost commented Jun 3, 2016

@keyanzhang updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Jun 3, 2016

@keyanzhang updated the pull request.

@keyz keyz force-pushed the rewrite-invariant branch from 75a2c44 to 0a2dacf Compare June 3, 2016 02:06
@ghost
Copy link

ghost commented Jun 3, 2016

@keyanzhang updated the pull request.

@keyz keyz force-pushed the rewrite-invariant branch from 0a2dacf to 889416b Compare June 3, 2016 02:26
@keyz keyz mentioned this pull request Jun 3, 2016
@facebook-github-bot
Copy link

@keyanzhang updated the pull request.

@keyz keyz force-pushed the rewrite-invariant branch from 86e2624 to 84365a3 Compare June 3, 2016 06:07
@ghost
Copy link

ghost commented Jun 3, 2016

@keyanzhang updated the pull request.

@keyz keyz changed the title WIP: add reactProdInvariant Add reactProdInvariant and corresponding babel rewrite pass Jun 3, 2016
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule reactProdInvariant
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that all React* modules currently capitalized React...

Copy link
Member

Choose a reason for hiding this comment

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

The pattern is actually this:

  • export an object or class? Upper case
  • export a function? lower case

The only other one we have of the latter that starts with react is reactComponentExpect

Copy link
Contributor

Choose a reason for hiding this comment

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

ah! thanks!

@sophiebits
Copy link
Collaborator

I think this looks good other than my inlines. Can you post build size numbers?

@sophiebits sophiebits added this to the 15-next milestone Jun 7, 2016
@keyz
Copy link
Contributor Author

keyz commented Jun 7, 2016

Yep,

Running "compare_size:files" (compare_size) task
   raw     gz Sizes
  1199    641 build/react-dom-server.js
   734    445 build/react-dom-server.min.js
  1180    632 build/react-dom.js
   715    436 build/react-dom.min.js
763407 170722 build/react-with-addons.js
164511  48727 build/react-with-addons.min.js
686407 154438 build/react.js
153055  45492 build/react.min.js

   raw     gz Compared to master @ d101f68bce34a62613a3e86981266111e8950267
     =      = build/react-dom-server.js
     =      = build/react-dom-server.min.js
     =      = build/react-dom.js
     =      = build/react-dom.min.js
 +1112   -349 build/react-with-addons.js
 +2323  +1048 build/react-with-addons.min.js
 +2083   -268 build/react.js
 +2342  +1020 build/react.min.js

I wonder if there are things I can do to further reduce the size. It's much better than not rewriting invariant calls (leave them as-is) though.

* WARNING: DO NOT manually require this module.
* This is a replacement for `invariant(...)` used by the error code system
* and will _only_ be required by the corresponding babel pass.
* It always throw.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(grammar: "It always throws.")

@ghost
Copy link

ghost commented Jun 7, 2016

@keyanzhang updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@keyanzhang updated the pull request.

@keyz keyz merged commit 1abce16 into facebook:master Jun 8, 2016
@keyz keyz deleted the rewrite-invariant branch June 8, 2016 00:11
zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
zpao pushed a commit that referenced this pull request Jun 14, 2016
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
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.

5 participants