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.

Sign up for GitHub

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

Jump to bottom

Better interop between native Node.js modules and compiled modules #12363

Open
1 task
nicolo-ribaudo opened this issue Nov 16, 2020 · 6 comments
Open
1 task

Better interop between native Node.js modules and compiled modules #12363

nicolo-ribaudo opened this issue Nov 16, 2020 · 6 comments
Labels
i: discussion
Milestone
v8.0.0

Comments

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 16, 2020 •
edited
Loading

Feature Request

  • I would like to work on this feature!

Is your feature request related to a problem?

A Babel-compiled module with a default export cannot be used in the same whey in a Babel-compiled module and in a native Node.js module.

Consider this library:

export default function getOne() {
  return 1;
}

When compiled by Babel, it becomes something like

"use strict";
exports.__esModule = true;
exports.default = function getOne() {
  return 1;
};

Now, let's suppose that a user of that library wants to use it in a Babel-compiled app. The have to write something like this:

import getOne from "library";

console.log(getOne());

However, if that user wants to use native Node.js modules, they'll get an error similar to

TypeError: { default: [function getOne] } is not a function.

They might try to rewrite that code as

import { default as getOne } from "library";

console.log(getOne());

but that still doesn't work, because import { default as X } is just an alias for import X.

What they need to write is

import _getOne from "library";
const getOne = _getOne.default;

console.log(getOne());

This code is ugly but works on Node.js. However, we now have a problem: if the file importing library is bundled by an __esModule-aware tool, for example because it's meant to be used both in Node.js and in the browser, it will generate an error similar to

TypeError: undefined is not a function.

because _getOne is the function, and thus _getOne.default is undefined.

The proper way of using a Babel-compiled library is this:

import _getOne from "library";
const getOne = _getOne.default ?? _getOne;

console.log(getOne());

This works but it's counter-intuitive, and it puts the burden on the library users rather than on the library author or (even better) on Babel.

Note that named exports don't have this problem

Context

This issue is a result of discussion in different repositories, where other tools are trying to figure out Node.js interop now that native ES modules are becoming common:

(cc @guybedford)

History

Back in version 5, when there was only a default export Babel used to output something like this:

exports.default = function getOne() {
  return 1;
};
module.exports = exports.default;

