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

feat: allow custom jsxImportSource #10583

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hermansje
Copy link

@hermansje hermansje commented Feb 20, 2021

PR content

This PR allows configuring a custom value for the importSource option of @babel/preset-react.

TypeScript users can set the corresponding compiler option jsxImportSource in tsconfig.json (introduced in TypeScript 4.1) to configure the JSX import source.
The compiler option will be removed if from tsconfig.json if Babel won't use it.

JavaScript users, or TypeScript users that want to override tsconfig.json, can set the environment variable JSX_IMPORT_SOURCE.

Motivation

This PR implements the feature request motivated in #9847 (Emotion without pragma + consistency between tsconfig and Babel) and supersedes the implementations of two existing pull requests #10138 (why-did-you-render, Preact) and #10580.

While this project tries to avoid flags, there are some considerations that might make the case for an exception: the limited logic, the parallel in the implementation with the React runtime heuristic, the integration with tsconfig.json, the assistance to TypeScript users and the benefits of the feature for React users (e.g. ease of use of libraries like Emotion, compatibility with developer tooling like why-did-you-render).

Test plan

I tested with a React + TypeScript + Emotion setup (compilerOptions.jsxImportSource in tsconfig.json set to "@emotion/react").

  • With the published version, the css prop is not transformed to classes.
  • With this branch, the css prop is correctly transformed to classes
  • When called with JSX_IMPORT_SOURCE=react the result is the same again as with the published version (because the environment variable takes precedence)
  • When the requirements for this feature are not met, a message is added to the list of tsconfig changes and jsxImportSource is removed

Screenshot 2021-02-20 at 17 06 55

@facebook-github-bot
Copy link

Hi @hermansje!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

if (parsedCompilerOptions.jsxImportSource != null) {
if (hasJsxRuntime && semver.gte(ts.version, '4.1.0-beta')) {
if (process.env.JSX_IMPORT_SOURCE == null) {
process.env.JSX_IMPORT_SOURCE = parsedCompilerOptions.jsxImportSource;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this shouldn't set this global but rather it should be passed around using normal arguments etc. Assigning to a global pollutes the global env and might be surprising - the relation between the files involved in this becomes much more implicit and much less obvious.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for taking a look at this @Andarist!
I know, I'm not a big fan either. I wrote it this way because an environment variable is used to control the related JSX transform (DISABLE_NEW_JSX_TRANSFORM), because it enables the feature for JavaScript users and because it made a first version easy to implement 🙂

An alternative implementation, at least for the TypeScript use cases, would be extracting the parsing of the tsconfig.json file and passing the result to the verify function and (a part of it) as an extra argument to configFactory.

Based on feedback on this PR, I can change the implementation to one that the maintainers are happy with.

Copy link
Contributor

Choose a reason for hiding this comment

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

The usage of process.env.JSX_IMPORT_SOURCE is not that bad - as you have mentioned, some other stuff is handled in a similar manner. The problem (from my PoV) is that you also write to a process.env and not only read from it.

Also - I'm just a bystander here, so it might be best to wait for maintainers' feedback before pursuing anything that I suggest 😉

@SimonChaumet
Copy link

When packing it from source I can't get it to work, do you have a way to make it work in "manual" ?

@stale
Copy link

stale bot commented Jun 26, 2021

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jun 26, 2021
@Andarist
Copy link
Contributor

Bump for the stale bot - I believe that people are still interested in this feature

@stale stale bot removed the stale label Jun 26, 2021
@JustTSK
Copy link

JustTSK commented Aug 5, 2021

Any update on this?

@iceColdChris
Copy link

iceColdChris commented Oct 21, 2021

Starting to use MUI5 with emotion and react 17 would love to use this!

@stale
Copy link

stale bot commented Jan 8, 2022

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 8, 2022
@Andarist
Copy link
Contributor

Andarist commented Jan 8, 2022

Bump for the stale bot - I believe that people are still interested in this feature

@stale stale bot removed the stale label Jan 8, 2022
@cfvbaibai
Copy link

cfvbaibai commented Jan 31, 2022

Projects in my company heavily use React 17 + MUI 5. This PR would allow us to remove the annoying @jsxImportSource pragma on all files, which greatly improved the maintainability. I also heard similar comments from other developers around me. We highly appreciate the contribution from @Andarist

@MattiasMartens
Copy link

Looks like this has been dormant for a month or seven. Since Material UI is now recommending Emotion as its go-to styling framework, I suspect the issue this PR solves will be affecting more and more developers.

@stgraham2000
Copy link

I'm working on a project as well with CRA+MUI5+Typescript+Emotion and I can confirm that without this PR I require the pragma line /** @jsxImportSource @emotion/react */ at the top of my component files and with this PR I can remove that line from the top of my files.

I do however agree with some of the review comments that the approach of writing to the process.env is not a good idea. If there is still willingness by the maintainers to take this through to completion then I would be happy to investigate an alternative to the current approach if that's the main sticking point right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants