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

Irp type implementation #28

Merged
merged 2 commits into from
Feb 6, 2019
Merged

Conversation

guybedford
Copy link
Contributor

This PR includes the specification and implementation work for:

The implementation is exactly in sync with the specification diff here, and provided as separate commits.

@MylesBorins
Copy link
Contributor

is this trumped by #29?

@guybedford
Copy link
Contributor Author

This is the PR seeking consensus. #29 is the addon given dynamic modules, but we still don't know what the status of dynamic modules is right now.

@GeoffreyBooth
Copy link
Member

This corresponds with nodejs/modules#256. If the group wants to merge in this and dynamic modules, then we could merge #29 instead; but we’re assuming there’s at least a good chance that there won’t be appetite just yet for dynamic modules, so we’re hoping that this PR (without dynamic modules) can get merged in at least.

@bmeck
Copy link
Member

bmeck commented Jan 30, 2019

I will not be able to attend any meeting today since TC39 is going on, but would like to express support for this PR if there is not quorum in a meeting. If such a meeting takes place, consider myself to be a +1.

@ljharb
Copy link
Member

ljharb commented Jan 30, 2019

I'll try to attend but may not be able to for the same reason; I object to type: 'esm' existing (ideally even in our fork) prior to a more generic, low-level approach that allows remapping of any extension/mime to any parse goal. (one the low-level feature exists, adding a mutually exclusive type field as a shortcut seems fine to me)

targos
targos previously requested changes Jan 30, 2019
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
@MylesBorins
Copy link
Contributor

As a note before the meeting so this isn't a surprise... If we cannot get named exports for CJS modules I am not in support of the IRP proposal.. specifically at this point I am not sure we should be shipping any interop without named exports. It appears, and I could be wrong, that if we will not be shipping interop that there is not a need for this proposal. Thoughts?

@guybedford
Copy link
Contributor Author

Thanks @targos for the review, I've integrated those changes.

@GeoffreyBooth
Copy link
Member

I think without CommonJS interoperability there’s still definitely a need for this proposal, though obviously not the parts of it that concern import statements of CommonJS files/packages. The proposal includes several features that are ESM-specific, like how to enable .js files as ESM. I would want to keep those features even without CommonJS interop, though the proposal would need to be rewritten.

@GeoffreyBooth
Copy link
Member

Per @MylesBorins in the meeting today, this will be merged in Wednesday, Feb. 6 unless there are objections.

If you have objections, please note them before Feb. 6 so that they can be addressed before the next meeting on Wednesday, Feb. 13.

ljharb
ljharb previously requested changes Jan 30, 2019
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.

surfacing my objection here: #28 (comment)

@weswigham
Copy link

@ljharb can't we agree that a mechanism should exist and that when it exists it will be mutually exclusive with this flag (and that this flag should be an alias for some kind of other more verbose configuration), but defer specifying it? Having to do that means getting into the weeds about pluggable loaders and mimes and a whole boatload of stuff that by and large doesn't matter for what we really want to solve with this flag.

@GeoffreyBooth
Copy link
Member

@ljharb I’m happy to support a "mimes" field or some other way of configuring file extensions to MIME types, but that feels like a separate feature that deserves its own proposal text and review. nodejs/modules#160 (comment) could be a starting point. And like @weswigham mentioned, I feel like it would be hard to design without a design for loaders.

Can we move forward without such a field for now and add it later once someone has done the work to design and implement it?

@ljharb
Copy link
Member

ljharb commented Jan 30, 2019

My concern is, what happens if we can never figure that out? Then we'd be stuck with type without being able to have the primitive it's sugar for.

Also, how can we make it mutually exclusive now if we don't know the final name for the generic mechanism's package.json field?

@GeoffreyBooth
Copy link
Member

My concern is, what happens if we can never figure that out? Then we’d be stuck with type without being able to have the primitive it’s sugar for.

My understanding of loaders is that they will certainly be able to control Node’s handling of file extensions, for example by sending .ts files through a TypeScript compiler. As loaders are very much on our roadmap, users will have a way to customize extensions and MIME types in whatever we ship.

@ljharb
Copy link
Member

ljharb commented Jan 30, 2019

I'm concerned with being able to specify it in package.json so that loaders can infer the only actually safe and reliable means of determining this: the author's intention. Otherwise, a (non-per-packaeg) loader has to unsafely make a choice for code that the person configuring the loader didn't author.

@GeoffreyBooth
Copy link
Member

