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

Add emit support for jsx/jsxs experimental jsx runtime api #39199

Merged
merged 2 commits into from Sep 11, 2020

Conversation

weswigham
Copy link
Member

This adds two new options for the jsx compiler option - react-jsx, and react-jsxdev. They use the jsx and jsxDEV constructors and the react/jsx-runtime and react/jsx-dev-runtime entrypoints, respectively. The primary differences, as noted in the linked issue, are that children are passed as part of the props object, and the key is passed as a separate argument. (The jsxDEV constructor further takes some debug data.) In addition, this adds a jsxImportSource compiler option (and @jsxImportSource file pragma) to control the root specifier the jsx runtime is imported from (which defaults to react). In it's current state, I believe this is usable, but there are some known flaws:

  • The checker/program need to load the jsxImportSource module and use it as the source for the JSX namespace automatically. Presently you need to include it in the program yourself (ie, with a types directive or compiler option). This is moreso a TODO for myself, since I don't think there's any question to what we should do (implicitly include the package or @types version in the program). This is more interesting for the jsxImportSource pragma, which, for all intents and purposes, is an import statement. We have a lot of services for real import statements - should they be applied to the pragma, where possible?
  • Babel additionally supports a jsxRuntime pragma (with values classic or automatic) to swap between the old emit and the new one. This is supportable; however currently I only key our emit off the jsx compiler option, and the presence of the jsxImportSource pragma.
  • In some cases, the new transform still falls back to createElement (when a key prop follows a spread) - this is a kind of soft deprecation; however it still implicitly imports the jsx runtime and pulls in createElement. This PR does not yet do this, as, awkwardly enough, the module createElement is supposed to come from is different from the jsx-runtime module entrypoint. I could make this work, but I think this is something we should provide some feedback on - so long as there's still a createElement fallback required in the API, why can't it be exported by the same entrypoint exporting jsx or jsxDEV?
  • Without an import or export, a file is not marked as a module, and will not have an import automatically added. Should this issue an error? Or should, under these jsx modes, the presence of a jsx tag imply module-ness?

Fixes #34547

This is currently experimental in babel, and AFAIK not yet supported in a stable version of react; as such, we should probably continue to experiment with it for awhile before merging it into a stable release.

TL;DR

When jsx: react-jsx (and target: esnext) is set,

export default (props: {className: string}) => <div className={props.className} key="stablekey">childtext</div>

compiles to

import { jsx as _jsx } from "react/jsx-runtime";
export default (props) => _jsx("div", Object.assign({ className: props.className }, { children: "childtext" }), "stablekey");

@weswigham weswigham added the Experiment A fork with an experimental idea which might not make it into master label Jun 22, 2020
@weswigham
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2020

Heya @weswigham, I've started to run the tarball bundle task on this PR at 330af8d. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2020

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/77435/artifacts?artifactName=tgz&fileId=73CABD8F9DC99542733F288ACADBC7457DB393374166FA900EBB0301EE5C325B02&fileName=/typescript-4.0.0-insiders.20200622.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@Andarist
Copy link
Contributor

The checker/program need to load the jsxImportSource module and use it as the source for the JSX namespace automatically.

This is an interesting piece of information for me - or it might be. Could you clarify for me if this works on per-config basis (or per-file when using @jsxImportSource)? In other words - could one compile a library with a particular JSX namespace for it but later have it consumed by an application with other JSX namespace "defined" for it and one would not affect the other?

Right now JSX "pollutes" the global namespace and AFAIK what I'm talking about is not possible.

@weswigham
Copy link
Member Author

Right now JSX "pollutes" the global namespace and AFAIK what I'm talking about is not possible.

We actually already support localized JSX namespaces (and therefore loading multiple into a project (and using them via pragmas)) - react just makes a global one (which we also still support for ecosystem compatibility). We'll look for a JSX member on the root of the factory function. Eg, if the factory is defined as React.createElement, we'll check for React.JSX.

@Andarist
Copy link
Contributor

Huh, that's great to hear - I will have to experiment with how it behaves to get a better feeling of how it works in various situations. 👍

Do you happen to know when the support for this was added? If @types/react would be to remove the global namespace in favor of a localized one then how much of a breaking change that would be?

With this PR it would be great to be able to customize the jsxImportSource in tsconfig.json. It seems a real bummer that the config only allows a strict set of values and it enforces the usage of pragma for all the other modules.

@weswigham
Copy link
Member Author

Do you happen to know when the support for this was added?

Same time as support for per-file @jsx pragmas.

