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

Implement React's jsx/jsxs Factory Changes #34547

Closed
DanielRosenwasser opened this issue Oct 17, 2019 · 34 comments · Fixed by #39199
Closed

Implement React's jsx/jsxs Factory Changes #34547

DanielRosenwasser opened this issue Oct 17, 2019 · 34 comments · Fixed by #39199
Assignees
Labels
Domain: JSX/TSX Relates to the JSX parser and emitter Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 17, 2019

https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md

Major changes for us:

  • JSX children are always installed as an array on the props object - not as trailing arguments.
  • key will be passed separately from other props (in place of children)

Also of note:

  • defaultProps will be deprecated on function components
  • Spreading key will be deprecated
  • String refs will be deprecated

We might need new jsx flags, or jsxFactory flags, or something similar. Checking might need to be changed to reflect this as well depending on if/how we resolve createElement calls.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. In Discussion Not yet reached consensus Domain: JSX/TSX Relates to the JSX parser and emitter labels Oct 17, 2019
@fabiancook
Copy link
Contributor

I currently use "jsx": "react", "jsxFactory": "h" which can be seen here:

https://github.com/opennetwork/vgraph.dev/blob/master/tsconfig.json#L25

h follows the style of the current createElement function, will there be an alternative that can be used in place?

@refhu
Copy link

refhu commented Dec 10, 2019

https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md

Cambios importantes para nosotros:

  • JSX childrensiempre se instala como una matriz en el objeto de utilería, no como argumentos finales.
  • keyse pasará por separado de otros accesorios (en lugar de children)

También de nota:

  • defaultProps será obsoleto en los componentes de la función
  • La difusión keyserá obsoleta
  • La cadena refsquedará en desuso

Es posible que necesitemos nuevas jsxbanderas, o jsxFactorybanderas, o algo similar. Comprobación necesidad fuerzas para ser cambiado para reflejar esto también dependiendo de si / cómo resolvemos createElementlas llamadas.

@weswigham
Copy link
Member

It's nearing the end of January 2020, coming up on 1 year from when reactjs/rfcs#107 was opened. It's still open for comments, a finalized set of changes still having not been accepted. I guess we'll keep monitoring it?

@BPScott
Copy link

BPScott commented Mar 20, 2020

The first step has just been completed and shipped in Babel 7.9.0:

Blog post: https://babeljs.io/blog/2020/03/16/7.9.0#a-new-jsx-transform-11154-https-githubcom-babel-babel-pull-11154
PR: babel/babel#11154

This new transform eliminates the need for users having to add React in scope of their js files in order for jsx to work, as it is injected by the transform. There's some talk in the PR of why this isn't solved with pragmas:

Distinguishing between jsx/jsxs/jsxDEV/createElement is an implementation detail for React and will change in the future. Therefore, we don't think that it makes sense to add a pragma option for all of them. In addition, the signature for jsx/jsxs/etc. is different than createElement, so Preact/Inferno/etc. will not be able to use the new transform the way they currently are.

I'm not familiar with the internals of this PR, I'm just an enthused bystander but I think changes would be something like:

  • When the jsx compiler option is set to preserve or react-native, don't throw a warning if you use JSX but React is not in scope - as the import to the jsx factory function would be added by a further transform step (i.e. Babel)
  • Add a new value to the jsx compiler option jsx-automatic (name up for debate, but this is based on the option for the babel transform), that will transform code in a similar manner to babel-plugin-transform-react-jsx as discussed in Add experimental version of the babel-plugin-transform-react-jsx transform babel/babel#11154.

@itsdouges
Copy link

Will adding the import work here? TS Transformers are unable to successfully add imports during a transformation - unless internally it's done a little differently for jsx transformation (before the binding step)? Which is a shame tbh - would love for transformers to be first class! 😞

@Andarist
Copy link
Contributor

Keep also in mind that a new pragma (and similar option) has been added - it’s /* @jsxImportSource foo */ which adds such imports (based on development flag):