However, this was a big refactoring hazard (#2047): just by adding an export const x = 1 line to our "libary", the output would became

exports.default = function getOne() {
  return 1;
};
exports.x = 1;

which is a breaking change, because require("library") doesn't return getOne() anymore. For this reason, this automatic interop was removed in Babel 6 (#2212).

Describe the solution you'd like

We can add an option to @babel/plugin-transform-modules-commonjs to allow compiling export default to module.exports.
This option (moduleExportsDefault?) would throw if

  1. There is any named export in the file, or
  2. the export is export { foo as default } (because module.exports cannot reflect foo's liveness), or
  3. the export is export default class/function X and X is reassigned (for the same reason as above).

With these rules, it's impossible to accidentally introducing a breaking change in your library just by refactoring the exports, since Babel will explicitly throw about it. Thus, library authors will be encouraged to only use this option for their entry-point and not for all the internal files (where they may want to freely refactor things):

// babel.config.json
{
  "overrides": [{
    "include: "src/index.js",
    "plugins": [
      ["@babel/transform-modules-commonjs", { "moduleExportsDefault": true }]
    ]
  }, {
    "exclude": "src/index.js",
    "plugins": ["@babel/transform-modules-commonjs"]
  }]
}

Describe alternatives you've considered.

  1. We can decide that the current situation is OK. CJS modules will hopefully go away in a few years (Node.js 10, the last LTS version without ESM support, reached EOL in a few months).

  2. We can start recommending https://github.com/59naga/babel-plugin-add-module-exports by @59naga more prominently in our docs, maybe even publishing it under the @babel/* namespace. However, this plugin replicates the exact Babel 5 behavior with the refactoring hazard.
@Andarist
Copy link
Member

Apart from what Babel should do here, there is also an issue of the overall DX of the whole situation. I believe it's just terrible for both consumers and library authors. The problem itself isn't only about a refactoring hazard but it also suddenly forces library authors to alter their public interfaces because - let's face it - we usually want to support node. This is hostile to both ends of the story because both authors and consumers need to alter their code.

I've been thinking about this a lot over the past year, watching how things develop, and this saddens me a lot. I understand node's compatibility and correctness concerns but it's IMHO really not a good situation that somebody so late to the party (node) is influencing things so greatly, leaving a lot of package authors/maintainers with so uneasy decisions on how to handle things like this.

Sure, CJS involvement will slowly die over the years but I highly doubt that it will get to a near-zero point in just a couple of years. Both a lot of packages will still be out there and also some packages will refuse to "upgrade" for a considerable amount of time. I've been advocating for adding pkg.json#module field to a lot of packages over the years and not always I had succeeded. People are afraid of those subtle changes. Sure - node introducing well-documented support for ESM should make it easier now but given how many tools and environments are out there it's still not an obvious decision to implement the exports field.

On top of that people will just make a lot of mistakes when implementing exports because they won't realize all intricacies up until it's too late and issues start pouring into their repos which will in a lot of situations lead only to people reverting a change and being afraid of attempting to reintroduce this.

Attempting to deal with this in Babel feels a lot like a small bandaid for a bad wound to me. You will also have to pick the default of an option like this, document it, deal with issues. I don't feel it's something that Babel maintainers, nor their users should be troubled with.

@jaydenseric
Copy link

jaydenseric commented Dec 3, 2020 •
edited
Loading

The only safe transpilation for ESM to CJS is export default -> module.exports =. By default, Babel should only facilitate this, and error if any named exports are encountered.

When Babel transpiles CJS to ESM, it should allow the exports object to be created then do export default exports, so in Node.js importing the CJS file will be identical to importing its ESM transpiled version.

IMO at this time there is not a good reason to transpile one module format to another. If you want to publish or deploy a CJS package, write CJS from the beginning. If you want to publish or deploy ESM, write ESM.

Writing ESM, then attempting to published it transpiled to CJS is a bad idea. Writing ESM, and attempting to publishing ESM with copies of the same API in separate CJS files is an even worse idea, due to the dual package hazard and the bloated node_modules install size.

The right thing to do is to write the individual bits of your API as CJS files with single module.exports =, e.g:

https://github.com/jaydenseric/graphql-upload/blob/1398e62a53ad8075d6d4fc0692e326ceef224ca0/public/GraphQLUpload.js#L77

Make sure all the things required in these files are done using deep require paths to modules containing only module.exports =, so only necessary code gets loaded or bundled. Bundler tree-shaking is a hack we don’t need if code is written correctly by only importing or requiring what’s needed. For an efficient and healthy ecosystem all packages need to start publishing separate files for each part of their API so they can be deeply required/imported.

Then, use the package exports field with conditional exports to publicly expose index entry points for both CJS and ESM consumers, using separate and manually created index.js (CJS) and index.mjs (ESM) files, e.g:

https://github.com/jaydenseric/graphql-upload/blob/bdb5f808e0d514c28c0f59c1abd71680aba29bae/package.json#L36-L39

Index files should only be used by tools such as bundlephobia and Size Limit; actual source code should always use deep import/require paths. Why would we rely on hacky and slow tree shaking when we can just write efficient code from the start!

The lowest hanging Babel fruit right now for Node.js ESM/CJS style interop is #6242 : The sourceType should automatically be script if the input file has a .cjs or .js file extension, and module for .mjs. No one realizes they need to do this manually, like this (I left .cjs out of the test because I know there are no files like that in this particular project):

https://github.com/jaydenseric/graphql-react/blob/d53cb2f0708a5afe403f52fc88bf1fd46a197c08/src/universal/.babelrc.json#L17-L18

It should also factor in the package.json type field if the input file extension is .js.

For housekeeping here are some issues on the same topic (dating back to the Node.js --experimental-modules days), that may now be duplicating this issue:

@Andarist
Copy link
Member

Andarist commented Dec 3, 2020

Writing ESM, then attempting to published it transpiled to CJS is a bad idea. Writing ESM, and attempting to publishing ESM with copies of the same API in separate CJS files is an even worse idea

This is much easier said than done - Babel has to support the existing ecosystem and the ecosystem has been publishing "dual mode" packages (without the dual package hazard!) for a long time. We could change the tide but this is kinda up to package authors and not Babel itself (from what I believe). This has a severe ecosystem impact though as it will often influence package setups, public interfaces, documentation and so on.

@dgoldstein0
Copy link

question: doesn't nodejs/node#35249 solve this? for users of newer nodejs versions, at least

@nicolo-ribaudo
Copy link
Member Author

No, it only solves the named exports problems (not the "default export" one).

@dgoldstein0
Copy link

dgoldstein0 commented Mar 2, 2021 via email

@liuxingbaoyu liuxingbaoyu added this to the Babel 8.0 milestone Sep 1, 2022
@devversion devversion mentioned this issue Dec 13, 2022
Closed
1 task
@chrisirhc chrisirhc mentioned this issue Dec 18, 2022
Closed
Ashoat added a commit to CommE2E/comm that referenced this issue Feb 13, 2023
@Ashoat
Make react-icomoon work with Webpack 5
08eb701 
Summary:
`react-icomoon` has a quirky CommonJS export with a hack meant to make it work when imported from an ES Modules context. The idea was that would set `exports.__esModule` to enable the hack, and you would export a prop named `default` that would be the "default" import for the ESM.

This hack was supported in Webpack 4, but not in Webpack 5. There's a lot of [noise](webpack/webpack#10971) about [this](webpack/webpack#7973) on [GitHub](babel/babel#12363), but the Webpack team points to Node as no longer supported this hack either as justification for pulling it out.

The `react-icomoon` package uses this hack. I tried upgrading it, but the latest version still had the same problem. To get around it, I just update the code to access `.default` directly.

Note that this means `lib/components/SWMansionIcon.react.js` wouldn't work in a React Native context, despite `react-icomoon` claiming to support React Native. But we don't need it on React Native since we use `createIconSetFromIcoMoon` from `@expo/vector-icons` instead.

Depends on D6703

Test Plan:
1. I tested dev build in `web` via `yarn dev` in `web` + `keyserver` and then tested the website
2. I tested prod build by running `yarn prod` in `web` and then tested the website by running keyserver with `yarn prod-build && yarn-prod`

Reviewers: atul, tomek

Reviewed By: atul

Differential Revision: https://phab.comm.dev/D6704
@joyeecheung joyeecheung mentioned this issue Jul 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
i: discussion
Projects
None yet
Milestone
v8.0.0
Development

No branches or pull requests
5 participants
@dgoldstein0 @jaydenseric @nicolo-ribaudo @Andarist @liuxingbaoyu