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

Babel Transform JSX to React.jsx/React.jsxDEV Plugin #16432

Merged
merged 23 commits into from
Aug 27, 2019

Conversation

lunaruan
Copy link
Contributor

@lunaruan lunaruan commented Aug 16, 2019

This babel transform is a fork of the @babel/plugin-transform-react-jsx transform and is for experimentation purposes only. We don't plan to own this code in the future, and we will upstream this to Babel at some point once we've proven out the concept.

As per the RFC to simplify element creation, we want to change the JSX transform from targeting React.createElement(type, props, children) to React.jsx(type, props, key). This modifies the existing @babel/plugin-transform-react-jsx (and helper) babel plugin to support React.jsx and React.jsxDEV.

The main differences between React.jsx/React.jsxDEV and React.createElement are:
1.) key is now passed as an explicit argument rather than through props
3.) children are now passed through props rather than as an explicit argument
4.) props must always be an object
5.) __source and and __self are now passed as separate arguments into React.jsxDEV rather than through props

Part of the rationale for this change is that we want to deprecate key spread through props because this is an expensive dynamic comparison operation. We want users instead always explicitly pass key as a prop. However, in the interim, we need a way to distinguish between <div {...props} key={foo} /> and <div key={foo} {...props} />. Therefore, until we completely deprecate key spreading, we will use React.createElement to transform <div {...props} key="Hi" /> and React.jsx to transform everything else.

@sizebot
Copy link

sizebot commented Aug 16, 2019

React: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: c1d3f7f...765ea44

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.6% +0.7% 112.65 KB 113.38 KB 28.84 KB 29.05 KB UMD_DEV
react.production.min.js 0.0% 0.0% 12.64 KB 12.64 KB 5.02 KB 5.02 KB UMD_PROD
react.profiling.min.js 0.0% 0.0% 14.82 KB 14.82 KB 5.55 KB 5.56 KB UMD_PROFILING
react.development.js +1.0% +1.2% 72 KB 72.73 KB 18.93 KB 19.15 KB NODE_DEV
react.production.min.js 0.0% 🔺+0.1% 6.66 KB 6.66 KB 2.77 KB 2.77 KB NODE_PROD
React-dev.js +1.1% +1.2% 69.97 KB 70.76 KB 18.01 KB 18.23 KB FB_WWW_DEV
React-prod.js 0.0% 0.0% 17.32 KB 17.32 KB 4.53 KB 4.53 KB FB_WWW_PROD
React-profiling.js 0.0% 0.0% 17.32 KB 17.32 KB 4.53 KB 4.53 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

Copy link
Contributor

@rickyvetter rickyvetter left a comment

Choose a reason for hiding this comment

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

This is so awesome! Thanks for working on this. Left lots of comments, but mostly about the auto-import stuff. This can definitely be incrementally added later, but is a pretty cool feature to have, so let me know what you're thinking!

I reviewed this code assuming that you copied the existing transform for createElement over and with almost no experience reading babel-transform code, so the transform itself is definitely the thing I spent the least amount of time on. Definitely could have missed opportunities for improvement there.


exports[`transform react to jsx React.fragment to set keys and source 1`] = `
"var _jsxFileName = \\"\\";
var x = React.jsxDEV(React.Fragment, {}, \\"foo\\", true, {
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that this doesn't cover is the auto-import portion of the proposal. This is (arguably) the most challenging part of the transform because it's certainly stateful, but it would be nice to have!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, planning on adding that soon! I wanted to incrementally land these changes first though. LMK what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with incrementally adding these features for sure. If that's the plan then I think this PR looks good as it stands.

});"
`;

exports[`transform react to jsx fragment with no children 1`] = `"var x = React.jsx(React.Fragment, {});"`;
Copy link
Contributor

Choose a reason for hiding this comment

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

@sebmarkbage how do you feel about auto-import of other React namespace values? It's not discussed in the RFC but I think it should be auto-imported as well.

Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 19, 2019

Choose a reason for hiding this comment

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

It wouldn't make sense if expected that to increase in scope. Fragment syntax is probably the only one that has special syntax so that makes sense.

<></> should auto-import.

It should not extend to things with identifiers though.

However, for example <Fragment /> and <Profiler /> shouldn't auto-import these names into scope.

packages/react/src/ReactElement.js Outdated Show resolved Hide resolved
packages/react/src/ReactElement.js Show resolved Hide resolved
packages/react/src/ReactElement.js Show resolved Hide resolved
packages/react/src/ReactElementValidator.js Show resolved Hide resolved
packages/react/src/ReactElementValidator.js Outdated Show resolved Hide resolved
@lunaruan
Copy link
Contributor Author

lunaruan commented Aug 22, 2019

@jridgewell and @Jessidhia Thanks for the code review! Most of this code is forked from @babel/plugin-transform-react-jsx and associated helper. This code is for experimentation purposes only. We plan to upstream these changes to Babel at some point in the future once we've proven out the concept, and making too many deviations from the original transform would make this process harder. However, you can modify the original transform with your proposed changes if you would like!

@jridgewell
Copy link

👍 . If the intention is to upstream these changes, we can wait to do a full review until then.

@lunaruan lunaruan merged commit f512537 into facebook:master Aug 27, 2019
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

8 participants