import { Fragment, jsx, jsxs } from "foo/jsx-runtime"
import { Fragment, jsxDEV } from "foo/jsx-dev-runtime"

@DanielRosenwasser
Copy link
Member Author

Having though about this a bit, I have a few implementation concerns. We need some feedback from the React core team on this so that we can give a good experience for everyone.

Naming

It's not clear what the new flag that users should use is. While this sounds trivial, I really want to get this right. Do we name it react-jsx? That might be really confusing for new users where react is already an option. That also brings us to versioning concerns.

Versioning

It sounds like the intent is for React.jsx and React.jsxs to be opaque functions so that transformers can use different functions in the future. This way, users can't take a hard dependency on the jsx/jsxs functions, and an upgrade in React might imply an upgrade in the transformer. (please correct me if this is wrong)

That's not possible for us since our transformer is part of our codebase and can't be versioned differently. Any new transform behavior would need a new jsx flag on our end.

I think something we need to know from the core team is what the expectation is of incompatible updates to the transformer.

Implicit Import Behavior

The current proposal is for JSX expressions to implicitly import "react" so that users don't have to. In the Babel world, this is fine because there's an automatic assumption that users are always using modules; however, in TypeScript, we only assume a module if we see some import or export. This means that a JSX expression in the tree can automatically convert a file into a module, and for files that have no other imports, we'd have to potentially do a full walk of the tree to decide whether a file is a module.

This also means users using the new JSX emit mode in global .tsx files would potentially be broken. I'm not sure how many people use JSX without using modules nowadays, but it's something that needs to be recognized.

What we'd probably end up doing is saying that under react-jsx, a file must be a module (in that it needs to have at least one import or export). Users could get a quick fix in the editor to add an explicit import * as React from "react", but it would be a weird divergent behavior that might be confusing.

@Jessidhia
Copy link

Jessidhia commented Apr 16, 2020

The babel transform necessarily requires either ES or CJS modules to be available (babel import helpers automatically add one or the other depending on sourceType being script or module); it also doesn't import 'react': jsx and jsxs are not exposed at all in the React object or react module and are only available on (what currently is the) react/jsx-runtime or react/jsx-dev-runtime modules.

It's an emit mode specifically for modules (commonjs or esm); global scripts should still use React.createElement, as that is the only method that is exported from the React global. This means TypeScript would need to fall back to React.createElement wherever it assumes the script is global, maybe with a warning. The babel transform itself has React.createElement fallbacks, though not one that depends on the assumed module format.


As an aside, the quick fix I'd recommend for turning a file into a module is to add an export {}; anything else may have unintended consequences.

@trusktr
Copy link

trusktr commented May 8, 2020

@Jack-Works In that case, you can write with two different file extensions, like .tsx and .foo.tsx, set "jsx":"preserve" in tsconfig, then handle each file type differently (i.e compile the output .jsx and .foo.jsx file differently). This won't work if you want to mix them in the same file though, but you can keep separate files for a workaround.

@craigkovatch
Copy link

The deprecation of defaultProps on FCs seems particularly ugly when viewed through a TypeScript ergonomics lens, added my comments on their RFC: reactjs/rfcs#107 (comment)

@weswigham
Copy link
Member

weswigham commented May 13, 2020

Personally, I've always just written my components along the lines of

import * as React from "react";
import * as ReactDOM from "react-dom";

interface CompProps {
  col?: number;
  row?: number;
}

const Comp = ({ row = 0, col = 0 }: CompProps) => <p>{row},{col}</p>;

const App = () => <Comp row={1} />;

ReactDOM.render(<App />, document.querySelector("body"));

Playground Link

which is notable for two things - never explicitly referencing a type from React in the components (they're just... functions... which happen to return elements), and being totally unaffected by the removal of defaultProps. (Downside is you need to write out the type of children and ref yourself if you use them... but that's usually not too big a deal)

@craigkovatch
Copy link

craigkovatch commented May 13, 2020

@weswigham thanks for the comment but not sure that addresses the issue I meant to bring up -- namely, if I have a lot of props (say 10-20) but only want to default a few of them, now I have to name every one of them in the destructuring or awkwardly rest them out ala { row = 0, col = 0, ...otherProps }. Am I missing something?

Also for context, the vast majority of my day job is creating/supporting/maintaining a couple of UI libraries, so in order to help with best practices for lots of consumers, I prefer using the @types/react types e.g. React.FC :)

@itsMapleLeaf
Copy link

or awkwardly rest them out ala { row = 0, col = 0, ...otherProps }.

This is what I do. I don't find this awkward

Also for context, the vast majority of my day job is creating/supporting/maintaining a couple of UI libraries, so in order to help with best practices for lots of consumers, I prefer using the @types/react types e.g. React.FC :)

Concretely, how does React.FC help you do this? What's the best practice here?

@Retsam
Copy link

Retsam commented May 13, 2020

@craigkovatch I don't think using React.FC is best practice. There's basically no benefits to using it, and a number of downsides that you might run into. I don't want to tangent this thread on the point; but I've described in detail some of the downsides here: facebook/create-react-app#8177

@dsebastien
Copy link

@Retsam I second this. In my book and based on my research, I've advised against the use of React.FC. Those types only add noise and hinders readability.

@nojvek
Copy link
Contributor

nojvek commented Jun 18, 2020

My ask for Typescript team regarding the RFC is not to treat React is a special snowflake where Typescript only works well with React but not other frameworks. JSX as an interface is adopted by many libraries. The jsxFactory interface i.e fn(tag: string | Function, props: Record<string, any> | null, ...children: SomeType), please don't break that. It seems this RFC is proposing to break that api and move children into props. I don't fully understand what we gain by doing that as it makes typing props more complicated. Why shove more things inside a property bag when they can be separately typed params?

I know React authors don't have to consult Typescript before making breaking changes to their apis and you have to follow their decisions but please keep rest of js ecosystem in mind.

@nstepien
Copy link

@nojvek have you read the link in the OP? They're not changing the behavior for createElement.

@nojvek
Copy link
Contributor

nojvek commented Jun 18, 2020

Change JSX transpilers to use a new element creation method.
 - Always pass children as props.
 - Pass key separately from other props.
 - In DEV,
   - Pass a flag determining if it was static or not.
   - Pass __source and __self separately from other props.

The goal is to bring element creation down to this logic:

function jsx(type, props, key) {
  return {
    $$typeof: ReactElementSymbol,
    type,
    key,
    props,
  };
}

How is one supposed to infer the meaning of Change JSX transpilers to use a new element creation method. and Always pass children as props. ?

I read it as asking for changes being make to the jsxFactory function api ?

Am I misreading this ?

@nstepien
Copy link

@nojvek
Copy link
Contributor

nojvek commented Jun 18, 2020

Thanks @nstepien, that deffo provides more context.

IIUC (If I understand correctly) The ask from typescript is to have another compiler flag like jsxRuntime: automatic | classic like babel right ? i.e Take existing jsx but emit in a different manner depending on runtime flag?

I guess in a way this boils down to the eternal "bake it into core" vs "make it an extension" tradeoff.

There are other interesting projects going on around in the JSX space. e.g https://github.com/ryansolid/solid

Solid works with jsx but emits code like https://svelte.dev/. It does away with virtual dom overhead.

So in terms of Typescript direction, the question that begs answering is if Typescript does specific emits for new React jsxs and new compiler flags, would typescript also do emits for other frameworks that emit jsx in different ways. How much does Typescript give special preference to React over other frameworks?

Is this something that should be done via the transformer plugin api, so it's extensible enough but the core responsibility of Typescript remains at the parser + checker level.

Again, this is just food for thought.

I'm all for evolution of frontend tooling to be simpler, more performant and easier to use.

@ScottAwesome
Copy link

Thanks @nstepien, that deffo provides more context.

IIUC (If I understand correctly) The ask from typescript is to have another compiler flag like jsxRuntime: automatic | classic like babel right ? i.e Take existing jsx but emit in a different manner depending on runtime flag?

I guess in a way this boils down to the eternal "bake it into core" vs "make it an extension" tradeoff.

There are other interesting projects going on around in the JSX space. e.g https://github.com/ryansolid/solid

Solid works with jsx but emits code like https://svelte.dev/. It does away with virtual dom overhead.

So in terms of Typescript direction, the question that begs answering is if Typescript does specific emits for new React jsxs and new compiler flags, would typescript also do emits for other frameworks that emit jsx in different ways. How much does Typescript give special preference to React over other frameworks?

Is this something that should be done via the transformer plugin api, so it's extensible enough but the core responsibility of Typescript remains at the parser + checker level.

Again, this is just food for thought.

I'm all for evolution of frontend tooling to be simpler, more performant and easier to use.

@nojvek

I think the wise move is definitely to make it as generic (and therefore, pluggable) as possible, though, I expect most frameworks that rely on JSX will over time migrate to the new jsx and jsxDev style functions, if history is any precedent here.

The real issue with leaving it up to being plugins, is that the Compiler API documentation just isn't great. It feels very neglected in comparison (I have this same gripe with babel, for what its worth). Also, with plugins you can only load them via the Node Compiler APIs, you can't have (IIRC) transform plugins added into the tsconfig under plugins (that appears to only be reserved for the editor plugins).

If the story around Plugins gets cleaned up (at least with much better and more detailed documentation, even if the API isn't completely stable), then relying on plugins is the way forward, and you could use React as a reference implementation of said plugins. I think that would be very acceptable.

@nojvek
Copy link
Contributor

nojvek commented Jul 15, 2020

If the story around Plugins gets cleaned up (at least with much better and more detailed documentation, even if the API isn't completely stable), then relying on plugins is the way forward, and you could use React as a reference implementation of said plugins. I think that would be very acceptable.

perhaps something to think about @DanielRosenwasser re: making compiler plugins more friendly so Typescript doesn't have to bake everything in it's core.

I'd love to have tsconfig.json support plugins like https://github.com/cevek/ttypescript. There's #14419 which as a ton of 👍 upvotes and wouldn't be too hard to implement.

However some of the Typescript core members have mentioned it won't come anytime soon. I'm not sure exactly why. #14419 (comment)

In any case, don't want to derail the conversation on this topic. I'm not a core Typescript member, it's their call at the end of the day. All I care is that Typescript remains nimble and does a few things really really well like Typechecking and code completion, rather than try to do many things but in a subpar way. It's already a 50MB install.

@xiaoxiangmoe
Copy link
Contributor

xiaoxiangmoe commented Jul 23, 2020

@DanielRosenwasser Will this release in TypeScript 4.0.1 ?I found this issue is in Milestone 4.0.1

@DanielRosenwasser
Copy link
Member Author

Thanks, I've rescheduled to 4.1.

@weswigham
Copy link
Member

Specifically, we have a draft up (#39199), but the babel transform equivalent is still experimental, and the react changes to allow use of this emit haven't shipped in a stable react version yet; so we may wait until react 17 actually ships.

@jeswin
Copy link

jeswin commented Dec 30, 2020

So in terms of Typescript direction, the question that begs answering is if Typescript does specific emits for new React jsxs and new compiler flags, would typescript also do emits for other frameworks that emit jsx in different ways. How much does Typescript give special preference to React over other frameworks?

@nojvek Yet another JSX-based framework author here. The new changes (jsx and jsxImportSource tsconfig options) have made things simpler actually. The emitted code seems generic and not tied to React. At least for me, it was quite easy to adopt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: JSX/TSX Relates to the JSX parser and emitter Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
Design Meeting Docket
  
Awaiting triage
Development

Successfully merging a pull request may close this issue.