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

.mjs files inside CommonJS packages #5

Closed
GeoffreyBooth opened this issue Dec 5, 2018 · 7 comments
Closed

.mjs files inside CommonJS packages #5

GeoffreyBooth opened this issue Dec 5, 2018 · 7 comments

Comments

@GeoffreyBooth
Copy link
Owner

Migrated from #4 (comment) and following comments; and nodejs/ecmascript-modules#19 (comment) and following comments.

@guybedford: Having import 'cjs/subpath.mjs' not work seems overly restrictive to me, where users that have already been writing mjs might expect it to work within CommonJS packages.

@GeoffreyBooth
Copy link
Owner Author

Migrated from nodejs/ecmascript-modules#19 (comment):

My concern is that if this is allowed, there’s no certain way for Node to determine the parse goal(s) of a package. In the proposal as written, a package needs a package.json with an "exports" key (or some other ESM-signifying key we may later support like "mode") to be treated as ESM; otherwise it’s CommonJS, or if it has both "exports" and "main" it’s dual-mode. Node can therefore determine the parse goal(s) of the package overall from just the package.json. If we allow deep imports of .mjs files within a package where the package.json lacks any of these fields, then we have the odd result of a package that Node treats as CommonJS supplying ESM files. I feel like there’s a strong likelihood that this can cause issues. Again, however, the proposal doesn’t prohibit this in the future; by erroring now, we preserve the possibility of supporting import 'cjs/file.mjs' down the road.

@GeoffreyBooth
Copy link
Owner Author

@ljharb: The extension is how you determine the parse goal of a package. It would make no sense to have a CJS module in an .mjs file, so maybe I’m unclear on the issue.

The proposal doesn’t change the fact that .mjs always means a JavaScript file with an ESM parse goal.

Allowing .mjs files in CommonJS packages/package scopes to be loaded without error means that we would be permitting ESM-exporting packages that lack an "exports" field. I’m not sure that that’s something we want to allow. I think there’s a good argument that we might want to require users to explicitly define an ESM package’s exports, even if the exports are just "": "./" (i.e. everything), if only to let Node know that that package exports ESM without Node needing to scan all the files in the package searching for .mjs extensions.

From a UX perspective, one could argue that since .js files behave according to the package scope, being interpreted as CommonJS or ESM based on the nearest parent package.json, therefore .mjs files should do so as well. And since a CommonJS .mjs file is impossible, an .mjs file in a CommonJS scope should throw.

@ljharb
Copy link

ljharb commented Dec 5, 2018

Why wouldn’t we want to allow it? The file alone should be sufficient; imposing a forever tax on dual packages seems like a very bad idea.

@GeoffreyBooth
Copy link
Owner Author

People will want to write import foo from 'foo' and not import foo from 'foo/index.mjs', and so they’ll be adding "exports" to their foo package’s package.json. There are very few packages that won’t want to publish an entry point for the bare specifier, and therefore almost all ESM packages will have "exports" defined. It’s a tax that just about everyone will be paying, whether they’re publishing ESM-only or dual-mode.

We wouldn’t want to allow it for the reasons I’ve listed:

  • Node might need, or at least want, a way to determine the package scope (CommonJS, ESM, dual) without needing to scan all the files in a folder tree. Therefore we need "exports" or something like it in package.json.
  • It might be confusing UX for package scope to apply only to .js files and not other formats.

@ljharb
Copy link

ljharb commented Dec 5, 2018

People will also want to both import from 'foo' and from 'foo/bar', without an extension, like they do now.

I'm still not clear on where this "exports" field is necessary - if "main" is "index", then index.js is CJS and index.mjs is ESM.

@GeoffreyBooth
Copy link
Owner Author

This proposal is based on the package exports proposal, not on --experimental-modules. In the package exports proposal, you need to define "exports" for bare specifiers or “extensionless” exports to work. "main" is only used by CommonJS.

@jkrems
Copy link
Collaborator

jkrems commented Dec 5, 2018

Since we want to encourage browser-compatible code (import './foo' will not magically append an extension in pretty much any setup out-of-the-box), it feels awkward to suddenly start implicitly appending extensions iff the URL path is coming from a bare specifier.

The exports proposal does allow import 'foo/bar' via configuration instead of implicitly via disk layout of the package.

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

3 participants