If @types/react would be to remove the global namespace in favor of a localized one then how much of a breaking change that would be?

Pretty bad, I think. There's a lot of JSX.IntrinsicElements references out there that assume react.

@Andarist
Copy link
Contributor

Andarist commented Jul 25, 2020

This is awesome - I have been searching for a way to define such local JSX namespace for a while and wasn't aware that this is already implemented!

I've played a little bit with it and it seems to work pretty well - I only have one caveat with it: it doesn't support /** @jsx jsx */ as it's not a member expression. Or at least I couldn't figure out how to make it work. As the whole thing only seems to work with namespaces anyway as you can't really put types onto runtime values - would it be possible to extend the support for it in a way that if a particular simple binding that is used as a pragma is imported from a module then this would check if the module itself exports JSX? I might be wrong - but the change seems quite logical and shouldn't be that hard to implement, given the fact that the whole notion of localized JSX is already implemented. I could even try to implement support for this myself. Would that be something that TS team could endorse?

EDIT:// I've tracked down the PR adding support for this and it seems that what I'm after can actually be done by defining a factory function and a namespace with the same name. I had no idea that this is possible. In a way, my problem is solved with this - but I kinda still believe that looking up for a module-level JSX would be nice.

@weswigham weswigham marked this pull request as ready for review September 9, 2020 20:45
@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Sep 9, 2020
@orta
Copy link
Contributor

orta commented Sep 11, 2020

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 11, 2020

Heya @orta, I've started to run the tarball bundle task on this PR at 330af8d. You can monitor the build here.

@Andarist
Copy link
Contributor

Would it be possible for you to support a new option (existing "jsx" would have slightly conflicting semantics with it), smth like jsxImportSource? I'm a maintainer of Emotion - a popular CSS-in-JS library and we recommend people to use our pragma approach for styling which works great and a tip from this thread by @weswigham has even allowed us to add more proper type-checking for our css prop API, but many users are complaining about having to add pragma manually over the place. It would really make a difference for us if this could be configured globally per tsconfig.json project

Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

This looks good to me.

My admittedly naive feelings on whether a tsx file should be treated as a module in this context is that it should. I don't know how you can't ever get a JS file which represents a script with these new imports.

@weswigham
Copy link
Member Author

I've opened #40502 and #40501 as followups to this PR to further refine the experience offered. I'm going to merge this as-is so we start getting feedback on the meat of the implementation (the emit code) during the upcoming nightlies and beta.

@weswigham weswigham merged commit a36f17c into microsoft:master Sep 11, 2020
@devarsh
Copy link

devarsh commented Oct 9, 2020

Question: Why vscode shows me Value is not accepted. Valid values: "preserve", "react", "react-native" and not recognizing react-jsx reacxt-jsxdev variables and keeps throwing me
Cannot use JSX unless the '--jsx' flag is provided error where I'm using JSX syntax in my code

@magic-akari
Copy link
Contributor

Question: Why vscode shows me Value is not accepted. Valid values: "preserve", "react", "react-native" and not recognizing react-jsx reacxt-jsxdev variables and keeps throwing me
Cannot use JSX unless the '--jsx' flag is provided error where I'm using JSX syntax in my code

Hello @devarsh,
Using the workspace version of TypeScript may help.

@devarsh
Copy link

devarsh commented Oct 9, 2020

Question: Why vscode shows me Value is not accepted. Valid values: "preserve", "react", "react-native" and not recognizing react-jsx reacxt-jsxdev variables and keeps throwing me

Cannot use JSX unless the '--jsx' flag is provided error where I'm using JSX syntax in my code

Hello @devarsh,

Using the workspace version of TypeScript may help.

Thanks this worked. I figured vscode was using an current stable version and not next 😊

@morlay
Copy link

morlay commented Oct 16, 2020

@weswigham The fallback to createElement should be createElement of @jsxImportSource.

https://www.typescriptlang.org/play?ts=4.1.0-dev.20201015#code/PQKhAIAECsGcA8CSBbADgewE4BcDK6BXTAYwFNxVNSBDY7cEYAKCYB5ZVqA7cAbwDpBldKlgBfcMAB8Ldpx4BrUgE8AvACJq6voP7DRE6bI7cdQzCPHgla9QBNqsbdKA

if using React.createElement, it is a different results from babel, and will broken react-like lib.

facebook/react#20031 (comment)

@gaearon
Copy link

gaearon commented Oct 17, 2020

Note #41146 is critical to fix not only for React-like libs, but for React itself too, since it would otherwise lead to a non-existent reference being emitted.

@Andarist
Copy link
Contributor

I have started looking into fixing this - doesn't look overly complicated but I haven't contributed much to the codebase, so we'll see how it goes.

@Andarist
Copy link
Contributor

@gaearon I've implemented this locally already - however, I've discovered that I will have to adjust the implementation further. I'll probably wrap this up tomorrow, but I've wanted to report back here, especially because of the obstacle I've encountered. TS compiler currently assumes that all imports that have to inserted comes from a single entry and thus after my changes the createElement gets imported from 'whatever/jsx-runtime.js'. This seems like a good testament that the way this got handled is confusing for people - most assume that everything JSX-related is put into those special new entries, but unfortunately, it's not the case because of the createElement quirk.

@gaearon
Copy link

gaearon commented Oct 17, 2020

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. It confused me too, I dug out the reasoning and wrote it up in the linked thread. I hope it’s helpful. I’m also happy to see alternative suggestions in the React issue thread.

@gaearon
Copy link

gaearon commented Oct 17, 2020

Re: confusion — if this is the criticism of the rollout and communication, I fully agree we could do a better job about this case. It is indeed very subtle (and I only understood it yesterday myself). 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.

@laurentlucian
Copy link

VScode shows me Value is not accepted. Valid values: "preserve", "react", "react-native" warning, even with TS workspace version on 4.1.1-rc

@weswigham
Copy link
Member Author

vscode uses this schemastore definition to provide help for tsconfig files. It needs to be updates for the release.

07akioni added a commit to 07akioni/schemastore that referenced this pull request Nov 19, 2020
typescript has support new jsx transformation, see microsoft/TypeScript#39199
madskristensen pushed a commit to SchemaStore/schemastore that referenced this pull request Nov 19, 2020
@eps1lon
Copy link
Contributor

eps1lon commented Nov 20, 2020

@weswigham Can we sync next time if this requires work on @types/react? DefinitelyTyped/DefinitelyTyped#48971 was open for a month now and now we learn that "jsx": "react-jsx" requires additional work in @types/react without any guidance how this should look like.

@Andarist
Copy link
Contributor

If you are referring to the types for the JSX namespace (because otherwise you only have to provide standard types for jsx, jsxs and jsxDEV factories) then you can look into the tests introduced in those PRs:

I've also commented in that DefinitelyTyped PR.

@Jack-Works
Copy link
Contributor

Can we sync next time if this requires work on @types/react?

This is the reason I'm getting this error?

error TS7016: Could not find a declaration file for module 'react/jsx-runtime'. 'node_modules/react/jsx-runtime.js' implicitly has an 'any' type.

 If the 'react' package actually exposes this module, consider sending a pull request to amend 'https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react\`

@eps1lon
Copy link
Contributor

eps1lon commented Nov 20, 2020

This is the reason I'm getting this error?

It seems to me. Changes like DefinitelyTyped/DefinitelyTyped#49701 fix the issue for me so it seems like the new compiler options need new releases of @types/react first.

@chandrarishabh
Copy link

can we use this with TypeScript3.9?

@DanielRosenwasser
Copy link
Member

No, it's a 4.1 feature.

@slorber
Copy link

slorber commented Jun 8, 2023

Was wondering: is there a way to emit preserved JSX, but using a .js extension instead of .jsx?

On Docusaurus we use jsx: "react-native" just for the sake of outputting the .js extensions, which is a bit weird.

Wouldn't it make sense to introduce a more generic "preserve-js" option or something? (exact same behavior as "react-native", just an alias)

@DanielRosenwasser
Copy link
Member

React Native was a weird (admittedly frustrating) case where Metro insisted on not accepting any file type other than .js. Is there a reason Docusaurus can't use .jsx?

@slorber
Copy link

slorber commented Jun 9, 2023

React Native was a weird (admittedly frustrating) case where Metro insisted on not accepting any file type other than .js. Is there a reason Docusaurus can't use .jsx?

Docusaurus can use .jsx but we'd prefer not to.

We use tsc to output clean React theme components that can be "swizzled": the React theme components are then copied to the user's source code for shadowing the original implementation so that users can override the default UI we provide.

Using .jsx is not very common these days and might be confusing for our users (not all of them are React/frontend devs) so it's probably better if the copied files have the .js extension, like the rest of the "modern" React industry does today.

We can probably rename the extension at copy time but it would look cleaner if there was an official way to output js instead of jsx. Not a big deal though, we have 2 possible workarounds ("react-native" or changing the extension ourselves)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Experiment A fork with an experimental idea which might not make it into master For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement React's jsx/jsxs Factory Changes