Skip to content
This repository has been archived by the owner on Apr 16, 2020. It is now read-only.

Exports main #41

Closed
wants to merge 13 commits into from
Closed

Conversation

guybedford
Copy link
Contributor

This supports the package.json "exports" main to allow dual mode workflows, as described in nodejs/modules#273.

In addition some minor refinements around file extension handling are provided as well, specifically disabling support for .node and .json mains in CommonJS packages when loaded from ESM.

@MylesBorins
Copy link
Contributor

@guybedford can you update the commits to have a namespace e.g. esm:

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per comment on the issue; without extension lookup i don’t think we should merge this.

@GeoffreyBooth
Copy link
Member

Per comment on the issue; without extension lookup i don’t think we should merge this.

Per the issue discussion, this PR doesn’t involve extension lookup. Even if extension lookup is added, this PR would still apply.

@ljharb
Copy link
Member

ljharb commented Feb 26, 2019

I disagree; with extension lookup, there's no need for an additional field at all, since the file extension would determine parsing goal (and a separate field to override parse goal per extension would continue to be useful).

@GeoffreyBooth
Copy link
Member

I disagree; with extension lookup, there’s no need for an additional field at all, since the file extension would determine parsing goal (and a separate field to override parse goal per extension would continue to be useful).

As was noted in that thread, a single field only works if the two files are side-by-side differing only in extension, e.g. ./index.mjs/./index.js. A very common, if not predominant, developer workflow is to have ESM code in a folder like src and the transpiled CommonJS code in a separate folder like dist. Therefore there would be separate entry points like ./src/index.mjs and ./dist/index.js that are impossible to reference in the extension-searching approach. Note also that this point still applies even if one uses separate extensions for their ESM and CommonJS code.

@ljharb
Copy link
Member

ljharb commented Feb 26, 2019

@GeoffreyBooth in that model, you'd transpile/copy two files, and have dist/index.mjs and dist/index.js.

@GeoffreyBooth
Copy link
Member

@GeoffreyBooth in that model, you’d transpile/copy two files, and have dist/index.mjs and dist/index.js.

That’s a much bigger burden on developers, to have essentially two build pipelines. Also there are a lot of developers who will want to use .js for both ESM and CommonJS files, which isn’t possible if they need to share the same folder.

About the only advantage "main": "./index" offers is that it doesn’t involve adding a new field to package.json. Conversely, adding a new field offers many advantages:

  • Users can use .js for everything if they want.
  • Users can have separate entry points in separate folders, without needing unnecessary build steps like copying files or rebuilding already-ESM files or creating symlinks. The package.json fields could point directly to the real files without any shenanigans necessary.
  • An "entrypoint" field that requires a full extension, regardless of whether or not extension searching is enabled for import statements, provides a way for build tools and other environments to find the ESM entry point for a package without needing to implement Node’s extension searching algorithm (which might very well be impossible once loaders are added).

For all these reasons and a few I can’t think of at the moment, we judge "exports" to be a better solution than "main": "./index". Yes it involves creating a new field, but there are big advantages to doing so.

@ljharb
Copy link
Member

ljharb commented Feb 27, 2019

It's a fair point that even with extension lookup, we might still want a way to have an alternative "main". However, since deep imports are important as well, imo it's not really sufficient unless we can provide an alternative root directory for the package, ie, "esm-root": "src" or something.

Re * Users can use .js for everything if they want., I don't think this is a benefit unless you can replace .js with "any extension they like", and unless you do replace "everything" with "ESM", since ESM JS is not "everything".

(Another alternative is to not use a dist dir, but instead, to write .mjs files in src and use a build process to produce the .js ones.)

Every build tool already implements node's searching algorithm - in practice this has proven to not be a hardship. Similarly, loaders would need to expose a way for build tools to tap into their resolution algorithm, otherwise they wouldn't work for build tools anyways (unrelated to extension searching).

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Feb 27, 2019

@ljharb As you know, one of the goals of the import file specifier resolution proposal was to support ESM in .js files. The reason to support ESM in .js files is so that users don’t have to work with multiple file extensions representing JavaScript—or to put it more bluntly, to not need to use .mjs. We know from user feedback that many users strongly dislike .mjs, and therefore we’re trying to provide an ESM implementation that doesn’t require the use of .mjs. Users are welcome to opt in to using .mjs, but whenever possible they should not be required to use it.

The "main": "./index" approach requires the use of either .mjs or .cjs (or some other secondary extension, if mapping becomes possible) for some of the users’ JavaScript files. Many users will find this unacceptable. I am one of them.

