Current code transform rule is not friendly for custom jsx-runtime
or react-like lib.
// have to re export the `createElement`
import { createElement } from "@emotion/core"
import { jsx, jsxs, Fragment } from "@emotion/core/jsx-runtime"
Could we follow rules like below, could got same behavior.
<span key={key} {...obj} />
=> jsx("span", obj, key)
<span {...obj} key={key} />
=> jsx("span", {...obj, key}, key)
https://github.com/babel/babel/issues/12177
https://github.com/microsoft/TypeScript/pull/39199
I struggle to understand the issue â could you please write a more detailed description? I understand that the transform (intentionally) falls back to createElement but I donât have a good sense of why this creates a problem for Emotion.
When using babel-transform-react-jsx with runtime: automatic
,
and set @jsxImportSource
to switch custom jsx-runtime
But if the fallback exists. codes transformed like below.
// @jsxImportSource @emotion/core
<span {...obj} key={key} />
// convert to
import { createElement } from "@emotion/core"
createElement("span", { ...obj, key: key })
and the createElement
need to export by @emotion/core
too ďźis this necessaryďźďź.
For support key
after object spread (if not do this, transformed codes will got createElement not found).
now @emotion/core
have as createElement
-like api named jsx
,
but @jsx jsx
and @jsxImportSource
could be set at same time.
but @jsx jsx
and @jsxImportSource
_could not_ be set at same time.
I think add a custom jsx-runtime
should only foucs @emotion/core/jsx-runtime
with exports jsx
,jsxs
, Fragment
@gaearon What do you think?
but @jsx jsx and @jsxImportSource could be set at same time.
can they? arent they mutually exclusive?
I think add a custom jsx-runtime should only foucs @emotion/core/jsx-runtime with exports jsx,jsxs, Fragment
That would be IMHO preferable as well, but I guess there are good reasons why this is required right now and we'd just have to add export { jsx as createElement } from './jsx'
to those new entry points, so it ain't bad. Although probably not having to worry about createElement
would allow us to cut a few bundlesize bytes.
Was this fallback in the original Babel's PR? I must have missed it there as I was following that PR rather closely. Couldn't spread operator or an Object.assign call be inserted instead of falling back to createElement?
EDIT:// Oh, I have just now realized that createElement
is supposed to be exported from the jsxImportSource
itself and not from those new jsx-related entrypoints. I find this quite weird as well and if it's required to provide a separate @jsx
pragma for this case like in this TS playground (it uses React.createElement
even though @jsxImportSource
is provided) then this is quite a big usability issue from Emotion's PoV.
I'm struggling to parse the discussion on this issue so I would really appreciate if you could rephrase this imagining that I know very little about the JSX transform. All of the comments are written in a very terse way so far.
Here's what I understand so far:
<div key="x" {...spread} />
intentionally falls back to createElement
because we can't know which key
to usecreateElement
â but doesn't want to after the JSX changes?The thing I struggle with is it's not obvious to me what exactly is the "usability issue" here. It's like something implied but I don't understand what it is.
Maybe it would help to reframe in the sense of: (1) here's the source code, (2) here's the Babel config we tell Emotion users to apply, (3) here's how they have to write code because of this issue, (4) here's how we'd like them to write code instead. Or (5) here's what we have to expose to work around this problem, (6) here's what we would prefer to expose instead. Or (7) here's what configuration they have to specify to work around this, (8) here's the configuration we want them to use instead.
Sorry for being overly terse and losing the required context. I'll try to amend it now as well as I can.
As you may know, Emotion has chosen to support the so-called css
prop, and to make it work in a variety of use cases we have chosen to support it using a custom JSX pragma. We got a fair share of critique from the community for that as people like to bikeshed over this sort of thing and for some people having to use a magic comment (pragma) was too odd. We stand by the technical choices we've made and pros that it gives us - supporting 0config tools like CRA is important for us. However, after that critique from the community, we've caved a little bit and we've provided a way to insert that pragma on behalf of our consumers automatically in a form of a Babel plugin.
So, currently, there are 2 ways of how one can use css
prop API:
/** @jsx jsx */
+ import { jsx } from '@emotion/core'
)@emotion/babel-preset-css-prop
The jsx
function that we provide is just a custom version of the createElement
that either calls React.createElement
if no css
prop is given to it or it resolves it and then calls React.createElement
using the rest of the props (the second part is a little bit simplified, but that's the gist of what it's doing).
If we consider only the second way of adding support for the css
prop then nothing really changes - it's already "automatic" from our side, so it's just a matter of adding support for those new entry points etc and it should just work. Note: this has never worked with just the TS compiler which is a real bummer as it doesn't allow people to set jsx pragma globally. I've hoped that this would change with the introduction of the new runtime but I got no comment back from the TS team when I've asked about this (more than once I believe).
The problem is with the first way - writing pragma manually. I was sure that using /** @jsxImportSource @emotion/react */
would be enough to make it work. I can already hear people complaining about this new pragma (way longer than the previous one) but it is what it is - we need to support 0-config tools and TS compiler.
Let's take a look at Babel:
input
// babelrc.js
module.exports = {
presets: ["@babel/preset-react"]
}
/** @jsxImportSource @emotion/react */
<div {...props} />;
<div {...props} key="key"/>;
output
function _extends() { _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; }; return _extends.apply(this, arguments); }
/** @jsxImportSource @emotion/react */
/*#__PURE__*/
React.createElement("div", props);
/*#__PURE__*/
React.createElement("div", _extends({}, props, {
key: "key"
}));
Result? It doesn't work - using @jsxImportSource
is not enough, one has to provide a { runtime: "automatic" }
to the React's preset to make it work. It's one more thing that has to be explained, documented etc. Arguably it's not that big of a deal as 0-config tools will probably already have this option provided automatically. Some people might get lost though when configuring stuff on their own and wanting to use the pragma approach, or just because they have skipped the section on how to add Emotion's Babel plugin, or for any other reason. It feels like a small detail but I don't quite see why a sole fact of using @jsxImportSource
doesn't switch the transformer to the automatic one.
Even if it works - it still requires createElement
from the root of the importSource
. I understand why React has to provide this (compat reasons) but the same scale and constraints do not apply to the rest ecosystem, parts of it (like Emotion) could "move" faster away from using createElement
and thus saving on the implementation complexity and bundlesize of the library. Supporting this doesn't add that much to both of those - but if we could skip that entirely, why wouldn't we like to do it? It also imposes the same on other alternatives runtimes/libraries wanting to integrate with the new transform - it makes the integration more difficult, it's just one more thing to be aware of. If one sees the automatic runtime and jsx
helpers the intuition (in most cases) will tell them that it's enough to make things work, but there is this special case with createElement
that I feel is quite easy to miss.
Let's take a look at TS now (note - I'm using TS online playground with nightly build that is available there):
input
/** @jsxImportSource @emotion/react */
<div {...props} />;
<div {...props} key="key"/>;
output
"use strict";
/** @jsxImportSource @emotion/react */
_jsx("div", Object.assign({}, props), void 0);
React.createElement("div", Object.assign({}, props, { key: "key" }));
This has not worked as expected at all. The createElement
exception has been inserted as React.createElement
. To make it work as expected one would have to use both pragmas and an explicit import for the @jsx
right now:
/** @jsxImportSource @emotion/react */
/** @jsx jsx */
import { jsx } from '@emotion/react'
I can only assume now that this a bug in TS nightlies but it is IMHO an indication of how easy is to miss this case and implement it incorrectly. If this is about to stay (please no) then this is a huge DX downgrade for the TS users.
The jsx function that we provide is just a custom version of the createElement that either calls React.createElement if no css prop is given to it or it resolves it and then calls React.createElement using the rest of the props (the second part is a little bit simplified, but that's the gist of what it's doing).
Hmm, are you saying that Emotion provides jsx
but it has no relation to React's concept of jsx
because Emotion wanted a shorter synonym for createElement
?
Result? It doesn't work - using @jsxImportSource is not enough, one has to provide a { runtime: "automatic" } to the React's preset to make it work.
I think it would work in Babel 8 since that the new transform will become the default one?
It feels like a small detail but I don't quite see why a sole fact of using @jsxImportSource doesn't switch the transformer to the automatic one.
Yeah, I'm not sure. Maybe because they're currently implemented as two completely separate transforms. Maybe @lunaruan or @sebmarkbage knows.
Supporting this doesn't add that much to both of those - but if we could skip that entirely, why wouldn't we like to do it?
I think the createElement
case is part of the reason, as we need to keep supporting <div {...stuff} key="x" />
but the new JSX signature needs key
as a separate argument. How do you propose to handle that instead? It's important to preserve the current spread semantics here, otherwise it would be a huge breaking change.
In the longer run we want to make this pattern a compile error. But that would require some transitional period. If you have a custom Babel plugin you could make that a compile error today, but this doesn't help the zero-config case.
This has not worked as expected at all. The createElement exception has been inserted as React.createElement
You didn't show a successful example, but I think you're saying that Babel would instead produce code with two auto-imports rather than one. Is this correct? Does Babel have the "severe DX downgrade" aside from a longer pragma?
I can only assume now that this a bug in TS nightlies but it is IMHO an indication of how easy is to miss this case and implement it incorrectly. If this is about to stay (please no)
I don't understand the reasoning here. It does sound like a bug in TS, because it doesn't match the behavior in Babel and in our tests. If it's a bug in TS, it's only "about to stay" if you don't report it to TS and they don't fix it. So it seems like the next action item is to report it.
I don't see why "easy to miss" is an important consideration here â it's only "easy to miss" once per JSX implementation. Today, we have two mainstream ones (Babel and TS). I think it's expected that transform implementations are nuanced. ES spec nuances are also easy to miss, but this doesn't preclude Babel from getting them right.
Assuming the TS gets reported and fixed, is the "DX downgrade" still severe?
@gaearon
My concern is why we need do the fallback.
if we just want to pass key prop, i think
<span {...obj} key={key} />
=> jsx("span", {...obj, key}, key)
could got same result with jsx-runtime
Is there other reasons to fallack createElement (only for key after props spread, key before props spread already using jsx-runtime)?
<div key="x" {...spread} />
intentionally falls back tocreateElement
because we can't know whichkey
to use
<div key="x" {...spread} />
not fallback.
babel-config: { plugins: ["@babel/plugin-transform-react-jsx", { "runtime": "automatic" }]}
<span {...props} key={"key-after"} />
<span key={"key-before"} {...props} />
// convert to
import { jsx as _jsx } from "react/jsx-runtime";
import { createElement as _createElement } from "react";
_createElement("span", { ...props,
key: "key-after"
})
_jsx("span", { ...props }, "key-before")
@Andarist
There is a point i confused - "only key after props spread do the fallback"
/** @jsxImportSource @emotion/react */
<div key="key "{...props} />;
<div {...props} key="key"/>;
// will convert to
import { createElement } from "@emotion/react"
import { jsx } from "@emotion/react/jsx-runtime"
jsx("div", props, "key")
createElement("div", { ...props, key: "key" }) // only key after props spread do the fallback
i don't know why not convert to jsx("div", props, "key")
for both cases.
it could be jsx("div", { ...props, key: "key" }, "key")
if want to pass key into component as https://github.com/babel/babel/issues/12177#issuecomment-708888465 mentioned
I think the
createElement
case is part of the reason, as we need to keep supporting<div {...stuff} key="x" />
but the new JSX signature needskey
as a separate argument. How do you propose to handle that instead? It's important to preserve the current spread semantics here, otherwise it would be a huge breaking change.
@gaearon
i still think <div {...stuff} key="x" />
transforming tojsx("div", stuff, "x")
will not break anything (props.key
is always undefined
in Component ).
However. current tools transforming <div {...stuffWithKeyProp} />
tojsx("div", { ...stuffWithKeyProp })
,
Which will cause the issue you mentioned. When user upgrade to jsx-runtime
, key missing error will be throw.
By infomation from https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-spreading-key-from-objects
i think the transforming rules could be unified:
<div {...stuffWithKeyProp} />
=> jsx("div", stuffWithKeyProp)
key
missing<div key="key-before" {...stuff} />
=> jsx("div", stuff, "key-before")
<div {...stuff} key="key-after" />
=> jsx("div", stuff, "key-after")
the key is the special prop as fact rule for a long time.
still let it continue special until it renamed to @key
even support @key
too.
if @key
not exists, but key
exists, show the deprecating warning and using key
as fallback of @key
, and do the merging.
<div @key="key-before" {...stuff} />
=> jsx("div", { ...stuff }, "key-before")
<div key="key-before" {...stuff} />
=> jsx("div", { key: "key-before", ...stuff }, "key-before")
<div {...stuff} @key="key-after" />
=> jsx("div", { ..stuff }, "key-after")
<div {...stuff} key="key-after" />
=> jsx("div", { ...stuff, key: "key-after" }, "key-after")
Considering that the last key prop (or any prop) should win, itâs very confusing that https://github.com/facebook/react/issues/20031#issuecomment-709677883 seems to be doing the exact opposite of what would be needed.
@ljharb you mean confusing for the current transforming results or my suggestionďź
The results. What I would expect is that <div {...stuff} key="key-after" />
would be the one that can be handled with the new transform, and that <div key="key-before" {...stuff} />
could not be, because stuff.key
would overwrite key-before
in the latter example.
Hmm, are you saying that Emotion provides jsx but it has no relation to React's concept of jsx because Emotion wanted a shorter synonym for createElement?
Not sure if I understand the question correctly but Emotion's jsx
is just a wrapper around React.createElement
. The name (jsx
) has probably been choosen as it's shorter and to make it different from the React's one - which was a gain in the times when people had to write those pragmas mostly manually.
I think it would work in Babel 8 since that the new transform will become the default one?
Yes - that would probably work out of the box in Babel 8 but there will be Babel 7 consumers for a considerable amount of time in the future so I still believe it's a problematic from the DX point-of-view (as mentioned - it's not a super big deal, but it could not exist at all, just like in the TS compiler).
Yeah, I'm not sure. Maybe because they're currently implemented as two completely separate transforms. Maybe @lunaruan or @sebmarkbage knows.
Their comment about this would be highly appreciated - those transforms are mostly internal in Babel right now, so this could always be refactored rather easily to merge them into 1 or something. I believe it would make sense as it would cover more cases and in the end of the day it would be more intuitive.
I think the createElement case is part of the reason, as we need to keep supporting
but the new JSX signature needs key as a separate argument. How do you propose to handle that instead? It's important to preserve the current spread semantics here, otherwise it would be a huge breaking change.
I'm slightly confused by this - in the similar way @ljharb is. Why this is a problem if we know what key is going to "win" and not the opposite situation where key is defined before the spread? Also - why <div {...props} />
doesn't have to call createElement
nor it provides an extra argument to React.jsx
for the key
? After all, props
could include a key
prop and you want to preserve the current semantics without breaking people right now, right? I bet answers to those could be found in the original RFC or in the Babel's PR but they are quite lenghty. I know it's not super fair - but I would appreciate pointers to the required background behind this or a short explanation here. I'm commenting on this right now between taking care of 2 kids & packing bags for a trip. I totally understand that you won't be able to provide those due to your own time constraints though so if you don't do that I will try to dig those things up in the following days.
You didn't show a successful example, but I think you're saying that Babel would instead produce code with two auto-imports rather than one. Is this correct? Does Babel have the "severe DX downgrade" aside from a longer pragma?
This particular input/output pair was from the TS compiler. The first one was from Babel and it has worked OK (after adding { runtime: "automatic" }
option which is a little annoyance from my PoV as mentioned above) - it has inserted 2 auto-imports and used configured importSource
(through @jsxImportSource
pragma) for the createElement
import. This is OK from consumers' PoV - they don't really care about what is inserted as it happens automatically. From the library maintainer's PoV it is weird to be forced to provide an extra export from the root entry (usually) of the package if this whole thing has introduced dedicated entry points for the JSX-related factories. But to sum this up - I haven't recognized major DX downgrades in Babel alone.
I don't understand the reasoning here. It does sound like a bug in TS, because it doesn't match the behavior in Babel and in our tests. If it's a bug in TS, it's only "about to stay" if you don't report it to TS and they don't fix it. So it seems like the next action item is to report it.
Yes, I was just wanting to establish in the latest comment that I should report a bug to the TS team about this. Would be cool to settle this discussion as a whole before doing so, so I could report everything at once if we find out that there is anything else worth reporting.
I don't see why "easy to miss" is an important consideration here â it's only "easy to miss" once per JSX implementation. Today, we have two mainstream ones (Babel and TS). I think it's expected that transform implementations are nuanced. ES spec nuances are also easy to miss, but this doesn't preclude Babel from getting them right.
Sure, there are always be nuances but I feel that if we could avoid them then the overall result would be better and it feels to me that this is just a case like this. This nuance could not exist - I'm purely referring here to the fact that createElement
is not part of the dedicated entry points in the new transform.
Assuming the TS gets reported and fixed, is the "DX downgrade" still severe?
No, they are not super severe. I've jumped into this issue first after seeing TS output - assuming they got it "right", especially that I've just learned about createElement
being used at all in the new JSX transform. I couldn't test this in Babel at the time because this can't be configured in the playground - again, kinda because @jsxImportSource
is not enough to trigger the new transform to kick in. Later I've setup local test and the situation is much better than I have thought at first but there are still those minor annoyances that I've mentioned in this comment and the earlier one. I totally understand that you won't classify them as severe (I don't either) or even recognize them as annoyances at all (although I believe that they are).
Sorry for confusing replies from my side. Iâm just picking up this work so I donât have all the context and am also confused by some things. I personally wish this stuff was brought up a little bit earlier than a stable release including the transforms. We published the first RC in August (although to be fair we didnât emphasise the runtime) and the JSX blog post almost a month ago. But I appreciate that itâs being raised. I just need a bit more time and context to digest this thread and respond to the comments.
Ok so I've done some digging and I think I have a slightly better idea of what's going on. Scratch my previous answers.
Like the RFC says, we want to get here:
function jsx(type, props, key) {
return {
$$typeof: ReactElementSymbol,
type,
key,
props,
};
}
The rest is tactics.
We have a few cases we can't break right away. That's our first constraint.
let obj = { key: "bar" }
// 1. Key Before Spread
<div key="foo" {...obj} />.key // "bar"
// 2. Key After Spread
<div {...obj} key="foo" />.key // "foo"
In the longer term, we either want to make Key Before Spread a compile error (due to ambiguity) or, more likely, just make both of them use "foo"
as a key. Potentially changing the syntax to @key=
in distant future to highlight it's not in the same "namespace" and thus doesn't get affected by spreading.
Since key before spread which has a key
in it is a problematic pattern that will change the behavior, we'll need to start warning about it at some point. Now here's the fun bit. The traditional createElement()
transform can't produce code warning about it.
// Classic transform
<div key="foo" {...obj} />.key // "bar"
// ->
React.createElement(
"div",
_extends({ key: "foo" }, obj)
).key; // "bar"
In this case, createElement
just sees the already merged { key: "bar" }
as its second argument, so it can't know whether we have, in fact, had a spread with an overwritten key there. We can't rely on createElement
to show that deprecation warning because it doesn't get enough information to detect the bad pattern.
It would have to be done by a different compile target. This is why rolling out the new JSX transform widely is so important. It enables the new runtime warning.
jsx
prefer config.key
or maybeKey
?jsx(type, config, maybeKey)
In the end state, we just use maybeKey
. This is clear enough. But what do we do in the meantime? We have two options:
config.key
if it's not undefined
, otherwise we use maybeKey
.maybeKey
if it's not undefined
, otherwise we use config.key
.There's a few things we can note about these options:
<div {...obj} key={undefined} />
(override key
with undefined
) and <div {...obj} />
. This is because maybeKey
would be undefined
in both cases. So how do we distinguish? We could hack something like look at arguments.length
but that's not ideal for a fast implementation.<div key="foo" {...obj} />
and make it fall back to createElement
. Because it would have gotten an incorrect key ("foo"
). But the problem is that, as we said in the section above, createElement
cannot add a warning for this case because the merging would happen before the call. From createElement
perspective, it would just be a regular call with { key: "bar" }
. So then we wouldn't be able to deprecate this pattern, which is the whole point.For these reasons, we went with Config Wins until breaking changes. Which means that we use config.key
, and only fall back to maybeKey
if that doesn't exist. This lets us use jsx()
for <div key="foo" {...obj} />
(config wins!), which can later add a deprecation warning specifically for this pattern (because it knows config.key
and maybeKey
clash).
However, if Config Wins, we can't use jsx()
for <div {...obj} key="foo" />
today. This is because it would incorrectly get the "foo"
key. This is why, counter-intuitively, Key After Spread doesn't use jsx()
today and falls back to createElement
.
Let's say:
let obj = { key: "bar" }
Here's what we have.
<div {...obj} key="foo" />
: The key is "foo"
. Always uses createElement
.<div key="foo" {...obj} />
: The key is "bar"
. Uses jsx()
with the new transform.<div {...obj} key="foo" />
: The key is "foo"
. Still uses createElement
.<div key="foo" {...obj} />
: The key is "bar"
. Uses jsx()
with the new transform, which warns.<div {...obj} key="foo" />
: The key is "foo"
. Uses jsx()
with the new transform<div key="foo" {...obj} />
: The key is "foo"
. Uses jsx()
with the new transform.@gaearon thanks, that's a very thorough analysis. Should eslint-plugin-react have a rule to help prevent combined usage of key and spread? Should it warn on both combinations or just one of them? What would be most helpful here?
I think we'll want to warn for spread after key
because that's the one that would eventually change the behavior.
The tricky case is when you have <div {...obj} />
and you don't know whether there's a key in there. Because static analysis can't catch that. That's the most important one we'll need to warn about at runtime.
The tricky case is when you have
and you don't know whether there's a key in there. Because static analysis can't catch that. That's the most important one we'll need to warn about at runtime.
react/jsx-key may help to warn in where key
required.
runtime warning may be confused. if we could pass key
into Component with jsx-runtime
.
@gaearon Thanks for your explanation. the final stage is same as i expect.
I have switch to using jsx-runtime
successfully when 16.4 landed.
Just confused by the createElement
fallback.
We could use other tool to avoid key after spread.
After all tool chain ready.
hope the createElement
fallback may be documented like https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html,
it is important for __react-like__ or __jsx__ wrapper packages.
At least, maintainers need to be notified the fallback rule for pushing forward to jsx-runtime
smoothly
For developer, may need npx react-codemod re-sort-react-key
.
I personally wish this stuff was brought up a little bit earlier than a stable release including the transforms. We published the first RC in August (although to be fair we didnât emphasise the runtime) and the JSX blog post almost a month ago. But I appreciate that itâs being raised.
Sure thing - that would be ideal. It's not that I haven't been paying attention to this at all, it's just that I probably have assumed some stuff and had to handle other stuff, my OSS backlog is only growing đ I was actually looking into both the original RFC (which doesn't mention createElement
fallback at all - probably because it wasn't yet decided how to handle this key problem at this point in time) and I've also participated in the discussion below the original Babel PR. I've raised some concerns there but I've totally missed the createElement
fallback stuff.
My concerns about pragma stuff were heard by Nicolo and I've responded back that what he has proposed was addressing my concerns but it seems that at the end of the day it was not implemented as described in Nicolo's comment or we've misunderstood each other. I'm talking about this comment that mentions pragmas as only requirement for chosing classic/automatic runtime (when pragmas are actually used, ofc). From that comment I did not conclude that for the new pragma to actually work an additional option passed to the preset itself would still be required (as I believe that was not Nicolo's intention there). I'm not trying to blame anyone - don't get me wrong. There were a lot of comments below this PR, I was just an observer/commenter and it was easy to miss stuff and not address every little detail.
Thank you for the thorough explanation of the createElement
fallback stuff! This is very helpful in understanding why this works how it does.
I understand now why this is needed for React. I know that you care deeply about compatibility, you have a huge ecosystem to support etc. The recent introduction of the new factories to the 3 last major versions of React is just a sign of this and this is great.
That being said - the JSX, even though it has originated in React, became more of its own thing that is decoupled from React. It seems that the whole point of new transforms is to allow slightly different semantics and optimizations opportunities - those are interesting from the PoV of all parties using JSX, but not all of them are having the same constraints as React in terms of the ecosystem and compatibility. IMHO this should act as a fresh start by default and only provide means for the React (and parties interested in the same) to allow for the smoother migration process. Some parties could just not care about this migration and jump right into the final state of things - instead they are forced to go along the React's migration process. For example in the case of Emotion this might need more syncing points in the future with React's release schedule than we'd like. Maybe we'd like to follow your path closely - I don't know. The point is that this choice has been taken away from us to some degree and I think that we should have this choice. Even more so runtimes not tied to React at all should have that choice. Just my 2 cents about this.
Assuming the TS gets reported and fixed, is the "DX downgrade" still severe?
Circling back a little bit to this - after internalizing more facts about this. If we just talk about the thing in abstract - no, the DX downgrade is not severe (apart from minor annoyances that we've mentioned here). However, if we take into consideration how this works in Babel now then - yes, we could call this quite severe.
Why? Solely because of the fact mentioned in this very comment of mine: pragmas alone are not reconfiguring the transform accordingly. 0-config tools have to use some kind of detection to configure the "runtime"
option, for example CRA is (right now) configuring this here. This has a following effects:
Emotion+CRA users won't be able to upgrade CRA seamlessly because they are using @jsx
pragma in their code and this, in combination with "runtime": "automatic"
, throws an error:
SyntaxError: /test__/index.js: pragma and pragmaFrag cannot be set when runtime is automatic.
> 1 | /** @jsx jsx */
| ^
2 |
3 | <div {...props} />;
Which is also somewhat weird because the opposite situation (@jsxImportSource
with "runtime": "classic"
) does not throw an error. It just doesn't do anything.
I believe this is somewhat a deal-breaker kind of severity if the overall intention is to provide the most seamless experience possible. However, it can be fixed rather easily - by allowing pragmas to reconfigure the used runtime (which would match the current default behavior of the TS compiler).
However, it can be fixed rather easily - by allowing pragmas to reconfigure the used runtime (which would match the current default behavior of the TS compiler).
Could you file a Babel issue for this proposal?
@gaearon sure thing, gonna ping you and @lunaruan once I do this. Might also offer my help implementing this.
Emotion+CRA users won't be able to upgrade CRA seamlessly because they are using @jsx pragma in their code and this, in combination with "runtime": "automatic", throws an error
Just to be clear, they would be able to solve that by changing the pragma they use (to the new one), would they not?
And I think you're also saying the /jsx-runtime
detection mechanism in tools like CRA hurts you because emotion
users also use React, and so react/jsx-runtime
gets detected, which opts them into the runtime that conflicts with their pragma?
Just to be clear, they would be able to solve that by changing the pragma they use (to the new one), would they not?
Yes, but it requires us to release an Emotion version with new factories (which we have to do anyway ofc) and users would have to upgrade CRA and Emotion at the same time (as soon as one upgrades CRA they have to upgrade pragma at the same time), which aint ideal (its not 100% seamless).
And I think you're also saying the /jsx-runtime detection mechanism in tools like CRA hurts you because emotion users also use React, and so react/jsx-runtime gets detected, which opts them into the runtime that conflicts with their pragma?
Yes, but only because pragma alone doesnt reconfigure the transform
Moving the discussion from here:
I donât think anyone is arguing that this isnât confusing for the implementors. :-) In particular because it wasnât clearly specified in the RFC. Thatâs our fault. But I think talking about whether this is confusing or not for the implementors doesnât move the discussion forward. Iâm not sure I understand the goal of continuously bringing this up.
Sorry for maybe bringing this up too often. It wasn't clear to me if you agree with my initial feedback about this as I don't recall you addressing this point explicitly, also can't find any comment responding to this particular issue right now but I, of course, could have missed it in the whole discussion. I just thought that maybe it's worth mentioning again after seeing that it wasn't solely my confusion.
Re: confusion â if this is the criticism of the rollout and communication, I fully agree we could do a better job about this case.
I'm not really sure if you could do much more here. Those changes have been rolling out for quite a long time and they are only really of interest to a rather small group of people. I certainly wouldn't blame you, your team members, or anyone else for this. Such things just happen sometimes. I was giving this feedback more to just point out those nuances so maybe next time you, or other readers, would have a better chance to spot them before the release - also in very different situations. I believe that learning about some problems of the past enhances our ability to spot problems in the future, and not necessarily to spot problems in similar situations, but rather just in general. I personally like to read about different angles that people have for various stuff.
Iâm not sure if youâre just communicating frustration about this or if you want to change this to a different solution. Regardless this should probably be discussed on the React repository instead.
As mentioned above - kinda neither of those đ
I would love to see this thing in a slightly different shape that would be easier for implementers etc but also the ship has already sailed and I don't think you can really fix this particular issue as the "fix" would require reexporting createElement
from those new JSX entries and adjusting the Babel transform but you probably don't want to break early adopters and mismatch what's exported from those between React versions. Unless you are open to that.
Please always assume my best intentions. I'm here just because I care about the community, projects I maintain, and projects I'm integrating with. I fully realize stuff is complex and also that things just slip sometimes. I also believe that it's important to discuss things like this even if we come to the conclusion that I'm not right or that you agree with the feedback but there is not much that can be done at a particular point in time. My tone also might be a little bit rough at times as I'm not a native speaker and it's not super easy to express myself in a way that the reader can feel my best intentions from the wording etc.
OSS â¤ď¸
No problem at all, I appreciate the feedback! I maybe have been unclear myself earlier â what Iâm saying is I wasnât aware of this nuance either, and even people working on it got confused by it a few times while explaining it to me. Itâs inherently confusing because of the migration path constraints. I agree we shouldâve stressed this case in more detail to the TypeScript team. The only thing I disagreed with in your statement was the implication that because something is not obvious or confusing, it demonstrates a flaw in the approach. I think itâs important to separate lapses in communication (which is what happened here) from lapses in the long term plan and the upgrade path itself. Criticism of both is welcome but we need to separate them because otherwise itâs hard to understand which of the sentiment people are expressing. I really appreciate the conversation!
as the "fix" would require reexporting createElement from those new JSX entries and adjusting the Babel transform but you probably don't want to break early adopters and mismatch what's exported from those between React versions. Unless you are open to that.
FWIW we donât want to re-export because we want to keep the new runtime minimal. Not because of the transform that has shipped per se. It doesnât quite make sense to us to re-export createElement from React when the point is to start moving away from it as a compile target.
I donât think I quite agree with your argument that ânewerâ environments like Emotion can jump faster to modern runtime and abandon the classic runtime altogether. The reason I donât agree with it is because when a person adopts Emotion, they donât intend to adopt different spread semantics. They wouldnât even be aware of such a difference. It would be confusing if I converted a project to Emotion, and suddenly my keys stopped working, leading to (e.g. in case of a messenger app) input state being shared between different peopleâs conversations. I think itâs important that aside from the css prop, JSX semantics work the same way in Emotion and React, which means that Emotion jsx() runtime needs to proxy to Reactâs jsx(), and Emotion createElement() runtime needs to proxy to Reactâs createElement().
The problem with the bruteforce approach of Emotion just having a modern JSX entry point is that it wonât be synced with Reactâs gradual migration path, deprecation warnings, etc. This will likely become a problem in the following years if they donât match 1:1. I think it would be preferable that Emotion doesnât implement the JSX runtime at all (and tells the users to keep using the classic transform) than if it implements it with different semantics and causes difficulties with the rest of the migration path.
The only thing I disagreed with in your statement was the implication that because something is not obvious or confusing, it demonstrates a flaw in the approach. I think itâs important to separate lapses in communication (which is what happened here) from lapses in the long term plan and the upgrade path itself.
I'm not saying that the overall approach was fundamentally flawed, just rather than a different approach would not cause such confusion and that there would not be a need to communicate this.
FWIW we donât want to re-export because we want to keep the new runtime minimal. Not because of the transform that has shipped per se. It doesnât quite make sense to us to re-export createElement from React when the point is to start moving away from it as a compile target.
I don't quite see how really the current situation is different from the proposed one. You will have a point in the future when you are going to deprecate/remove the createElement
anyway and I don't see how the location of that export changes the overall scheme of things. Either way - this is already bikeshedding-stuff now, so let's drop this.
I donât think I quite agree with your argument that ânewerâ environments like Emotion can jump faster to modern runtime and abandon the classic runtime altogether. The reason I donât agree with it is because when a person adopts Emotion, they donât intend to adopt different spread semantics.
Yes, I agree that we would probably choose to match your release timeline - having a choice would be appreciated though. In the same way - it would be really cool for end consumers to opt-in to the final state of those semantics. I personally would prefer to just migrate now in a one go and stick to the new semantics from now on rather than migrate a little bit now and later have to circle back to this again. I totally understand this might cause a lot of churn in the community and a migration path forward is really important but it's not as important to all people.
Dropping the argument for Emotion potentially using new semantics - there is still a high-level problem of non-React runtimes being bound to your migration path and I believe that this problem is "real". It's, of course, not super terrible, but it exists - with the change of the transform the complete new semantics should be established (from the overall discussion I got a feeling that it's still undecided how to handle key
in the end - only that it's most likely be "renamed" to @key
, but this has not been fully decided yet) and other runtimes, old ones or new ones, should be able to use them from now on. Especially for new runtimes it doesn't make sense to be forced to implement the legacy exports now.
Most helpful comment
Ok so I've done some digging and I think I have a slightly better idea of what's going on. Scratch my previous answers.
The End Goal
Like the RFC says, we want to get here:
The rest is tactics.
Key and Spread
We have a few cases we can't break right away. That's our first constraint.
In the longer term, we either want to make Key Before Spread a compile error (due to ambiguity) or, more likely, just make both of them use
"foo"
as a key. Potentially changing the syntax to@key=
in distant future to highlight it's not in the same "namespace" and thus doesn't get affected by spreading.Key Before Spread Deprecation
Since key before spread which has a
key
in it is a problematic pattern that will change the behavior, we'll need to start warning about it at some point. Now here's the fun bit. The traditionalcreateElement()
transform can't produce code warning about it.In this case,
createElement
just sees the already merged{ key: "bar" }
as its second argument, so it can't know whether we have, in fact, had a spread with an overwritten key there. We can't rely oncreateElement
to show that deprecation warning because it doesn't get enough information to detect the bad pattern.It would have to be done by a different compile target. This is why rolling out the new JSX transform widely is so important. It enables the new runtime warning.
Should
jsx
preferconfig.key
ormaybeKey
?In the end state, we just use
maybeKey
. This is clear enough. But what do we do in the meantime? We have two options:config.key
if it's notundefined
, otherwise we usemaybeKey
.maybeKey
if it's notundefined
, otherwise we useconfig.key
.There's a few things we can note about these options:
<div {...obj} key={undefined} />
(overridekey
withundefined
) and<div {...obj} />
. This is becausemaybeKey
would beundefined
in both cases. So how do we distinguish? We could hack something like look atarguments.length
but that's not ideal for a fast implementation.<div key="foo" {...obj} />
and make it fall back tocreateElement
. Because it would have gotten an incorrect key ("foo"
). But the problem is that, as we said in the section above,createElement
cannot add a warning for this case because the merging would happen before the call. FromcreateElement
perspective, it would just be a regular call with{ key: "bar" }
. So then we wouldn't be able to deprecate this pattern, which is the whole point.For these reasons, we went with Config Wins until breaking changes. Which means that we use
config.key
, and only fall back tomaybeKey
if that doesn't exist. This lets us usejsx()
for<div key="foo" {...obj} />
(config wins!), which can later add a deprecation warning specifically for this pattern (because it knowsconfig.key
andmaybeKey
clash).However, if Config Wins, we can't use
jsx()
for<div {...obj} key="foo" />
today. This is because it would incorrectly get the"foo"
key. This is why, counter-intuitively, Key After Spread doesn't usejsx()
today and falls back tocreateElement
.Summary
Let's say:
Here's what we have.
Today
<div {...obj} key="foo" />
: The key is"foo"
. Always usescreateElement
.<div key="foo" {...obj} />
: The key is"bar"
. Usesjsx()
with the new transform.Stage 1 (Future Warning in React)
<div {...obj} key="foo" />
: The key is"foo"
. Still usescreateElement
.<div key="foo" {...obj} />
: The key is"bar"
. Usesjsx()
with the new transform, which warns.Stage 2 (Future Breaking Change in React)
<div {...obj} key="foo" />
: The key is"foo"
. Usesjsx()
with the new transform<div key="foo" {...obj} />
: The key is"foo"
. Usesjsx()
with the new transform.