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

Provide new jsx transform target for reactjs/rfcs#107 #15141

Merged
merged 3 commits into from
Apr 7, 2019

Conversation

rickyvetter
Copy link
Contributor

@rickyvetter rickyvetter commented Mar 18, 2019

This exposes two new top-level functions on React - jsx, and jsxs. These are targets for JSX transformers which open up many simplifications to element creation described in reactjs/rfcs#107.

The intent is to be backwards-compatible with the existing createElement call and to migrate transformers over so that most people are using this new transform. Once there are actual transforms written against this api, the more substantive changes can be tested.

@sizebot
Copy link

sizebot commented Mar 18, 2019

React: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: 43b1f74...5153dc5

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +4.8% +2.4% 101.94 KB 106.85 KB 26.42 KB 27.05 KB UMD_DEV
react.production.min.js 0.0% -0.0% 12.01 KB 12.01 KB 4.64 KB 4.64 KB UMD_PROD
react.development.js +7.6% +3.6% 64.7 KB 69.61 KB 17.35 KB 17.98 KB NODE_DEV
React-dev.js +8.0% +4.0% 62.98 KB 68 KB 16.66 KB 17.33 KB FB_WWW_DEV
React-prod.js 🔺+4.7% 🔺+3.3% 15.35 KB 16.07 KB 4.11 KB 4.25 KB FB_WWW_PROD
React-profiling.js +4.7% +3.3% 15.35 KB 16.07 KB 4.11 KB 4.25 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

@rickyvetter
Copy link
Contributor Author

I suspect that tests are failing because CI tests against the bundles and uses ReactFeatureFlags.readonly.js. I don't know how to repro this locally and didn't see any special way other feature flags get around this. Recommendations would be appreciated!

@gaearon
Copy link
Collaborator

gaearon commented Mar 19, 2019

You can add .internal.js suffix to test files that use feature flags. They won't run on bundles.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Hm. Sizebot is reporting that file size went up in the Node bundle. UMD is fine (or maybe already broken). Some compilation artifact seems to not work correctly to DCE these.

@rickyvetter
Copy link
Contributor Author

There were a couple new feature flag forks added for the new scheduler - I thought maybe missing those had caused this issue, but after adding them it seems that sizebot is still reporting an increase (although only on NODE_PROD - does the absence of a row here mean there's no increase?). Looking at scripts/rollup/forks.js it seems like the logic is correct to return false on this feature flag for all non-WWW builds. So yes, it seems like there is an issue with the DCE setup but I'm having trouble building locally (bad Java version - playing around with it) and can't inspect the bundles that CI produced so I'm not sure how to proceed.

@sebmarkbage
Copy link
Collaborator

Hm. I can't repro locally neither.

* @internal
*/
const ReactElement = function(type, key, ref, self, source, owner, props) {
const ReactElement = function(type, props, key, ref, owner, self, source) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so I downloaded the artifacts. Somehow, this is not getting inlined anymore. That's why the file increases.

I'm not sure if it's called in more places and maybe it stops trying to inline it before it's able DCE the other paths.

It could also be the argument order. You could try restoring the argument order. It might have an effect on the compiler since the property reads of mutable values are not pure operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey looks like reordering fixes it! Wild. Now there is only a size regression on WWW prod which is to be expected since the feature is turned on there.

@sebmarkbage
Copy link
Collaborator

Neat! Feel free to "Squash and merge".

@rickyvetter rickyvetter merged commit 745baf2 into facebook:master Apr 7, 2019
@rickyvetter rickyvetter deleted the jsx branch April 7, 2019 19:02
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.

None yet

5 participants