Add experimental version of the babel-plugin-transform-react-jsx transform#11154
Conversation
I think that this option has too many possible values. There are only three possible cases:
(1) and (2) can be "collapsed" together, using the The In these examples I'm using the Input code:
This avoids hard-coding the We would probably also need
I'm not sure I follow. Why doesn't this work? const props = { key: "foo" };
React.jsx("div", { ...props}, "Hi");You'll still see that the second parameter contains a |
| // as a sequence expression to preserve order. So: | ||
| // <div children={x++} foo={y}>{child}</div> becomes | ||
| // React.jsx('div', {foo: (x++, y), children: child}); | ||
| // duplicateChildren contains the extra children prop values |
There was a problem hiding this comment.
We could consider throwing in the transform if there are both syntactic children and the children property.
If we don't want to, we don't need this custom logic anyway. This output has the same effect:
React.jsx("div", {
children: x++,
foo: y,
children: child,
});There was a problem hiding this comment.
We actually did this at some point but it broke on a older browser because duplicate keys caused an error, so we had to do it this way instead. I'll add that to the comment though to clarify.
There was a problem hiding this comment.
There's a period in ECMAScript's lifetime when duplicate keys wasn't allowed in strict mode. Unfortunately those browsers are still around. Babel should probably have a transform to remove duplicate keys from any object literal.
We could error but that would make it harder to upgrade. Since we did run into this, we know people write this code today. Arguably it shouldn't error for the same reasons ECMAScript doesn't error on duplicate keys.
There was a problem hiding this comment.
Yeah, Babel already has that transform. If people need to support a browser which throws for duplicate keys, it's handled by preset-env.
Since we are going to enable object spread by default in the jsx transform, most people will have preset-env anyway.
There was a problem hiding this comment.
What is the transform accounts for duplicate keys? I guess since this is for Babel 8 it's probably okay to introduce this as a breaking change, since we're also doing things like defaulting to spread anyway. @sebmarkbage
There was a problem hiding this comment.
It produces a sub-optimal output, but:
- The only popular browser that needs it is IE (these browsers support them)
- If we want to produce a more optimized output by merging the first duplicate key into the following one, it should be done in
@babel/plugin-transform-duplicate-keysand not here - Duplicate keys are super rare anyway
| ); | ||
| } | ||
|
|
||
| if (useSpread && useBuiltIns) { |
There was a problem hiding this comment.
We are going to remove these options in Babel 8, and default to .... We can avoid adding them to this new transform.
There was a problem hiding this comment.
Sounds good! I'll delete them for now and default to ... in a separate PR?
There was a problem hiding this comment.
Would it be possible to default to ... already in this PR?
|
Thanks for the review! I forgot to explain some things that might make this PR and the implementation make a bit more sense. This Babel transform is only meant to be used for React.
Distinguishing between To satisfy the people currently using the pragma with the
Using It seems like your concern with the
What you are suggesting is our end goal; we want to ignore the "props" |
0793e3a to
ddc42e9
Compare
|
Note that part of the reason we want autoImport to be the default is that it prevents sketchy shadowing. E.g. this should not work: OR The So specifying the actual implementation details in pragmas goes directly against that goal. We want to make the coupling of JSX to a particular implementation stronger to enable compilers to operate on the assumption that it can compile JSX however it wants as long as it knows which runtime it's targeting. E.g. an optimizing Babel plugin can see that this is using importSource "react" and know how to instead use "react/jsx-optimized-helpers" with a completely different implementation, but if it sees importSource "preact" it would ignore it since it doesn't know what that the runtime is compatible with the optimization. Similarly another plugin can do the inverse. Allowing people to specifying the fine grained implementation details would undermine that ecosystem goal. |
|
|
||
| // We want to use React.createElement, even in the case of | ||
| // jsx, for <div {...props} key={key} /> to distinguish it | ||
| // from <div key={key} {...props} />. This is an intermediary |
There was a problem hiding this comment.
Can you explain why this is necessary?
When I try to reason about it, it seems like the current implementation is the opposite of what should happen. We can reliably tell that <div {...props} key={KEY} /> is supposed to be KEY. In <div key={KEY} {...props} />, we can't tell if the key is KEY or was passed in from the spread props. So to maintain the current behavior while you deprecate, you should be using createElement only in the second case.
There was a problem hiding this comment.
For <div key={key} {...props} /> we'll look at props to determine if it has a key and if so override the key passed in. So we preserve the behavior by keeping the runtime behavior + warn and then later change the runtime behavior.
For <div {...props} key={key} /> we don't know if it is <div {...props} key={undefined} /> and we can't distinguish that from <div {...props} /> at runtime because they both pass undefined to the key argument.
There was a problem hiding this comment.
It makes sense 👍
The comment should probably be updated to mention that it's needed for the undefined case.
Half joking
because they both pass undefined to the key argument.
arguments.length? 😛
|
Thanks for explaining that I still think that
I prefer 1. Depending on what the other React-like libraries decide to do (if they want to keep using |
|
I was thinking about the If you don't think that they would be needed in the real world, we could only allow the option in the config file. If someone asks for them, we can always add them later. I'm sorry if I'm playing the part of the one who just wants to remove features from the PR, but I'm trying to keep it as simple as possible by avoiding options which might not be necessary 😅 |
Hey! We talked about it a bit and we decided that we don't need the
(In Babel 8, we can swap the versions so that
We do still need |
|
I like it! The only thing that I'm not sure about is the name of the option (the implementation idea is 💯). I think that Also, I don't like "current" because whenever someone sees it they need to check when that option was introduced: the meaning of "current" changes over time. I can think of a few alternatives, but I'm not sure about them. If we can't find anything that satisfies both you and us, we can keep
The last thing: from a code point of view, (1) and (2) don't need to be handled separately. You can call |
What do you think about We can default to
The reason why we handle this separately is because would prefer to use |
👍 |
|
Sounds good 👍 |
52b492c to
d00b7c4
Compare
|
That's super awesome! Simplifying About PragmaReading through the code I think we can fully support the proposed features. The main reason we need configurable pragmas today is because of React's usage of a In our case we'll likely alias Auto importsThe only config option I'd maybe like to propose is for the import source itself. We'd like to change RefsThe changes proposed here allow us to simplify our Currently // Today
const Foo = forwardRef((props, ref) => <div ref={ref) />);
// vs
const Foo = props => <div ref={props.ref} />;iirc that was part of the original proposal @sebmarkbage made for the future direction of ConclusionAll in all we're very excited about these changes. They allow us to simplify our code even further and save more precious bytes. We're fully on board with them and we really appreciate reaching out to us. Great work all around! 🙌 💯 I'll probably have a go at implementing |
|
@lunaruan You can disable the failing test on windows (like it was done at |
|
@marvinhagemeister WDYT about the If it's not needed by Preact, I can think of three possibilities:
_jsx("div", { foo: 1, ..._warnIfKey(props) }, key);
// warnIfKey helper implementaition
function _warnIfKey(props) {
if ("key" in props) console.warn(...);
} |
|
@nicolo-ribaudo Personally I'm fine with moving users off of spreading keys, but I'm admittedly a bit scared of the amount of support questions we may get. I think for us and similar frameworks the most preferable solution would be opt-in. Knowing that, we're very likely at the mercy of the React ecosystem either way, because most warnings/mismatches come from older third party components. Some of those may break unexpectedly (for both React and Preact) if they rely on Looping in @DanielRosenwasser from the TypeScript team because many Preact users compile jsx via TS directly. |
cf59c61 to
d895cb7
Compare
As part of React's RFC to simplify element creation, we want to update the
babel-plugin-transform-react-jsxtransform so that it usesReact.jsxinstead ofReact.createElementand adds to ability to auto import React for JSX.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 (added through separate Babel plugins) are now passed as separate arguments into React.jsxDEV rather than through props
6.) In production, we will use
jsxs(type, props, maybeKey)if there MORE THAN ONE child in the AST andjsx(type, props, maybeKey)if there is ONE OR FEWER children in the AST.7.) We want to deprecate key spread through props, so we will use
React.createElementto transform the case where props spread comes before a key (ex.<div {...props} key="Hi" />) so that we differentiate between prop spread after a key (ex.<div key={foo} {...props} />, which will override the key) and warn appropriately.8.) In development mode we will use
jsxDEV(type, props, key, isStaticChildren, source, self), where isStaticChildren is true when you should usejsxsand false if you should usejsx. This will be specified using amode: developmentoption. (One concern here is that someone might accidentally forget to set this. We can make this option required so that this doesn't happen)This PR also adds two options for auto import to React:
1.)
importSource: The React module to import from. Defaults to react.2.)
runtime: 'classic' | 'automatic'. In Babel 7.9 this will default toclassicand in Babel 8 it will default toautomatic. In classic mode, we will usecreateElementwithout auto importing (ie. the current behavior). In automatic mode, we will usejsx/jsxsand auto import. If the sourceType is module, we will use named exports (import {jsx as _jsx} from "react"; JSX compiles to_jsxetc.). If the sourceType is script, we will use require (var _react = _interopRequireWildcard(require("react"));. jSX compiles to_react.jsxetc.)Automatic runtime adds two pragmas (
jsxAutoImportandjsxImportSource) that allow users to specifyautoImportandimportSourcein the docblock. Classic runtime will still havepragmaandpragmaFrag.In addition, for Babel 8, development mode will require a separate Babel plugin,
@babel/plugin-transform-react-jsx-development. This mode will automatically add__sourceand__selffor both runtimes so that users will not need to use separate plugins. Adding thesourceandselfplugins will be a no-op.