JSX runtime appends .js to importSource leading to broken import paths

[marvinhagemeister]

Bug Report

• I would like to work on a fix!

Current behavior

For some reason an extension is appended to the end of the import source string. This is very unexpected as it breaks module resolution in node. It doesn't play well with the recently established export map feature. One of our Preact users traced the error back to this commit: d6b0822

Here is a full repro case: https://github.com/marvinhagemeister/babel-jsx-runtime-bug

Input Code

export const foo = <div />;

Expected behavior

I expected babel to not add a .js extension to the import path. It breaks node's export map feature. With export map's the package.json of the consuming package will look something like this:

{
  "name": "my-package",
  "exports": {
    "jsx-runtime": {
      "require": "./path/to/browser.jsx-runtime.js",
      "import": "./path/to/import.jsx-runtime.mjs"
    },
    "jsx-dev-runtime": {
      "require": "./path/to/browser.jsx-dev-runtime.js",
      "import": "./path/to/import.jsx-dev-runtime.mjs"
    }
  }
}

This worked fine with recent versions of babel before the mentioned commit was added.

Babel Configuration (babel.config.js, .babelrc, package.json#babel, cli command, .eslintrc)

• Filename: babel.config.js

// babel.config.js
module.exports = {
  plugins: [
    [
      "@babel/plugin-transform-react-jsx",
      { runtime: "automatic", importSource: "preact" }
    ]
  ]
}

Output:

import { jsx as _jsx } from "preact/jsx-runtime";
export const foo = _jsx("div", {});

Environment

  System:
    OS: Linux 5.8 Arch Linux
  Binaries:
    Node: 14.13.0 - ~/.nvm/versions/node/v14.13.0/bin/node
    Yarn: 1.22.10 - /usr/bin/yarn
    npm: 6.14.8 - ~/.nvm/versions/node/v14.13.0/bin/npm
  npmPackages:
    @babel/cli: ^7.12.1 => 7.12.1 
    @babel/core: ^7.12.3 => 7.12.3 
    @babel/plugin-transform-react-jsx: ^7.12.1 => 7.12.1 

Possible Solution

Revert this commit:
d6b0822

Additional context

preactjs/preact#2801

[babel-bot]

Hey @marvinhagemeister! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

[marvinhagemeister]

Idea: what if instead of doing all that magic that the plugin is doing now we could let users specify the full import specifier: preact -> preact/jsx.-runtime.

That way everybody could set it to whatever they need. This would avoid the weird scenarios we are in right now and would drop a bit of code from babel's repo. Would be easier to maintain too.

I think in general having babel do module resolution is a slippery slope. There are too many configurations, tools and environments to take care of and with custom user resolvers on top.

What do you think?

[nicolo-ribaudo]

This is a tough one...

• Our default action should be reverting that commit, since it effectively introduced a regression breaking a contract established in Babel 7.9.0. We can do this now but we need to release a proper solution quickly (@JLHwung @existentialism @hzoo wdyt?)

• If we revert that commit, React will stop working with Webpack 5 because React doesn't specify "exports" in its package.json and Webpack 5 doesn't resolve implicit extensions; (https://github.com/nicolo-ribaudo/test-react-webpack-5)

• If we just revert that commit, Babel's output won't work in browsers unless you use a bundler.

Idea: what if instead of doing all that magic that the plugin is doing now we could let users specify the full import specifier: preact -> preact/jsx-runtime.

Since there are multiple entry points, it would look more like this:

"importSource": {
  "jsx-runtime": "preact/jsx-runtime", // for jsx
  "jsx-dev-runtime": "preact/jsx-dev-runtime", // for jsxDEV
  "/": "preact", // for createElement
}

However, I don't know how it would look like when set using the inline comment pragma.

Hidden because I don't think that it can work with the "root" import for `createElement`

WDYT about something like `"importSource": "preact/[module]"`, and we do `importSource.replace("[module]", "jsx-runtime")` (or `"jsx-dev-runtime"`) in the plugin?

If [module] is not present in the importSource, than we do importSource += "/[module]" to match the Babel 7.9.0 behavior, and we update the default to "react/[module].js" so that React works.

(cc @lunaruan / @gaearon / @sebmarkbage )

[nicolo-ribaudo]

The ideal solution would be that we revert the commit that introduced the regression, and React adds "exports" to package.json to make the extension optional.

If someone really needs to further customize the import path (for example, because using a CDN that doesn't resolve the exports field), they can use https://www.npmjs.com/package/babel-plugin-module-resolver.

[nicolo-ribaudo]

Oh, React + webpack 5 is not a concern because:

1. If "type": "module" is not specified in package.json, webpack still automatically resolves imports
2. If someone specifies "type": "module", it doesn't work anyway (not even specifying the .js extension):

./src/main.js 95 bytes [built] [code generated]

ERROR in ./src/main.js 2:30-34
Can't import the named export 'jsx' (imported as '_jsx') from default-exporting module (only default export is available)

webpack 5.1.3 compiled with 1 error in 392 ms

[nicolo-ribaudo]

Oh and for the original issue (facebook/react#19905), it seems that some CDNs correctly support extension-less imports if they are declared as "exports" in package.json: try running await import("https://cdn.skypack.dev/preact/jsx-runtime") in your browser's console.

[marvinhagemeister]

@nicolas-marien Thanks for the quick response! Using the export field avoids some of the complexities around resolution. All modern CDNs that I know of support it and will resolve it to an extension. Strange about webpack 5 + type "module", I would've expected it to work regardless.

cc @sokra

[JLHwung]

I am good to revert d6b0822 but we should coordinate with the React team. As for react team, I suggest specifying "exports" and those two runtime:

{
  "exports": {
    "./jsx-runtime": "./jsx-runtime.js",
    "./jsx-dev-runtime": "./jsx-dev-runtime.js",
    "./": "./"
  }
}

So both require("react/jsx-runtime") and require("react/jsx-runtime.js") will work. I lean to discourage the usage of explicit extension as in the future we may have react/jsx-runtime.cjs or react/jsx-runtime.mjs but it is impossible to point ./jsx-runtime.js to ./jsx-runtime.mjs in exports.

[nicolo-ribaudo]

Fixed in @babel/helper-builder-react-jsx-experimental@7.12.4

[daniele-orlando]

It is fine to revert, but I don't think it solves the problem. We can't just ask any package maintainer out there to update their package.json, and some legacy version of these packages could never be updated.
Maybe one day the React team will add the exports map to their package.json, but what about until then? And for the "legacy" versions?

Anyway, I agree with @marvinhagemeister

Idea: what if instead of doing all that magic that the plugin is doing now we could let users specify the full import specifier: preact -> preact/jsx.-runtime.

That way everybody could set it to whatever they need. This would avoid the weird scenarios we are in right now and would drop a bit of code from babel's repo. Would be easier to maintain too.

I think in general having babel do module resolution is a slippery slope. There are too many configurations, tools and environments to take care of and with custom user resolvers on top.

I think it is important to take the right path on this issue, because the TypeScript team is matching the Babel behaviour and wrong assumptions for the sake of "auto-magic-features" damage the entire ecosystem.

For reference, same issue on the TypeScript project:
microsoft/TypeScript#41623

[Andarist]

@daniele-orlando could u describe in more detail what problem do u see here? The inserted import statements allow the standard node’s resolution algorithm to resolve final paths through different means - trying the .js extension, trying a directory with nested package.json and exports map of the root package.json. Its already pretty flexible. Its not browser compatible “as is” but nor import 'react' is.

[nicolo-ribaudo]

This is being also discussed in the React repository: facebook/react#20235.

I would be fine with both the conclusion (extension or not), but:

1. Since we know that changing it breaks things, it will have to wait for Babel 8.
2. This decision should really be coordinated between the different tools. The current Babel implementation relfects the original React RFC, but React itself doesn't "follow" it. (follow = be compatible according to the current Node.js resolution algorithm)

[daniele-orlando]

@Andarist As said

We can't just ask any package maintainer out there to update their package.json (to include the exports map), and some legacy versions of these packages could never be updated.

And Webpack (v5) follows strict rules on resolving imports, which means that a missing '.js' breaks Webpack. Of course we could use hacks like remapping inside Webpack.config.js react/jsx-runtime to react/jsx-runtime.js, but this is an hack and it's just shifting the problem.

Please check these for more details.
microsoft/TypeScript#41623
facebook/react#20235

[JLHwung]

Note that React already added exports maps in facebook/react#20304, so I expect this issue will be fixed in its next release.

some legacy version of these packages could never be updated.

Indeed. But type: "module", webpack 5 and jsx-runtime... these are new stuffs. I don't expect legacy versions will work with them.

[Andarist]

Ok, thank you for the explanation - the gist of it is that exports map are required for ESM in node to support extensionless imports and webpack matches their behavior (only for type: "module" and .mjs).

As mentioned here and in other issues - adding extensions in the emitted code is being problematic for a couple of reasons and I believe that it should be avoided. I understand that it's unfortunate that things break right now because of it - but node's semantics are very new. The ESM support in node has been released just this month - so it's understandable that some packages are not yet ready in full for it.

Indeed. But type: "module", webpack 5 and jsx-runtime... these are new stuffs. I don't expect legacy versions will work with them.

I partially agree with that. It was already a great gesture from the React team to ship runtimes for all~ React versions. I suppose they could add exports map in a similar fashion to all of them, just to avoid confusion etc. It makes sense given how many users they have.

As to other packages - jsx-runtime is indeed very new so I don't really see how old packages would be affected by this at all? They will just use the classic runtime after all which doesn't even emit any implicit imports at all so you have freedom to write them in any way that you see fit, considering your environment etc.

As to the current webpack's situation - just don't use .mjs for now. If you rename your file to .js then webpack will gladly resolve the react/jsx-runtime entrypoint.

[sebmarkbage]

We weren’t sure if we could include the exports field safely in 17 because it’s so complicated to predict all possible outcomes.

But it seems plausible that we could just publish a minor with it.

[Andarist]

Yeah, super hard to predict everything but if u stick to CJS only then things get easier significantly - most issues come from trying to support both ESM and CJS in a single package. Your setup looks safe-ish but Ive already seen so many things breaking in relation to node’s ESM support (i consider exports field to be a part of that) that i wouldnt bet any money on that 😅

It still seems worth trying though

[nicolo-ribaudo]

@sebmarkbage We got some experience by adding "exports", breaking things, and then fixing it 😅

If you'll want a review, feel free to ping me.

[Andarist]

Same thing here - feel ping me any time regarding this craziness