-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Suggestion: transforming jsx to jsx-runtime without createElement
fallback
#20031
Comments
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 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 now I think add a custom @gaearon What do you think? |
can they? arent they mutually exclusive?
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 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 |
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:
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 So, currently, there are 2 ways of how one can use
The If we consider only the second way of adding support for the The problem is with the first way - writing pragma manually. I was sure that using 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"/>; outputfunction _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 Even if it works - it still requires 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 /** @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. |
Hmm, are you saying that Emotion provides |
I think it would work in Babel 8 since that the new transform will become the default one?
Yeah, I'm not sure. Maybe because they're currently implemented as two completely separate transforms. Maybe @lunaruan or @sebmarkbage knows.
I think the 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.
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 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? |
My concern is why we need do the fallback. if we just want to pass key prop, i think
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)?
<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") |
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 it could be |
i still think However. current tools transforming |
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:
the key is the special prop as fact rule for a long time. even support
|
Considering that the last key prop (or any prop) should win, it’s very confusing that #20031 (comment) 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 |
Not sure if I understand the question correctly but Emotion's
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).
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'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
This particular input/output pair was from the TS compiler. The first one was from Babel and it has worked OK (after adding
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.
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
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 |
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. The End GoalLike the RFC says, we want to get here: function jsx(type, props, key) {
return {
$$typeof: ReactElementSymbol,
type,
key,
props,
};
} The rest is tactics. Key and SpreadWe 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 Key Before Spread DeprecationSince key before spread which has a // Classic transform
<div key="foo" {...obj} />.key // "bar"
// ->
React.createElement(
"div",
_extends({ key: "foo" }, obj)
).key; // "bar" In this case, 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
|
@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 |
The tricky case is when you have |
react/jsx-key may help to warn in where runtime warning may be confused. if we could pass |
@gaearon Thanks for your explanation. the final stage is same as i expect. I have switch to using After all tool chain ready. For developer, may need |
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 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 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.
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 Emotion+CRA users won't be able to upgrade CRA seamlessly because they are using
Which is also somewhat weird because the opposite situation ( 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). |
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! |
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. |
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.
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
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 |
Apologies for wandering into a 10 month stale discussion with questions about motivations for long-stable functionality. My own motivation, for contextI’m working on an ESM loader for Node to automatically transpile with ESBuild. This is something that seems like it would be trivial, and in fact there are a couple implementations already out there. Both are fairly incomplete in a variety of ways, enough so that I’m more comfortable building my own than trying to contribute to existing implementations. In the course of my own work on this, I’ve been watching two open issues hoping they’d gain some traction in tandem with my own efforts, but it seems pretty unlikely they will given they’re similarly stale. So my next (naive) hope was to get enough context that I could offer to contribute a solution. I already knew the I’m sure there’s plenty to find and read on the subject. Unlike an earlier comment I’m explicitly asking if someone will help me shortcut to understanding. I also know that’s unfair to ask, however… This Node loader project isn’t the thing I’m interested in working on, it’s just a yak I’ve been shaving to try to get around it, and it keeps growing weight and fur. My goal is to have a reliable and consistent JSX and TS runtime in Node, and most importantly have it perform well in tests (ruling out So…
|
@eyelidlessness semi-serious question: are you actually familiar with what
React does not pass Because of this, there's both a conceptual and a runtime difference between a field named (Apologies if you are familiar with this already - you clearly are working on some complicated stuff here, but from the context of the question it does sound like you're asking why |
This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment! |
bump |
The |
I don’t think appeals to “meant to” are going to help you here; jsx is “meant to” be used for React, it’s just become used more broadly. |
@ljharb Yes, React was the reason of the popularity of JSX, however JSX was a thing even before its adoption by React (For example https://en.wikipedia.org/wiki/ECMAScript_for_XML). |
@iMrDJAi the concept of "xml in JS" certainly was, but JSX, specifically, was created by the react team for use in react. |
@ljharb Regardless of the origins of JSX, and the |
@iMrDJAi congrats! I’m not saying at all that jsx should only be for react; I’m just saying an appeal to original intentions won’t help you :-) |
Update: I have added docs and published the package under a different name: https://github.com/iMrDJAi/xml-jsx-runtime. |
This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment! |
Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you! |
Current code transform rule is not friendly for custom
jsx-runtime
or react-like lib.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)
babel/babel#12177
microsoft/TypeScript#39199
The text was updated successfully, but these errors were encountered: