Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Proposal: Allow CommonJS modules to declare ESM named exports #448

Closed
coreyfarrell opened this issue Nov 27, 2019 · 18 comments
Closed

Proposal: Allow CommonJS modules to declare ESM named exports #448

coreyfarrell opened this issue Nov 27, 2019 · 18 comments

Comments

@coreyfarrell
Copy link
Member

Goal

Allow module authors to support const {named1, named2} = require('pkg1') and import {named1, named2} from 'pkg1' in the same version of pkg1.

Proposed implementation

When CommonJS is loaded by the ESM loader it would use const esmExports = module.esmExports || {default: module.exports}; to determine the intended export structure. The result of Object.entries(esmExports) would be iterated to set exports on the ES module.

Usage Examples

Named exports only

module.exports.named1 = 'value1';
module.exports.named2 = 'value2';
module.esmExports = module.exports;

Default function with named exports

function defaultFn(...args) {}
defaultFn.namedExport = 'const value';

module.exports = defaultFn;
module.esmExports = {
  default: (...args) => defaultFn(...args),
  ...module.exports
);

With API compatibility

module.esmExports = {
  ...module.exports,
  default: module.exports
);

This compatibility code would ensure import pkg from 'pkg' will provide the same result in 13.2.0 as it will in a future version supporting esmExports, so named ESM exports can be added to a package without a semver-major release.

@babel/plugin-transform-modules-commonjs

// ./dist/index.js is generated by babel
module.exports = module.esmExports = require('./dist/index.js');

This shows how to produce correct ESM exports using output of ESM code translated by babel. This does not address API compatibility so it would potentially be a semver-major release. This example does not show fix-ups to the CJS default export, so CJS users would still have to use require('pkg').default to access the default export.

Why not interpret __esModule

The meaning of import cjsDefault from 'cjs-module' is already established. Supporting the babel __esModule attribute now would result in a breaking change to existing modules, authors would be unable to control for this. If this is implemented I think we should ask @babel/plugin-transform-modules-commonjs if they're willing to add an option to have the transformed code set module.esmExports in addition to module.exports.

@ljharb
Copy link
Member

ljharb commented Nov 27, 2019

so esmExports would contain the actual values of the exports, trumping module.exports? Would this allow cjs and ESM consumers to see a different default value?

@GeoffreyBooth
Copy link
Member

When CommonJS is loaded by the ESM loader it would use const esmExports = module.esmExports || {default: module.exports}; to determine the intended export structure.

We can’t do this, because this would mean the CommonJS code gets evaluated twice: once in the loader, and again when the program itself is executed. If the code has any side effects (like it deletes a file, for example, or prints to the console) these actions would happen twice.

The lack of support for named exports in CommonJS is not for lack of effort. Many, many proposals have been floated, but most have this issue: since there’s no way to statically analyze CommonJS, there’s no way to know the exports without evaluating the code; but we can’t evaluate the code without breaking spec or causing issues like the above. See https:/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md#phase-3-path-to-stability-removing---experimental-modules-flag section “Finalize behavior of import of CommonJS files and packages.”

The only proposal that’s still potentially viable, in that it doesn’t violate spec or execute code twice, is #324, which suggests putting the named exports in package.json. But we decided that if conditional exports are possible, there wouldn’t be a need for a special additional package.json field for CommonJS named exports since package authors can create an ESM wrapper for an otherwise CommonJS package.

@MylesBorins
Copy link
Contributor

There was work done at TC39 to try and support dynamic modules... Which would allow us to defer the loading of named exports until execution time but we were unable to reach consensus within this group or tc39 on semantics

#252

A new object doesn't help solve the problem unfortunately

@coreyfarrell
Copy link
Member Author

Would this allow cjs and ESM consumers to see a different default value?

In theory though you'd probably want the two to be related.

@coreyfarrell
Copy link
Member Author

When CommonJS is loaded by the ESM loader it would use const esmExports = module.esmExports || {default: module.exports}; to determine the intended export structure.

We can’t do this, because this would mean the CommonJS code gets evaluated twice: once in the loader, and again when the program itself is executed. If the code has any side effects (like it deletes a file, for example, or prints to the console) these actions would happen twice.

I don't understand how this would cause dual execution. I'm not proposing any changes to when the module would be executed. Once CJS completes the load it would call ModuleWrap#setExport to add the exports.

The lack of support for named exports in CommonJS is not for lack of effort. Many, many proposals have been floated, but most have this issue: since there’s no way to statically analyze CommonJS, there’s no way to know the exports without evaluating the code; but we can’t evaluate the code without breaking spec or causing issues like the above. See /doc/plan-for-new-modules-implementation.md@master#phase-3-path-to-stability-removing---experimental-modules-flag section “Finalize behavior of import of CommonJS files and packages.”

The only proposal that’s still potentially viable, in that it doesn’t violate spec or execute code twice, is #324, which suggests putting the named exports in package.json. But we decided that if conditional exports are possible, there wouldn’t be a need for a special additional package.json field for CommonJS named exports since package authors can create an ESM wrapper for an otherwise CommonJS package.

Conditional exports could be an alternative but the idea seems to have enormous resistance. I'm honestly surprised it was implemented even behind a flag.

@coreyfarrell
Copy link
Member Author

There was work done at TC39 to try and support dynamic modules...

I'm not proposing dynamic modules. CJS loads synchronously and module.esmExports would need to be set synchronously. Changes after the fact would be ignored.

@GeoffreyBooth
Copy link
Member

I don't understand how this would cause dual execution. I'm not proposing any changes to when the module would be executed. Once CJS completes the load it would call ModuleWrap#setExport to add the exports.

The ESM loader loads the graph of modules in an earlier phase before any code execution is done. That’s part of the spec. That’s why imports can only accept plain strings and not variables, and therefore are statically analyzable. See discussion in #264 and nodejs/ecmascript-modules#31. Assuming your esmExports object is defined within the code of your CommonJS files, it won’t be evaluated until after the ESM loader has already determined all the files and named exports.

I'm not proposing dynamic modules.

I think the assumption is that since you say you want to avoid dual execution, the ESM loader wouldn’t evaluate any CommonJS code to determine its named exports, but rather rely on the names in import statements to deduce what those exports should be; and hopefully those names actually are exported when the CommonJS code is eventually evaluated. That’s pretty similar (if not the same approach) as the dynamic modules proposal, if I remember it correctly.

@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 27, 2019 via email

@addaleax
Copy link
Member

Just an idea, but could we maybe introduce a standardized comment format for CJS that provides the shape of the module when imported from ESM? For example – and this is just one of many possible implementations – look for a comment starting with ES exports: in the file, followed by a JSON-formatted array of strings, then use that to provide the module shape?

// ES exports: ['foo', 'bar']

// ...

module.exports = {
  foo, bar
}

I think similar ideas have been floated in the past, like automatically generated wrapper modules, but I feel like this might be a bit more ergonomical?

One option is to execute CJS during the link phase... But that would cause
side effects

Fwiw, I’m still generally a fan of this idea too. Side effects are a small price to pay for larger ecosystem compatibility.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Nov 27, 2019

look for a comment starting with ES exports: in the file, followed by a JSON-formatted array of strings, then use that to provide the module shape?

That’s essentially the same idea as #324, just in a comment in source code rather than in package.json.

I think the question is really, assuming conditional exports get unflagged and released (and @coreyfarrell to the contrary, the current status quo is that they will be released in January unless a better alternative reaches consensus), is some special comment- or package.json-based syntax better than an ESM wrapper file?

Edit: To be clear, what I mean is, all three solutions (comment, package.json, ESM wrapper file) require effort on the part of the package author. The wrapper will definitely be possible, since we’re committed to shipping dual packages. So the question is why we should additionally support one of the alternatives; what does it offer more than an ESM wrapper? Containing within a file is a benefit, sure, but is it worth Node now treating certain comments as special syntax? Is there any precedent for that?

I think where the group landed was that the goal was really automatic named exports from CommonJS; and failing that, if any effort was going to be required on the author’s part, the ESM wrapper would probably be good enough.

@coreyfarrell
Copy link
Member Author

One option is to execute CJS during the link phase... But that would cause
side effects

https:/nodejs/node/blob/master/lib/internal/modules/esm/translators.js#L111-L117

Moving the CJSModule._load call before new ModuleWrap would cause CJS to load during the link phase? If I understand correctly my proposal would cause node.js to incorrectly execute commonjs-module before real-es-module in the following code:

import {sync as pkg1} from 'real-es-module';
import {sync as pkg2} from 'commonjs-module';

@addaleax
Copy link
Member

If I understand correctly my proposal would cause node.js to incorrectly execute commonjs-module before real-es-module in the following code:

That is my understanding as well, yes.

@coreyfarrell
Copy link
Member Author

Thanks everyone for considering this and for explaining to me why it's not viable.

@weswigham
Copy link
Contributor

It's not that it's not viable, per sey, it's that it needs spec work to happen. Many of us would like it to happen, we just can't put in the time at tc39 to make it happen ourselves.

@guybedford
Copy link
Contributor

guybedford commented Nov 28, 2019

This what we couldn't build consensus on as it could not support export *

For reference, it could support export *, just not in very specific cases of export * used alongside cycles where an error would need to be thrown. Always throwing on export * was added based on TC39 feedback due to the complexity of the back-referencing system.

@GeoffreyBooth
Copy link
Member

@coreyfarrell I honestly appreciate the effort, even if all it achieves is making us review our own history and justify our decisions and explain our goals. There’s value in that.

There is consensus that we’d love to see named exports from CommonJS if there’s a way to make that happen. At the moment the best option is the ESM wrapper via conditional exports; but if you can find another way, that both doesn’t violate spec and ideally is automatic (requires no effort on the CommonJS author’s part), such a solution would be met with enthusiasm.

@Jamesernator
Copy link

Is there a reason a directive (determinable at parse time) wouldn't work?

"export one";
"export two";

module.exports = function fn() {

}
module.exports.one = 1;
module.exports.two = 2;

@GeoffreyBooth
Copy link
Member

Is there a reason a directive (determinable at parse time) wouldn't work?

It would work, just as the suggestion to define them via comments would work; but either solution requires explicit effort on the part of the CommonJS author. If the author is going to have to do anything, they might as well write an ESM wrapper file that has the benefit of being a spec rather than a pragma/directive or comment that is a Node invention.

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

No branches or pull requests

8 participants