Currently "type": "esm" conveys the author’s intention that .js files in that package should be treated as ESM. That’s all it does and all it has to do. I don’t see why we need a more complicated configuration block at this time. I would be happy to add one, if you’d like to design it, but I don’t see why it’s a blocker for this PR.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2019

I think that privileging one remapping (.js means "ESM") over all the other possible ones (any extension means "CJS", "ESM", "WASM", etc) is a bad precedent to set, even temporarily.

@weswigham
Copy link

I think that privileging one remapping (.js means "ESM") over all the other possible ones (any extension means "CJS", "ESM", "WASM", etc) is a bad precedent to set, even temporarily.

I think privileging that one remapping is expressly what we're trying to do - we're expressly responding to the community's adverse reaction to not being able to have esm in .js files by default by making it an expressly trivial operation to, within a package, allow that. We are intentionally privileging that remapping, because for many, it's the only remapping that matters. While I agree that the more general system is more versatile and desirable, I do not however think it's implementation should block this - especially since the exact implementation of that configuration shouldn't have any bearing on the shape or observable behavior of this flag.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2019

Fair point. I'll think about that between now and the 6th, including thinking about a possible design for the more granular case.

@GeoffreyBooth
Copy link
Member

For what it’s worth, "type": "esm" isn’t meant just as a shorthand for a particular MIME mapping. It’s called type because it’s describing the package type, the way a file extension describes a file’s type. "type": "esm" means this package is an ESM package. It’s a piece of metadata about the package.

Currently all Node is doing with that piece of metadata is mapping .js files to be ESM, but that might not be all that "type": "esm" infers in the future. It’s a different concept than a "mimes" field or shorthand for a particular MIME mapping. Again I’m not opposed to a "mimes" field, but that’s a different feature and can be iteratively added.

The algorithm to resolve an ES module specifier is provided through
_ESM_RESOLVE_:
The algorithm to load an ES module specifier is given through the
**ESM_RESOLVE** method below. It returns the resolved URL for a
Copy link
Member

Choose a reason for hiding this comment

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

Prolly should tweak any "URL" reference to "File URL".

@ljharb
Copy link
Member

ljharb commented Jan 31, 2019

@GeoffreyBooth right, but "an X package" is not the same thing as "a package that parses .js as X" - because a package might not have any .js in it at all, eg if it's .json, or .node, or extensionless. .node and .json aren't suddenly going to be parsed as ESM in a type esm package, i assume?

@GeoffreyBooth
Copy link
Member

.node and .json aren’t suddenly going to be parsed as ESM in a type esm package, i assume?

"type" is a description, not a configuration. "type": "esm" says that this is an ESM package. That’s it. What Node does with files inside ESM packages is up to Node. "type": "esm" doesn’t say anything about how Node should treat .node or .json files. It doesn’t even say how Node should treat .js files, because it’s description, not configuration. We’re defining Node’s default behavior for ESM packages to include that Node should treat .js files as ESM, but that’s not mandated by the package.json, as the package.json includes no configuration of Node. It only includes some metadata where the author is informing any consumers that this is an ESM package, whatever that means to whatever is consuming the package.

Node isn’t the only consumer of NPM packages: they’re also used by bundlers, for example. Having neutral metadata describing a package is different than something like a "babel" field configuring how Babel should interact with a package. I’m not opposed to adding a configuration field, but that’s not what "type" is intended to be.

The file extension .zip tells all who see it that file.zip is a .zip file. It doesn’t say that you can or can’t list its contents, extract it, add a new file to the archive, etc. Some applications that are consumers of .zip files can do all those things; some can only extract, etc. In this sense, .zip is descriptive metadata, just like "type": "esm".

MylesBorins added a commit that referenced this pull request Mar 14, 2019
MylesBorins added a commit that referenced this pull request Mar 15, 2019
MylesBorins added a commit that referenced this pull request Mar 18, 2019
MylesBorins added a commit that referenced this pull request Mar 18, 2019
MylesBorins added a commit that referenced this pull request Mar 18, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 19, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 20, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 21, 2019
MylesBorins added a commit that referenced this pull request Mar 21, 2019
MylesBorins added a commit that referenced this pull request Mar 21, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 22, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 23, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 24, 2019
MylesBorins added a commit that referenced this pull request Mar 26, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 27, 2019
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Mar 27, 2019
This PR updates the current `--experimental-modules` implementation
based on the work of the modules team  and reflects Phase 2 of our
new modules plan.

The largest differences from the current implementation include

* `packge.type` which can be either `module` or `commonjs`
  - `type: "commonjs"`:
    - `.js` is parsed as commonjs
    - default for entry point without an extension is commonjs
  - `type: "module"`:
    - `.js` is parsed as esm
    - does not support loading JSON or Native Module by default
    - default for entry point without an extension is esm
* `--entry-type=[mode]`
  - allows you set the type on entry point.
* A new file extension `.cjs`.
  - this is specifically to support importing commonjs in the
    `module` mode.
  - this is only in the esm loader, the commonjs loader remains
    untouched, but the extension will work in the old loader if you use
    the full file path.
* `--es-module-specifier-resolution=[type]`
  - options are `explicit` (default) and `node`
  - by default our loader will not allow for optional extensions in
    the import, the path for a module must include the extension if
    there is one
  - by default our loader will not allow for importing directories that
    have an index file
  - developers can use `--es-module-specifier-resolution=node` to
    enable the commonjs specifier resolution algorithm
  - This is not a “feature” but rather an implementation for
    experimentation. It is expected to change before the flag is
    removed
* `--experimental-json-loader`
  - the only way to import json when `"type": "module"`
  - when enable all `import 'thing.json'` will go through the
    experimental loader independent of mode
  - based on whatwg/html#4315
* You can use `package.main` to set an entry point for a module
  - the file extensions used in main will be resolved based on the
    `type` of the module

Refs: https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md
Refs: https://github.com/GeoffreyBooth/node-import-file-specifier-resolution-proposal
Refs: nodejs/modules#180
Refs: nodejs/ecmascript-modules#6
Refs: nodejs/ecmascript-modules#12
Refs: nodejs/ecmascript-modules#28
Refs: nodejs/modules#255
Refs: whatwg/html#4315
Refs: WICG/webcomponents#770
Co-authored-by: Myles Borins <[email protected]>
Co-authored-by: John-David Dalton <[email protected]>
Co-authored-by: Evan Plaice <[email protected]>
Co-authored-by: Geoffrey Booth <[email protected]>
Co-authored-by: Michaël Zasso <[email protected]>

PR-URL: nodejs#26745
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Apr 5, 2019
This PR updates the current `--experimental-modules` implementation
based on the work of the modules team  and reflects Phase 2 of our
new modules plan.

The largest differences from the current implementation include

* `packge.type` which can be either `module` or `commonjs`
  - `type: "commonjs"`:
    - `.js` is parsed as commonjs
    - default for entry point without an extension is commonjs
  - `type: "module"`:
    - `.js` is parsed as esm
    - does not support loading JSON or Native Module by default
    - default for entry point without an extension is esm
* `--entry-type=[mode]`
  - allows you set the type on entry point.
* A new file extension `.cjs`.
  - this is specifically to support importing commonjs in the
    `module` mode.
  - this is only in the esm loader, the commonjs loader remains
    untouched, but the extension will work in the old loader if you use
    the full file path.
* `--es-module-specifier-resolution=[type]`
  - options are `explicit` (default) and `node`
  - by default our loader will not allow for optional extensions in
    the import, the path for a module must include the extension if
    there is one
  - by default our loader will not allow for importing directories that
    have an index file
  - developers can use `--es-module-specifier-resolution=node` to
    enable the commonjs specifier resolution algorithm
  - This is not a “feature” but rather an implementation for
    experimentation. It is expected to change before the flag is
    removed
* `--experimental-json-loader`
  - the only way to import json when `"type": "module"`
  - when enable all `import 'thing.json'` will go through the
    experimental loader independent of mode
  - based on whatwg/html#4315
* You can use `package.main` to set an entry point for a module
  - the file extensions used in main will be resolved based on the
    `type` of the module

Refs: https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md
Refs: https://github.com/GeoffreyBooth/node-import-file-specifier-resolution-proposal
Refs: nodejs/modules#180
Refs: nodejs/ecmascript-modules#6
Refs: nodejs/ecmascript-modules#12
Refs: nodejs/ecmascript-modules#28
Refs: nodejs/modules#255
Refs: whatwg/html#4315
Refs: WICG/webcomponents#770
Co-authored-by: Myles Borins <[email protected]>
Co-authored-by: John-David Dalton <[email protected]>
Co-authored-by: Evan Plaice <[email protected]>
Co-authored-by: Geoffrey Booth <[email protected]>
Co-authored-by: Michaël Zasso <[email protected]>

PR-URL: #26745
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
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