Skip to content

Edit of minimal kernel doc #2

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Sep 20, 2018
Merged

Edit of minimal kernel doc #2

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
79d8fff
Rewrite: emphasize that minimal kernel is just the first phase of a m…
GeoffreyBooth Sep 14, 2018
6148b83
Revisions per Myles' notes
GeoffreyBooth Sep 14, 2018
7392f9f
Move up features that are already part of the existing implementation
GeoffreyBooth Sep 17, 2018
d79cad9
Don't name formats
GeoffreyBooth Sep 17, 2018
1ec5469
Remove specific later phases plans
GeoffreyBooth Sep 17, 2018
7597e0b
some minor changes to satisfy myles
MylesBorins Sep 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 0 additions & 44 deletions doc/minimal-kernel.md
View file
Open in desktop

This file was deleted.

100 changes: 100 additions & 0 deletions doc/plan-for-new-modules-implementation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
# Plan for New Modules Implementation

This document outlines the plan for building a new implementation to support ECMAScript modules in Node.js. The general idea is to start with a “minimal kernel” as Phase 1, which consists of features that the @nodejs/modules group have agreed will be necessary for all potential iterations of our ESM implementation. Phase 1 does _not_ include features that preclude other potential features or implementation approaches; and Phase 1 also does not include some features that should naturally be built in a later phase of development, for example because those features depend on features planned for Phase 1. The minimal kernel/phase 1 is _not_ intended to be merged into Node core or released; it is only a starting point for gradually building layers of consensus.

At every phase, the following standards must be maintained:

* Spec compliance ([#132](https://github.com/nodejs/modules/issues/132)): We must always follow the ES spec.
* Browser equivalence ([#133](https://github.com/nodejs/modules/issues/133)): There’s room for debate in specific cases, but in general if Node is doing something that browsers also do, Node should do it in the same way. Alternatively, code that executes in both environments should produce identical results.
* Don’t break CommonJS ([#112](https://github.com/nodejs/modules/issues/112)): We cannot cause breaking changes with regards to CommonJS.

See also the [features list in the README](https://github.com/nodejs/modules#features).

## Phase 1: The Minimal Kernel

These features will be part of the first phase of development:

* `createRequireFunction` ([nodejs/node#19360](https://github.com/nodejs/node/pull/19360)) is the only way to import CommonJS into an ES module, for now.
- `import.meta.require` does not fail early enough.
- Hold off on `import` statements for CommonJS until more progress is made on the dynamic modules spec.

* `import` statements will only support files with an `.mjs` extension, and will import only ES modules, for now.
- In a later phase, the intention is to move forward with format databases to map extensions and support multiple use cases.
- No JSON or native modules; `createRequireFunction` can be used to get these.


### How will we get from where we are to Phase 1

* Remove support in the `import` statement of formats other than ESM:
- No CommonJS.
- No JSON.
- No native modules.
- Implemented in https://github.com/nodejs/ecmascript-modules/pull/3

* Remove dynamic path searching:
- No extension adding.
- No directory resolution, including no support for `index.js` or `index.mjs`.
- Still support the `main` field, but it must specify a full filename.
- Implemented in https://github.com/nodejs/ecmascript-modules/pull/2

* Add `createRequireFunction`.
Copy link
Owner

@MylesBorins MylesBorins Sep 17, 2018

Choose a reason for hiding this comment

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

we can likely remove this from the doc as it has landed on master.

Unless we want to leave it in for posterity

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 17, 2018

Choose a reason for hiding this comment

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

I don't have a preference. I could add the same "already in" note, that's probably clearest.

Copy link

@michael-ciniawsky michael-ciniawsky Sep 17, 2018

Choose a reason for hiding this comment

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

Then it should be createRequireFromPath as that's sadly the clunk name it landed with...

- Implemented in https://github.com/nodejs/node/pull/19360


## Features Expected in Later Phases
Copy link
Owner

@MylesBorins MylesBorins Sep 14, 2018

Choose a reason for hiding this comment

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

Are all these features expected?

Perhaps "Features being considered"

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 14, 2018

Choose a reason for hiding this comment

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

Several of these features would upset certain people if they’re not included somewhere in the document. That’s the basic issue I’m trying to address, is that people will feel like they need to fight for their top priorities to be part of Phase 1 unless they feel secure that yes, it’s on the list, it’s gonna get built, etc.


These features are agreed upon, but make sense to include in a later phase of development; or there is agreement on the need for this feature, but a lack of consensus on the implementation details:

* Browser-compatible specifier resolution ([#109](https://github.com/nodejs/modules/issues/109)), a.k.a. bare imports.
Copy link
Owner

@MylesBorins MylesBorins Sep 14, 2018

Choose a reason for hiding this comment

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

This is in conflict with supporting the main field above.

we either support bare imports or we don't

bare imports with a main resolution as stated above is the equivelent to support bare imports that are 100% compatible with package name maps

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 14, 2018

Choose a reason for hiding this comment

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

Okay, so move the reference to main field into this item? You’re right, if we don’t have bare imports in phase 1 then we wouldn’t have main there either (for ESM, anyway).

Copy link
Owner

@MylesBorins MylesBorins Sep 17, 2018

Choose a reason for hiding this comment

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

bare imports will be supported in the initial version, so this is not entirely accurate

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 17, 2018

Choose a reason for hiding this comment

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

bare imports will be supported in the initial version

As in, the legacy require resolution algorithm is supported currently? Perhaps the support for that (in ESM) should be removed as well, if the implementation might end up going in a different direction?

Copy link
Owner

@MylesBorins MylesBorins Sep 17, 2018

Choose a reason for hiding this comment

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

that's not what I'm talking about. I'm talking about the ability to import a module that is living in the node_modules folder... only supporting main

that is what the original document outlined

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 17, 2018

Choose a reason for hiding this comment

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

I don’t understand. How would you import a module from node_modules other than using the bare imports require algorithm? What kind of code are you talking about?

import _ from 'lodash'; // bare imports using `main` field?
import _ from './node_modules/lodash'; // no bare import, but still using `main`?
import _ from './node_modules/lodash/index.mjs'; // fully explicit?

Copy link
Owner

@MylesBorins MylesBorins Sep 17, 2018

Choose a reason for hiding this comment

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

I'm saying the idea was to support main. That being said I understand where you are coming from and support removing bare imports from the minimal kernel

- Implementation to be discussed, possibly based on [package name maps](https://github.com/domenic/package-name-maps).

* `import.meta.url`.
Copy link
Owner

@MylesBorins MylesBorins Sep 17, 2018

Choose a reason for hiding this comment

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

Why would we remove this from the kernel? It is already implemnted

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 17, 2018

Choose a reason for hiding this comment

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

It is already implemnted

Then why is it on this list at all? 😄I assumed it was a todo item, otherwise it wouldn’t be here. I’ll move it up to Phase 1. Is it already in the branch, or is it a PR?

If this and import() are already merged into the core, do we even need to list them in the doc?


* Dynamic `import()`.
Copy link
Owner

@MylesBorins MylesBorins Sep 14, 2018

Choose a reason for hiding this comment

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

This is already in the implementation, necessary for getting ESM into CJS, stage 3 at TC39, and does not preclude any other features.. why not include in phase 1?

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 14, 2018

Choose a reason for hiding this comment

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

It is? Sure, I didn’t realize.

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 14, 2018

Choose a reason for hiding this comment

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

Is there a PR I can link to?


* User-configurable map for file extensions to parse goals, a.k.a. `mimes` field ([#160](https://github.com/nodejs/modules/pull/160)).
- This can expand the uses of the `import` statement to import ES modules from `.js` files, as well as support JSON and other formats.
- Common formats like JSON or WASM will have default mappings.
Copy link
Owner

@MylesBorins MylesBorins Sep 17, 2018

Choose a reason for hiding this comment

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

Do we have consensus on this? I don't think so.

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 17, 2018

Choose a reason for hiding this comment

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

Do we have consensus on this? I don’t think so.

On the “JSON or WASM will have default mappings” part? Yeah, there was consensus on that in all the discussions about mappings. Node will have a default set of mappings, like .json ↔️ application/json, that will apply in the absence of the user specifying otherwise (unless they add null as the first element of an array of mapping objects, per Bradley’s proposal). All the final versions of all the issues related to the mappings stuff had consensus.

Copy link
Owner

@MylesBorins MylesBorins Sep 17, 2018

Choose a reason for hiding this comment

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

There is not currently consensus about what types of imports the default implementation will support

- Need to work out how this functionality interacts with loaders.

* Dynamic path searching.
- Requiring the full path is an issue for tooling and a long term solution is required.
- Migration strategies also have issue with this.
- Need to resolve issues related to building this in a browser-compatible way.

* Multi-mode packages ([#94](https://github.com/nodejs/modules/issues/94)).

* Loaders ([#82](https://github.com/nodejs/modules/issues/82)), ([#96](https://github.com/nodejs/modules/issues/96)).
Copy link
Owner

@MylesBorins MylesBorins Sep 14, 2018

Choose a reason for hiding this comment

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

We need an action plan for removing loaders if we plan to remove it from the minimal implementation.

TBH I never found loaders to be a controversial topic so I was kind of leaving them alone within this doc. Is there something about current implementation that precludes other features?

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 14, 2018

Choose a reason for hiding this comment

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

I thought the plan was to significantly change the implementation of loaders. Is that not the case? I agree the desire to have loaders isn’t controversial, but I thought the implementation was still being debated.

- Implementation to be determined.

* ESM in executable files ([#152](https://github.com/nodejs/modules/issues/152)).

* Callable resolver ([#157](https://github.com/nodejs/modules/issues/157)).

* Mock modules (injection) ([#98](https://github.com/nodejs/modules/issues/98)).


## Other Features
Copy link
Owner

@MylesBorins MylesBorins Sep 14, 2018

Choose a reason for hiding this comment

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

This seems odd and ripe for controversy tbh

I'd like us to reach consensus on a minimal implementation as soon as possible so we can begin iterating over specific combinations of features. I don't think that this list of "expected features" vs "other features" is based on any specific consensus and is likely to distract from getting consensus on this document.

I personally think we are better off focusing on "what the kernel is" and "what specific changes get us there". What's next should be a planning phase next, or we simply won't get anything done.

Copy link
Author

@GeoffreyBooth GeoffreyBooth Sep 14, 2018

Choose a reason for hiding this comment

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

On the one hand I see your point, and feel like sure, we can just cut the section. But on the other, I think it’s useful to list out features that are blocked with reasons why they’re blocked, like all the ones waiting on spec updates. This could be a separate or later document though. What do you think?


The features on this list cannot be said to be sure to come in a later phase, for one reason or another. It may be that there’s a dispute over whether the feature can be achieved in a spec compliant way, or a browser-compatible way. It could be that the group might decide the feature simply isn’t worth including in the final implementation, for design or other reasons. A feature’s inclusion on this list does _not_ mean that it might not move up to the “expected in later phases” list, but rather only that there are issues to be resolved before it can do so.

* Named imports from CommonJS modules via an `import` statement, e.g. `import { shuffle } from 'underscore'` ([#81](https://github.com/nodejs/modules/issues/81)):
- Waiting on updates to the spec to allow this.

* Any CommonJS imports via an `import` statement ([#100](https://github.com/nodejs/modules/issues/100)).
- See previous.

* Arbitrary sources for module source text ([#97](https://github.com/nodejs/modules/issues/97)).
- We would need to work out how to make this compatible with `import` statements, the `mimes` field, etc. Perhaps implement via a loader?

* Specifier resolution customization ([#110](https://github.com/nodejs/modules/issues/110)).
- This raises implementation concerns, such as for static analysis. Perhaps implement via a loader?

* Package encapsulation ([#111](https://github.com/nodejs/modules/issues/111)):
- Depends on implementation of browser-compatible specifier resolution ([#109](https://github.com/nodejs/modules/issues/109)), a.k.a. bare imports.

* Conditional imports ([#113](https://github.com/nodejs/modules/issues/113)):
- Depends on top-level `await`, which is working its way through the spec process.

* WASM modules (#[106](https://github.com/nodejs/modules/issues/106)).
- There’s a lot of other work to do to support WASM in Node in general; the ES modules implementation can ship without support for WASM modules, so long as there’s a clear path for adding WASM support later.