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

Entry points proposal spec and implementation #32

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Projects
None yet
@guybedford
Copy link
Contributor

guybedford commented Feb 11, 2019

This implements a top-level --type flag for --experimental-modules with the following behaviours.

In addition "type": "esm" in the package.json is renamed to "type": "module", and a new extension .cjs is supported.

These behaviours are as defined in https://github.com/GeoffreyBooth/node-esm-entry-points-proposal.

  • node --experimental-modules ./x.unknown --type=module will always execute the top-level file as an ES module, unless it ends in .cjs, in which case an error is thrown.
  • node --experimental-modules ./x.unknown --type=commonjs will always execute the top-level file as a CommonJS module.
  • Support node --experimental-modules flags --check, --print, -e and stdin, including compatibility with --type
  • New error types are defined to handle the failure cases on these new flags.

With this PR, we get both the package.json boundary as the module format hint, as well as an ability to now override the format to a known format.

As with all previous PRs there are two commits - one with just the spec/doc changes, and another with the implementation.

The following features in the entry points proposal are not yet implemented in this PR and should be implemented as follow-up work:

  • --type=auto for syntax detection
  • symlink handling at the top-level boundary
  • -m alias for --type=module

@guybedford guybedford changed the title Entry points proposal: top-level --type, -m flags, spec updates Entry points proposal spec and implementation Feb 11, 2019

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Feb 11, 2019

I think a --type option on node is excellent, since it can only ever map to one unit of source code (stdin, the repl, or a single file). However, can you clarify:

With this PR, we get both the package.json boundary as the module format hint

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Feb 11, 2019

@ljharb glad to hear that! When no --type is specified, the default behaviour is to use the hint from the package boundary.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 11, 2019

@ljharb glad to hear that! When no --type is specified, the default behaviour is to use the hint from the package boundary.

The file extension has the highest precedence. If the filename has .mjs or .cjs, that’s all we need, Node parses as ESM or CommonJS respectively. Nothing can override that, and node --type=commonjs file.mjs (or the inverse) would throw.

If the file extension is ambiguous (.js) or missing (--eval etc.) then --type takes the next highest precedence.

If --type is unspecified, the package.json hint is used.

If there is no package.json hint, we parse as CommonJS for backward compatibility.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Feb 11, 2019

can it please be renamed to source-text-module so its correct and we can add more types later? also, we use cjs instead of commonjs throughout the rest of the codebase as a goal key. its fine if we change it but they should all be the same thing.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 11, 2019

can it please be renamed to source-text-module so its correct and we can add more types later?

No. The point is to match <script type="module">. Users already associate "module" with ES modules, both from <script type="module"> and the package.json "module" key that many users have already been using for years.

also, we use cjs instead of commonjs throughout the rest of the codebase as a goal key. its fine if we change it but they should all be the same thing.

We considered --type=cjs, but with the .cjs file extension we don’t want users to be confused that type is referring to a file extension (like cjs) as opposed to a module type. If we had --type=cjs then users might also expect --type=mjs to work, and node --type=mjs file.js is nonsensical. Within the Node codebase, sure, let’s rename it to commonjs everywhere if that’s fine with everyone, that might also help disambiguate from the file extension there too.

Again, Stage 4 is designated for UX review and feedback. What we name things now doesn’t matter, everything is temporary until we get broader exposure and feedback from a wider audience of users.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Feb 11, 2019

The point is to match....

kinda unfortunate design :shrug: i guess we can discuss it later though

[cjs stuff]

like i said, its fine to be "commonjs", it just needs to be updated in all the other places too.


--type

personal reminder for --entry-type

@nodejs-ci nodejs-ci force-pushed the nodejs:modules-lkgr branch from 66aebb1 to 5374da9 Feb 11, 2019

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Feb 11, 2019

What is the expected behavior of having colliding CLI flag and package.json?

package.json#type:commonjs
bin/entry.js
node -m bin/entry

The spec makes it look like package.json wins. The docs/this PR make it look like the CLI wins. I would heavily prefer either throwing on collision or not allowing the CLI to alter the format of a resource if it is already unambiguous.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 11, 2019

What is the expected behavior of having colliding CLI flag and package.json?

The spec says that the flag will “tell Node to parse as ESM entry points that would otherwise be ambiguous.” Hence the flag throws on .mjs or .cjs, since they’re never ambiguous. A package in general is ambiguous. It can always contain either CommonJS or ESM files, regardless of what gets put in package.json. A package with a package.json of {} can have .mjs files, or a package with a package.json of {"type": "module"} can have .cjs files. A package can be dual-mode, etc.

I would be wary of throwing in the example you listed above, because the user’s intent is clear. They need to tell Node the module type to use for the entry point either via a flag or package.json; they don’t necessarily need to set both. What would the error message even be for that case? “Error: bin/entry.js cannot be executed in module mode because its nearest parent package.json lacks "type": "module"“?

We can always add more error-checking later on to increase the cases under which we throw errors; we don’t need to throw for this case. Even though we could, it’s probably much simpler UX to just tell users that --type is always applicable for .js files, regardless of what package.json may say; especially now, since at least for a while there will be lots and lots of packages and projects out there with ESM in .js files and no "type": "module" in their package.jsons.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 11, 2019

On further thought, I guess this is a case of user intent versus author intent. Currently author intent overrides user intent for file extensions but nothing else (I think). I could also see a UX case for user intent always taking priority.

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Feb 11, 2019

The spec says that the flag will “tell Node to parse as ESM entry points that would otherwise be ambiguous.” Hence the flag throws on .mjs or .cjs, since they’re never ambiguous. A package in general is ambiguous. It can always contain either CommonJS or ESM files, regardless of what gets put in package.json. A package with a package.json of {} can have .mjs files, or a package with a package.json of {"type": "module"} can have .cjs files. A package can be dual-mode, etc.

I don't understand this statement. A package is unrelated to my question really except that it is used as a configuration point, only files have specific formats and the configuration points seem to have some conflict in this PR about specifying formats for files. The question isn't about if we can have dual mode packages, but rather what happens when a resource has conflicting configuration.

I would be wary of throwing in the example you listed above, because the user’s intent is clear. They need to tell Node the module type to use for the entry point either via a flag or package.json; they don’t necessarily need to set both. What would the error message even be for that case? “Error: bin/entry.js cannot be executed in module mode because its nearest parent package.json lacks "type": "module"“?

I don't understand this either, the user has 2 configuration points via CLI and package.json, and they are conflicting in my example. Since we have this conflict, why not just state that the conflict itself is an error "Cannot treat bin/entry.js as ESM via --type because a package.json has marked it as CJS" or somesuch; that would cause the user to resolve the conflict.

To clarify, the design of this PR currently means that files are not having a well defined format statically, as they can be altered depending on how they are consumed. importing bin/entry.js loads it as CJS, but you can use the CLI to make it load as ESM. Therefore, given the behaviors of this PR we have of no way of knowing how files are interpreted for bin/entry.js because the conflict makes a "it depends" scenario. Once bin/entry.js has been loaded it will permanently be in a format for the purposes of the Module Map, which also adds a wrinkle here as that means even though normally importing it would give a CJS format, if loaded via the CLI in the example above it would always be loaded as ESM even though we are using import:

node --type=module bin/entry.js
// bin/entry.js
// package.json denotes this file as CJS
// CLI denotes this file as ESM

// works if loaded via CLI with --type=module
// fails if loaded via static information from package.json
import('./entry.js');

We can always add more error-checking later on to increase the cases under which we throw errors; we don’t need to throw for this case. Even though we could, it’s probably much simpler UX to just tell users that --type is always applicable for .js files, regardless of what package.json may say; especially now, since at least for a while there will be lots and lots of packages and projects out there with ESM in .js files and no "type": "module" in their package.jsons.

We don't need to do anything really since we can mandate any design decision we can agree upon and we can write up rationalizations afterwards. The concern I have is that we have multiple configuration points and they are contradicting each other.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 11, 2019

@bmeck Sure, we can error if the package scope and --type are in conflict. Perhaps that makes the most sense.

For the purposes of development, though, I think that that can be a separate PR, if @guybedford doesn’t have time to add that before this week’s meeting.

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Feb 11, 2019

Seems fine as a diff PR

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 11, 2019

Seems fine as a diff PR

The other remaining thing we need to work out is how exactly dual-mode packages will work, since we would want to throw only if the flag conflicts with either way that a dual-mode package allows its files to be run. So maybe this error handling can be part of figuring out the details of how dual-mode packages can be specified by users and run by Node.

@nodejs-ci nodejs-ci force-pushed the nodejs:modules-lkgr branch 2 times, most recently from 55dcaf8 to 4228797 Feb 12, 2019

@guybedford guybedford force-pushed the guybedford:type-flag branch from af0d1ef to a63e09c Feb 13, 2019

@bmeck

bmeck approved these changes Feb 13, 2019

Copy link
Member

bmeck left a comment

LGTM

@jdalton
Copy link
Member

jdalton left a comment

LGTM

@MylesBorins
Copy link
Member

MylesBorins left a comment

Rubber stamp LGTM

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Feb 13, 2019

please run CI before landing and make commit messages follow upstream guidelines

@SMotaal

This comment has been minimized.

Copy link
Contributor

SMotaal commented Feb 13, 2019

Finally… this is an amazing step in my books 🙏

@SMotaal
Copy link
Contributor

SMotaal left a comment

LGTM

Alhadis added a commit to file-icons/atom that referenced this pull request Feb 14, 2019

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 14, 2019

☝️ someone’s following along closely

@guybedford guybedford force-pushed the guybedford:type-flag branch from a63e09c to 3a71de7 Feb 14, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Feb 14, 2019

@guybedford guybedford merged commit 35127a5 into nodejs:modules-lkgr Feb 14, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Feb 14, 2019

CI all green, merged.

@thysultan

This comment has been minimized.

Copy link

thysultan commented Feb 14, 2019

How would libraries(in node_modules) provide a path to the module, considering that "pkg.main" already points to for example a "umd" source file. Will there be a "pkg.src" field in symmetry with <script type=module src=url>?

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 15, 2019

How would libraries(in node_modules) provide a path to the module, considering that "pkg.main" already points

Currently in this implementation main points to the entry point for the package whether it's ESM or CommonJS. This precludes dual ESM/CommonJS packages, though, so we've been discussing creating a new field to handle defining separate entry points for each. That's another proposal yet to come.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Feb 15, 2019

It doesn't preclude it if we do extension look up - you'd omit the extension (which is a reigning best practice anyways with cjs) and then have one extension for ESM and another for CJS.

@Alhadis

This comment has been minimized.

Copy link
Contributor

Alhadis commented Feb 15, 2019

you'd omit the extension (which is a reigning best practice anyways with cjs)

That's what I've been doing with Roff.js (and a few other projects published to NPM with both .mjs and .js versions of the package entry-point:

/* package.json */
{
	…
	"main": "./pan-and-zoom",
	…
}

With package contents like this:

├── Makefile
├── README.md
├── package.json
├── pan-and-zoom.js
└── pan-and-zoom.mjs

Are you saying this will stop working at some point?

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Feb 15, 2019

Automatic extension resolution contradicts our goal of browser equivalence, so I assume it won't be enabled by default; though I expect it would be possible via opt-in configuration or a package-level loader.

In ESM mode .js files are treated as ESM, so at the very least it would be confusing to rely on extensions for disambiguation. A better UX is probably a way to explicitly define each entry point. Such a new field would also allow specifications of other types of entry points, like for browsers (see the commonly used browser field today) and future potential package types.

@devsnek

This comment was marked as off-topic.

Copy link
Member

devsnek commented Feb 15, 2019

@GeoffreyBooth slightly off topic, but if extension resolution is enabled by default, those who want browser equivalence can still type the full path, so i'm not really sure why you keep saying it contradicts browser equivalence.

@ljharb

This comment was marked as off-topic.

Copy link
Contributor

ljharb commented Feb 15, 2019

More to the point, i consider extension resolution by default a requirement for unflagging - browser equivalence doesn’t mean we have to hamstring ourselves; we can be a superset.

@Alhadis

This comment was marked as off-topic.

Copy link
Contributor

Alhadis commented Feb 15, 2019

Extension look-up could theoretically be fudged by servers specifically configured to serve different files when a missing index file has been requested by a client (assuming its capabilities were already known by the server...)

If browsers could perform extension resolution the way Node does, I'm sure they would (had they open-slather filesystem access, I mean). IIRC, Electron uses some Node-like mechanism for resolving path-specs without extensions.

@GeoffreyBooth

This comment was marked as off-topic.

Copy link
Contributor

GeoffreyBooth commented Feb 15, 2019

@ljharb and @devsnek I understand you both feel strongly about file extension resolution, but it’s not currently on the group-approved roadmap for this implementation. The plan even states specifically:

  • Remove dynamic path searching:
    • No extension adding.
    • No directory resolution, including no support for index.js or index.mjs.

Furthermore, #6 explicitly removed support for file extension and directory index resolution in ESM. That PR was approved per quorum of the group in a meeting on 2018-10-03 and merged in.

Of course, the group can always change its mind, and like I said I expect the group to support enabling automatic extension and index resolution somehow. But since it’s not currently part of the agreed-upon plan, it’s not unreasonable for me to operate under the assumption that it won’t be added.

The group has democratic procedures that we’ve been following to get our desired functionality into the new implementation. The first step would be to submit a PR to the roadmap to add that feature to the plan. The second step would be to write a proposal for how it would be implemented in concordance with the current state of our new implementation; it’s a little complicated now, since you’d need to define an order of precedence between .js, .mjs and .cjs and how they should behave in each mode. The third step would be to write a PR that implements it. If the group approves all three steps, your feature is in. I encourage you to participate in the process if you’d like to influence the final product of this group.

@Alhadis and @thysultan, for context @ljharb and @devsnek were two of the designers of the current --experimental-modules implementation that this group is working to replace.

@Alhadis

This comment was marked as off-topic.

Copy link
Contributor

Alhadis commented Feb 15, 2019

I understand. =)

Well however things turn out, I've no doubt it'll be something everybody will be happy with in the end. =) I've total faith in you lot — keep on keeping on. We'll reach Harmony† eventually.

† - punny shoutout to ES4, which never existed. R.I.P.

@devsnek

This comment was marked as off-topic.

Copy link
Member

devsnek commented Feb 15, 2019

@GeoffreyBooth you've linked phase 1, which is literally "remove all the features we don't agree on". we don't actually have consensus on whether the current resolution algorithm (which is require.resolve) will be modified. consensus is never needed to keep something unchanged.

@GeoffreyBooth

This comment was marked as off-topic.

Copy link
Contributor

GeoffreyBooth commented Feb 15, 2019

@devsnek There was consensus to remove automatic file extension resolution. That happened when the roadmap for Phase 1 was approved and when #6 was merged in. So yes, consensus isn’t needed to leave something unchanged—but at the moment, the consensus is that the new --experimental-modules will ship without automatic extension and index resolution, since later phases don’t mention re-adding that functionality. If you’d like to change that, I encourage you to participate in the process. You might very well find a supportive majority for your proposal.

Personally I think that file extension and index resolution should be opt-in functionality. That way people are encouraged to write packages that are compatible with browsers by default, rather than requiring consumers to set up a build process to make packages usable in browsers. It also follows what users expect when writing ESM code in browsers, where extensions are generally required. Requiring extensions means we sidestep the UX question of which of the JavaScript extensions has precedence over the others (and .wasm and other file types when those are eventually added) and relieves Node of the performance hit caused by searching the file system for file.mjs, file.js, file.cjs, file.wasm, file/index.mjs, file/index.js, file/index.cjs, file/index.wasm, etc.

So please, make a proposal and we can debate how you propose to address these concerns; or you can argue that the convenience of automatic resolution outweighs them. Whatever. But we have a process. Please participate: open a PR.

@devsnek

This comment was marked as off-topic.

Copy link
Member

devsnek commented Feb 15, 2019

@GeoffreyBooth

the content of phase 1 is NOT eligible to ship, and this was made EXTREMELY clear when we achieved consensus to move to phase 1. If you’d like to change that, I encourage you to participate in the process. You might very well find a supportive majority for your proposal.

@GeoffreyBooth

This comment was marked as off-topic.

Copy link
Contributor

GeoffreyBooth commented Feb 15, 2019

So I wanted to enable ESM support for code passed in via --eval. It wasn’t supported already in the new implementation in this repo, and it wasn’t on our roadmap. So I opened a PR against the roadmap to get group approval that we would be adding such functionality. I also invited anyone in the group who was interested in joining me in developing the specifics of how the feature should work.

That PR was approved, so then I wrote a proposal, and opened an issue where I invited the group to offer feedback on the proposal. I and the others who collaborated on drafting the proposal responded to feedback, agreeing with most of it, and we made changes to address concerns.

Then @guybedford wrote this PR to implement the proposal, and this was submitted for group approval. The group had a few days to review it, and many people made comments, and there was discussion; and in the meeting we agreed to approve this PR, and now it’s in.

That’s the process as I understand it. The simple fact is that automatic file extension/index resolution is not currently included in the implementation in this repo. It’ll need to get added via a pull request to this repo. You can go straight to submitting such a PR, but I recommend you follow the precursor steps to offer the group chances to collaborate with you and provide feedback, to increase your PR’s chances of success. I would be happy to collaborate.

@ljharb The same goes with your idea for a file extensions map. I support that! But it’s your idea, so you should champion it. Let’s work together to make it happen.

@devsnek

This comment was marked as off-topic.

Copy link
Member

devsnek commented Feb 15, 2019

@GeoffreyBooth i think we might be talking about different things at this point, i'll try to clarify tomorrow (not in this thread)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.