Stop using old JSX transform #12253

Andarist · 2020-10-25T08:18:59Z

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

As the new JSX transform has been shipped and is already used in production by many I think we can safely just switch to it - the new transform has been called "automatic" at some places but it actually is capable of handling the classic transform as well and I believe the plan was to just replace the old code with the new one. It just got implemented this way as at first it was, in fact, experimental.

To make this actually happen in Babel 7 I had to introduce useSpread and useBuiltIns options to the new transform but the plan is to remove them in Babel 8. I've focused right now on making tests pass so those options are only used by createElement - to avoid confusing people I would like to actually use them for new factories as well. I also need to split the code a little bit to make it easier to just remove the compatibility code created for those options later (without having to refactor stuff too much to make it happen).

I think we should just move the -experimental code to the old "builder" package and just remove the -experimental directory. Is this approach OK with you?

I will also put some additional "inline" comments near some code lines to explain a few things with a better context.

packages/babel-helper-builder-react-jsx-experimental/src/index.js

Andarist · 2020-10-25T08:21:33Z

packages/babel-helper-builder-react-jsx-experimental/src/index.js

@@ -10,6 +10,29 @@ const DEFAULT = {
 };

 export function helper(babel, options) {
+  const { useSpread = true, useBuiltIns = false } = options;
+
+  if (typeof useSpread !== "boolean") {


this is an opposite check to what had to be introduced at an upper level (jsx transform) because from the perspective of this builder the useSpread: true is default but we need to keep the compat for public packages which had useSpread: false as default

Andarist · 2020-10-25T08:22:15Z

packages/babel-helper-builder-react-jsx-experimental/src/index.js

-          state.get("@babel/plugin-react-jsx/usedFragment") &&
-          !state.get("@babel/plugin-react-jsx/pragmaFragSet")
-        ) {
-          throw new Error(


I don't think this makes much sense - it's overly restrictive. The old transform had no such validation and TS also has never thrown for this situation:
https://www.typescriptlang.org/play?ts=4.1.0-beta#code/PQKhAIAECsGcA9x0SYAoNBuAPAEwJYBuAfNsASRjqcMUA

Can you just comment out this code for now?

I'm not 100% sure that we should remove this check (well, we should now for backward compat, but we might re-introduce it in Babel 8). The question is: should we prioritize for React libraries (e.g. Emotion) or for other "frameworks" (e.g. Preact)?

In the first case it's better not to throw because it should "work by default", while in the second case it's better to "prevent a mistake by default".

The question is: should we prioritize for React libraries (e.g. Emotion) or for other "frameworks" (e.g. Preact)?

Yeah, definitely a concern and I don't have a perfect answer for that. My main motivation factors here are that not throwing in this case is:

backward-compatible

matching how TS behaves

Using pragmas is not a very common use case so I would be inclined to favor backward compatibility, usually, people should really have those things configured globally per project in a config file. Pragmas are mostly useful in configuration-constrained tools.

Andarist · 2020-10-25T08:22:41Z

packages/babel-helper-builder-react-jsx-experimental/src/index.js

@@ -804,8 +814,9 @@ You can set \`throwIfNamespace: false\` to bypass this warning.`,
   * breaking on spreads, we then push a new object containing
   * all prior attributes to an array for later processing.
   */
-  function buildCreateElementOpeningElementAttributes(path, attribs) {
-    const props = [];
+  function buildCreateElementOpeningElementAttributes(file, path, attribs) {


changes here are based on the old transform

Andarist · 2020-10-25T08:23:07Z

packages/babel-plugin-transform-react-jsx/src/index.js

  }
+
+  const visitor = helper(api, {


this is basically just "./transform-automatic" inlined here

codesandbox-ci · 2020-10-25T08:24:14Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4b3c31a:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

babel-bot · 2020-10-25T13:59:30Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/33065/

kripod · 2020-10-27T22:36:57Z

I'm not sure if this is strongly related or a new issue should be opened, but the ecosystem might not be ready for such a big change yet. Please see airbnb/babel-plugin-inline-react-svg#91 for further details.

nicolo-ribaudo · 2020-10-27T23:09:16Z

@kripod We have two transforms: one that only supports the classic runtime, and one that supports both classic and automatic. We are getting rid of the old transform, but not of the default classic support 😉

nicolo-ribaudo · 2020-10-27T23:13:32Z

packages/babel-helper-builder-react-jsx-experimental/src/index.js

-          state.get("@babel/plugin-react-jsx/usedFragment") &&
-          !state.get("@babel/plugin-react-jsx/pragmaFragSet")
-        ) {
-          throw new Error(


Can you just comment out this code for now?

I'm not 100% sure that we should remove this check (well, we should now for backward compat, but we might re-introduce it in Babel 8). The question is: should we prioritize for React libraries (e.g. Emotion) or for other "frameworks" (e.g. Preact)?

In the first case it's better not to throw because it should "work by default", while in the second case it's better to "prevent a mistake by default".

nicolo-ribaudo · 2020-10-27T23:14:50Z

packages/babel-plugin-transform-react-jsx/src/index.js

+    development: false,
+    runtime,
+    useSpread:
+      "useSpread" in options ? options.useSpread : runtime !== "classic",


Can we throw if useSpread is set with the automatic runtime?

This is somewhat tricky because the runtime option doesn't match the actual runtime that will be used when transforming.

What should happen for:

// config const options = { runtime: 'classic', useSpread: false } // ... /** @jsxRuntime automatic */ <div {...props} key="foo" />

If we want to keep using extends helper etc here (which seems rather logical) then this would also imply that we should implement extends helper support for jsx, jsxs and jsxDEV outputs, right?

If we want to keep using extends helper etc here (which seems rather logical)

No, this should also throw.

I think that we can throw if:

runtime: "automatic" and useSpread: false are set in the options (when instantiating the plugin)

useSpread: false is explicitly set in the options and there is a /** @jsxRuntime automatic */ comment (when traversing) <-- In this case it's maybe also ok to just warn and ignore useSpread: false, depending on what would be a breaking change.

runtime: "automatic" and useSpread: false are set in the options (when instantiating the plugin)

Technically this was previously allowed, even if useSpread has been silently ignored in such a case. I'm OK with adding an error in a situation like this - I just want to ensure myself that you are as well.

useSpread: false is explicitly set in the options and there is a /** @jsxRuntime automatic */ comment (when traversing) <-- In this case it's maybe also ok to just warn and ignore useSpread: false, depending on what would be a breaking change.

Previously the /** @jsxRuntime automatic */ was completely ignored if the runtime was set to "classic" at the plugin level. Technically one could have it in a file but it would have no effect.

So what you are proposing is that whenever we use the automatic runtime we should ignore options like useSpread regardless of if the runtime has been chosen at the plugin or a file level? So to make those options only applicable when one uses the classic runtime through options (or by just using the default which is still the classic) and not changing the runtime at the file level (so when it stays to be classic). Did I get the intention right?

So what you are proposing is that whenever we use the automatic runtime we should ignore options like useSpread regardless of if the runtime has been chosen at the plugin or a file level? So to make those options only applicable when one uses the classic runtime through options (or by just using the default which is still the classic) and not changing the runtime at the file level (so when it stays to be classic). Did I get the intention right?

Yes, that's exactly what I'd like to see.

Andarist · 2020-11-10T17:11:05Z

@nicolo-ribaudo friendly 🏓

nicolo-ribaudo · 2020-11-22T13:46:07Z

packages/babel-preset-react/src/index.js

+          // TODO (Babel 8): Remove `useBuiltIns` & `useSpread`
+          useBuiltIns: !!opts.useBuiltIns,
+          useSpread:
+            "useSpread" in opts ? opts.useSpread : runtime !== "classic",


This can simply be opts.useSpread, unless I'm missing something: it defaults to undefined, which is falsy with the classic runtime and ignored with the automatic one.

packages/babel-helper-builder-react-jsx-experimental/src/index.js

jsg2021 · 2020-12-10T00:48:59Z

I'm now seeing this in my build logs... I'm not using those transforms directly. I'm just using the react preset.

Duplicate __self prop found. You are most likely using the deprecated transform-react-jsx-self Babel plugin. Both __source and __self are automatically set when using the automatic runtime. Please remove transform-react-jsx-source and transform-react-jsx-self from your Babel config.

nicolo-ribaudo · 2020-12-10T01:20:02Z

That is probably a regression caused by accidentally mixing some logic for the classic and automatic transforms. Could you open a new issue, with your full config and an example of input code that generates that message?

jsg2021 · 2020-12-10T01:23:40Z

This triggers it...

['@babel/preset-react', { development: true }]

if i set it false, its fine.

nicolo-ribaudo · 2020-12-10T09:17:53Z

babelrc: false only disable .babelrc loading. If you're ant to disable also the other config files, you'll need configFile: false.

Andarist · 2020-12-10T09:31:49Z

@jsg2021 could you paste in your whole Babel config? I expect that you have either loading @babel/preset-react twice or that you include @babel/plugin-transform-react-jsx-self somewhere

jsg2021 · 2020-12-10T11:09:35Z

my config was being included twice... I think this change made it obvious... I had been using babelrc: false incorrectly. I had a babel.config.js in my project root... and in my webpack and jest configs I pointed to my config as a preset, and attempting to disable inferred config lookup with babelrc: false... I see now I should be using configFile: false. Thanks!

jsg2021 · 2020-12-10T11:14:17Z

@Andarist I'm not sure how to dump my config... its spread across a few parts. 😅 I'm not seeing those errors anymore and I'm still getting the __self and __source props added... so I'm pretty confident I fixed my config. My config is reused across multiple projects in a react-scripts style package. So it took inspiration for over-specifying the config... dropping that and letting babel figure it out on its own fixed it.

nicolo-ribaudo · 2020-12-10T11:22:40Z

I'm not sure how to dump my config

You can do something like BABEL_SHOW_CONFIG_FOR=./a-file/containing-jsx.jsx npm run build (or with whatever command you are using to build your project).

Anyway, it you found that the problem was exactly that your config was loaded twice it's the error @Andarist is fixing in the PR 👍

billyjanitsch · 2020-12-11T22:39:40Z

This is an exciting change! 🥳 Unfortunately, it has at least some observable/undesirable side effects: emotion-js/emotion#2173.

Andarist · 2020-12-11T22:42:13Z

Just a note - I'm looking already into this but not sure how long this one will take me

Andarist added PR: Polish 💅 A type of pull request used for our changelog categories area: react area: jsx labels Oct 25, 2020

Andarist requested review from lunaruan and nicolo-ribaudo October 25, 2020 08:18

Andarist commented Oct 25, 2020

View reviewed changes

packages/babel-helper-builder-react-jsx-experimental/src/index.js Show resolved Hide resolved

Andarist commented Oct 25, 2020

View reviewed changes

Andarist force-pushed the drop-old-jsx-transform branch from d016154 to 48b4ab7 Compare October 25, 2020 13:57

nicolo-ribaudo reviewed Oct 27, 2020

View reviewed changes

Andarist force-pushed the drop-old-jsx-transform branch 2 times, most recently from 3abc2c0 to 8618ad5 Compare November 22, 2020 13:21

nicolo-ribaudo reviewed Nov 22, 2020

View reviewed changes

Andarist force-pushed the drop-old-jsx-transform branch 5 times, most recently from b5478e4 to 6483ba2 Compare November 22, 2020 22:52

Stop using old JSX transform

4b3c31a

Andarist force-pushed the drop-old-jsx-transform branch from 6483ba2 to 4b3c31a Compare November 22, 2020 23:09

nicolo-ribaudo approved these changes Nov 23, 2020

View reviewed changes

nicolo-ribaudo added PR: Internal 🏠 A type of pull request used for our changelog categories and removed PR: Polish 💅 A type of pull request used for our changelog categories labels Nov 23, 2020

Andarist requested a review from JLHwung November 23, 2020 08:38

JLHwung reviewed Nov 26, 2020

View reviewed changes

packages/babel-helper-builder-react-jsx-experimental/src/index.js Show resolved Hide resolved

Andarist requested review from JLHwung and existentialism December 8, 2020 18:00

JLHwung approved these changes Dec 8, 2020

View reviewed changes

existentialism approved these changes Dec 8, 2020

View reviewed changes

nicolo-ribaudo merged commit c2fcd69 into main Dec 8, 2020

nicolo-ribaudo deleted the drop-old-jsx-transform branch December 8, 2020 21:41

Andarist mentioned this pull request Dec 9, 2020

Replace the old JSX builder with the one that has been used experimentally #12465

Closed

This comment has been minimized.

Sign in to view

Andarist mentioned this pull request Dec 10, 2020

Fixed a regression for multiple __self & __source props with classic runtime #12475

Merged

billyjanitsch mentioned this pull request Dec 11, 2020

__self and __source no longer added in development mode using classic runtime emotion-js/emotion#2173

Closed

Andarist mentioned this pull request Dec 12, 2020

Fixed issue with skipping over self & source generated by another instance #12495

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 14, 2021

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using old JSX transform #12253

Stop using old JSX transform #12253

Andarist commented Oct 25, 2020 •

edited by gitpod-io bot

Loading

Andarist Oct 25, 2020

Andarist Oct 25, 2020

nicolo-ribaudo Oct 27, 2020

Andarist Oct 28, 2020

Andarist Oct 25, 2020

Andarist Oct 25, 2020

codesandbox-ci bot commented Oct 25, 2020 •

edited

Loading

babel-bot commented Oct 25, 2020 •

edited

Loading

kripod commented Oct 27, 2020

nicolo-ribaudo commented Oct 27, 2020

nicolo-ribaudo Oct 27, 2020

nicolo-ribaudo Oct 27, 2020

Andarist Oct 28, 2020

nicolo-ribaudo Nov 10, 2020 •

edited

Loading

Andarist Nov 10, 2020

nicolo-ribaudo Nov 17, 2020

Andarist commented Nov 10, 2020

nicolo-ribaudo Nov 22, 2020

jsg2021 commented Dec 10, 2020

nicolo-ribaudo commented Dec 10, 2020

jsg2021 commented Dec 10, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nicolo-ribaudo commented Dec 10, 2020

Andarist commented Dec 10, 2020

jsg2021 commented Dec 10, 2020

jsg2021 commented Dec 10, 2020

nicolo-ribaudo commented Dec 10, 2020 •

edited

Loading

billyjanitsch commented Dec 11, 2020

Andarist commented Dec 11, 2020

Stop using old JSX transform #12253

Stop using old JSX transform #12253

Conversation

Andarist commented Oct 25, 2020 • edited by gitpod-io bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesandbox-ci bot commented Oct 25, 2020 • edited Loading

babel-bot commented Oct 25, 2020 • edited Loading

kripod commented Oct 27, 2020

nicolo-ribaudo commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Nov 10, 2020

Choose a reason for hiding this comment

jsg2021 commented Dec 10, 2020

nicolo-ribaudo commented Dec 10, 2020

jsg2021 commented Dec 10, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nicolo-ribaudo commented Dec 10, 2020

Andarist commented Dec 10, 2020

jsg2021 commented Dec 10, 2020

jsg2021 commented Dec 10, 2020

nicolo-ribaudo commented Dec 10, 2020 • edited Loading

billyjanitsch commented Dec 11, 2020

Andarist commented Dec 11, 2020

Andarist commented Oct 25, 2020 •

edited by gitpod-io bot

Loading

codesandbox-ci bot commented Oct 25, 2020 •

edited

Loading

babel-bot commented Oct 25, 2020 •

edited

Loading

nicolo-ribaudo Nov 10, 2020 •

edited

Loading

nicolo-ribaudo commented Dec 10, 2020 •

edited

Loading