"exports" provides a way for defining separate CommonJS and ESM entry points without requiring users to use either .mjs or .cjs. Again, people still can use .mjs if they desire, but they would be able to create dual-CommonJS/ESM packages while still using .js for all their JavaScript files. It also corresponds with what users are already used to, where they define the CommonJS entry point in "main" and the ESM entry point in "module". It’s not much of a stretch to switch from using "module" to using "exports".

I support a way to map extensions, whether via some new package.json configuration or via loaders; but I would ask that you please stop blocking PRs where the intent is to provide a solution that doesn’t require the use of .mjs or some other secondary extension for some of users’ JavaScript. We’re trying to build an implementation where users can keep using .js if they prefer. Even in this new implementation there’s plenty of encouragement for them to use .mjs: better performance, and no need for --type or "type". But if users still want to keep using .js for all their JavaScript files, whether CommonJS or ESM, we should let them.

@ljharb
Copy link
Member

ljharb commented Feb 27, 2019

I agree they shouldn't be required to use .mjs, but no, it should not be opt in - it should be opt out, ie, the default (which it is). It's totally fine to offer a way to opt out of .mjs into another extension; but just because we've only heard about one desire (to parse .js as ESM) doesn't mean that one combination out of all the others should be privileged.

Your desire - to avoid using .mjs - is totally valid and something we should provide a way to opt in to. However, this can and should be done in a generic way, and "exports" does not provide this.

I'm going to continue blocking PRs that I don't think are a good fit - half measures are worse than none imo. For me, the goal of "let people not use .mjs" or "let people parse .js as ESM" is one I'm happy to provide, but I find to be one of the least important goals we have. Any effort that prioritizes that goal over others isn't going to get support from me, and it's not reasonable to ask me to sacrifice my priorities just because you disagree with them.

@devsnek
Copy link
Member

devsnek commented Feb 27, 2019

not entirely in the same context but this addresses the points of mjs because of course mjs has to keep coming up again and again :( please read this: nodejs/modules#253 (comment)

i see absolutely no reason for node to encourage any file extension for new stmr files besides .mjs. handling legacy cases is important but legacy is legacy, not the best design nor the design we should encourage.

@GeoffreyBooth
Copy link
Member

I agree they shouldn’t be required to use .mjs, but no, it should not be opt in - it should be opt out, ie, the default (which it is).

.mjs is already the default in this implementation. You opt out by using "type": "module" or --type=module. If you want to provide another way to opt out, that’s explicitly file extension-based, that’s fine; but that’s a new feature, that you should propose.

Since we both agree that users shouldn’t be required to use .mjs, and since it’s clear that the "main": "./index" approach requires the use of .mjs (or .cjs), can you drop your objection to this PR? Unless you can think of another way to support dual-CommonJS/ESM packages, it would seem that this is the best option on the table. We can always replace this with another implementation later if someone comes up with one. Perhaps when the file extensions map is proposed, part of that proposal can be a revision of how dual packages can work once extensions can be mapped.

guybedford and others added 11 commits February 27, 2019 09:06
Refs: nodejs/modules#180

PR-URL: nodejs#6
Reviewed-By: Myles Borins <MylesBorins@google.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Refs: nodejs/modules#180

PR-URL: nodejs#6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Refs: nodejs/modules#180

PR-URL: nodejs#6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Refs: nodejs/modules#180

PR-URL: nodejs#6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Myles Borins <mylesborins@google.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@nodejs-ci nodejs-ci force-pushed the modules-lkgr branch 2 times, most recently from 9301a06 to e721cd2 Compare March 14, 2019 08:10
@MylesBorins MylesBorins force-pushed the modules-lkgr branch 2 times, most recently from 484d1fb to 7efc53d Compare March 18, 2019 22:07
@nodejs-ci nodejs-ci force-pushed the modules-lkgr branch 3 times, most recently from c7fa700 to d69f765 Compare March 21, 2019 08:06
@MylesBorins MylesBorins force-pushed the modules-lkgr branch 5 times, most recently from 335d584 to 9a343ce Compare March 21, 2019 19:09
@nodejs-ci nodejs-ci force-pushed the modules-lkgr branch 2 times, most recently from 3a00b51 to bc101f6 Compare March 24, 2019 08:06
@MylesBorins MylesBorins force-pushed the modules-lkgr branch 3 times, most recently from fd5b55a to 3a40599 Compare March 27, 2019 02:42
jaydenseric referenced this pull request in mike-marcacci/fs-capacitor May 6, 2019
@guybedford guybedford closed this Jul 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants