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

Node-style default interop #10971

Closed
guybedford opened this issue May 29, 2020 · 4 comments
Closed

Node-style default interop #10971

guybedford opened this issue May 29, 2020 · 4 comments

Comments

@guybedford
Copy link
Contributor

guybedford commented May 29, 2020

Feature request

Currently Webpack supports some of the following interops:

const pkg = require('pkg');

where pkg has exports.__esModule and exports.default, will import the pkg.default property instead of returning the pkg property directly. This is in contrast to Node.js behaviour.

In addition:

import pkg from 'pkg'

should import the pkg property forcing users to do .default.default in this case (and not checking __esModule interop, as Node.js doesn't do this).

Named exports are still up in the air for Node.js but would be additions to the above if this happens.

I would suggest still supporting named exports from the instance, but ensuring the default export interop still behaves as above.

What is the expected behavior?

Correct Node.js interop semantics would be for:

Object.defineProperty(exports, '__esModule', { value: true });
exports.name = 'name';
exports.default = 'default';

to be represented by the namespace object ModuleNamespace { default: exports }, such that:

import * as ns from 'cjs';
import exports from 'cjs';
// ns === { default: exports }
// exports.default === 'default'

Also when importing from one CJS module to another, the namespace default export should be used.

Node.js semantics do not govern how CJS should import ESM, so getting the full ES module namespace as the require() output does seem fine in this case.

What is motivation or use case for adding/changing the behavior?

Ensuring smooth ecosystem behaviours between Node.js, build tools and browsers.

How should this be implemented in your opinion?

A simple flag should suffice. I do think it should be the default mode eventually.

Are you willing to work on this yourself?

If there are dev bandwidth constraints for such a feature, I can certainly see if I can find wider support.

@robpalme
Copy link

Just FYI - this is effectively asking for a webpack mode or flag to disable support for faux modules.

faux modules is an emerging term for published modules that were transpiled to CJS + "__esModule" tags. It is meant to be a snappy phrase to help discuss the problem space and not intended to be disparaging.

@sokra
Copy link
Member

sokra commented May 30, 2020

I won't call them such way. Flagged modules existed long before node started to think about esm support. They are a valid module format in the middle between cjs and esm. Even while there is no official spec for that, in this module format exports.default is the value of the default export when imported via esm. In fact this is widely supported by multiple tools and only node.js missed to add proper support for them.

webpack has a special mode in .mjs (and type: module in future) where this flag is ignored, but I'm unhappy with the complexity and work created because node.js varys from the ecosystem standards. Same for the module field in package.json.

After all this will be behind an experiments flag in webpack 5 as it's still experimental in node.js.

@guybedford
Copy link
Contributor Author

This issue is specifically not about breaking any semantics for named exports of CJS in ESM.

This issue is only about tracking that import pkg from 'cjs' will always represent the module.exports value, and that require('pkg') in CJS will also always represent that value, without checking __esModule checks.

The default export syntax sugar and proposals were originally designed exactly for this purpose of Node.js interoperability. If you don't believe me I would be glad to find the original design notes and TC39 meeting notes to share from 5 years ago!

This issue is not about strictNode flag or breaking things for users - it is simply about ensuring that the default interop works as expected with Node.js. Named exports, extensions, type checking are all separate restrictions IMO.

@alexander-akait
Copy link
Member

Close in favor #7973

Ashoat added a commit to CommE2E/comm that referenced this issue Feb 13, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants