Edit of minimal kernel doc

[GeoffreyBooth]

I took a pass at the minimal kernel doc. Sorry it doesn’t diff well; once I started putting things into complete sentences it got all mismatched. But there’s less changed here than first meets the eye.

Gist of the changes:

• The phrase “minimal kernel” makes me (and others) nervous, because a minimal kernel could very easily be a minimal modules implementation that merges into core and ships, potentially unflagged. I renamed it to “Phase 1” to emphasize that this is just the first stage of a multistage process, and that the goal isn’t just the minimal kernel but rather a new modules implementation.

• To further emphasize that the minimal kernel is just the first step, I added sections for many of our other features from the features list. Which features go in which section is very much up for debate; in general for now I wanted to put everything in the “expected to come in a later phase” section aside from features that we’ve already discussed as having significant issues.

• I moved browser-compatible import specifiers/bare imports, import.meta.url and dynamic import() into the “expected soon” section, because we don’t have PRs ready to go right now for these features. For bare imports we need to go through a design process, whether it’s adopting package name maps or something else. None of these three are able to be “merged in tomorrow,” to use @MylesBorins’ phrase from the meeting. That’s not to say that they’re not all important, or that we don’t have consensus on them; but they have the potential to push out the date of completing Phase 1 for weeks. I’d much rather get a phase 1 “tomorrow” and score a big quick win, and then immediately start talking about adding these three (or more) features in phase 2.

• I added a lot of links and tried to connect this to our features list.

https://github.com/GeoffreyBooth/modules/blob/minimal-kernel/doc/plan-for-new-modules-implementation.md

[MylesBorins]

A lot of good ideas here. Put comments in line. Open to landing changes to the earlier part of the document but have concerns about the attempt to define later stags.

Would prefer this document to be as focused as possible.

Call is Phase-1.md and we can work on a Phase-2.md afterwards

[MylesBorins]

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

[GeoffreyBooth]

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).

[MylesBorins]

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?

[GeoffreyBooth]

It is? Sure, I didn’t realize.

[GeoffreyBooth]

Is there a PR I can link to?

[MylesBorins]

Are all these features expected?

Perhaps "Features being considered"

[GeoffreyBooth]

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.

[MylesBorins]

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?

[GeoffreyBooth]

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.

[MylesBorins]

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.

[GeoffreyBooth]

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?

[MylesBorins]

You would have to dig it up. I think it was authored by @targos. It was originally behind a separate flag and I landed a change to make it on by default when experimental modules landed. Is there a reason to remove from minimal implementation? Without it we don't have bi directional interop... Don't think we need to reference ordinal PRS

…

On Fri, Sep 14, 2018, 1:48 AM Geoffrey Booth ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/plan-for-new-modules-implementation.md <#2 (comment)>: > + - Implemented in nodejs/ecmascript-modules#2 + +* Add `createRequireFunction`. + - Implemented in nodejs/node#19360 + + +## Features Expected in Later Phases + +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](nodejs#109)), a.k.a. bare imports. + - Implementation to be discussed, possibly based on [package name maps](https://github.com/domenic/package-name-maps). + +* `import.meta.url`. + +* Dynamic `import()`. Is there a PR I can link to? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV5sFrszKM85uC_UhxerLpANRwG7Bks5ua0MsgaJpZM4Woorm> .

[GeoffreyBooth]

You would have to dig it up. I think it was authored by @targos. It was originally behind a separate flag and I landed a change to make it on by default when experimental modules landed. Is there a reason to remove from minimal implementation? Without it we don’t have bi directional interop… Don’t think we need to reference ordinal PRS

Sorry I misread, I thought you meant it was implemented but not already merged in. Nevermind.

I updated per your comments. Not sure about the Phase 2 vs 3 split but I feel like this document needs to include more than just Phase 1 or people will fight over Phase 1. I would really love to get Phase 1 out the door before our next meeting, how awesome would that be!

[MylesBorins]

Left some comments in line.

I have mixed feelings about making any sort of decision regarding Phase 2 / 3 in this document. It seems a bit premature. I was imagining that we would have the kernel and then the we could design implementations on top of it. Outlining phases seems to be making decisions about the orders of features as well as the idea that the implementation may not come all at once.

I would like to see this document scoped to just the kernel.

[MylesBorins]

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

[GeoffreyBooth]

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?

[MylesBorins]

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

[GeoffreyBooth]

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?

[MylesBorins]

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

[MylesBorins]

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

[GeoffreyBooth]

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?

[MylesBorins]

Why would we remove the current loader implementation>

[GeoffreyBooth]

Why would we remove the current loader implementation>

Because I thought the team was planning on redesigning it. Is that not the case? That was what came up with my mimes proposal, that I was told to slow down because it related to loaders and loaders were going to be redesigned.

[MylesBorins]

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

[GeoffreyBooth]

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.

[MylesBorins]

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

[MylesBorins]

This was not part of the initial decision on the minimal kernel. Have real mixed feelings about removing this, but can understand why we may not want to make this decision right now, especially if we are going to use another keyword. I'd rather punt this conversation to review on the original PR rather than making the decision here

[GeoffreyBooth]

Have real mixed feelings about removing this

I put this here because I was assuming there would be no bare imports resolution algorithm of any kind (not require‘s algorithm nor package name maps) in Phase 1. If there are no bare imports in Phase 1, of what use is a main field?

Don’t be like everyone else concerned that their priority isn’t part of the minimal kernel. 😄 Obviously we’ll have some way to do bare imports in the final implementation. It doesn’t need to be in Phase 1, and probably shouldn’t, since figuring it out will take a lot of discussion. I think it’s more important to show progress and get a Phase 1 nailed down than get sidetracked on bare imports for weeks.

[MylesBorins]

bare imports + main is not the require algorithm.

This was what was discussed and in the original document. From the conversation we initially had (me + @bmeck + @guybedford) it was discussed that any implementation, including the browsers, would require bare imports to be useful. A practical argument against this for right now is that we don't have consensus for main, so it would make sense to potentially drop.

Don’t be like everyone else concerned that their priority isn’t part of the minimal kernel.

I see where you are coming from this but don't find this particular kind of rhetoric helpful or productive. Say what you need to say from a technical stand point and don't make it personal.

[GeoffreyBooth]

it was discussed that any implementation, including the browsers, would require bare imports to be useful

Of course it would. But again, we’re not shipping Phase 1, are we? So Phase 1 doesn’t need to be useful in a technical sense. Phase 1 will be useful that it will start to establish consensus and show that we can make forward progress, and give people a starting point for further PRs. There absolutely is value in pulling together a single commit on the repo that does that, even if it lacks support for huge obvious use cases that we all agree need to be covered in a final release.

I think “whatever we can merge together right now without needing further debate” is a good dividing line for what goes into Phase 1 versus later on. And if Phase 1 is incomplete technically, that’s all the better, in that it reassures everyone that there will be later phases.

[michael-ciniawsky]

Supporting th main field for ESM at this point doesn't make a lot of sense in my point of view since 99.9% of existing main fields are CJS and one would have to explicitly write an ESM package to make it work, which I hope isn't the final intend. It's very confusing and in the worst case leads to folks starting to publish name-esm packages for 'testing proposes or 'because that's what node does now'. pkg.module is already a thing for tooling and while it shouldn't be final to support it in the future it would definitely be sufficient for testing atm. Otherwise it's better to not support anything related to package.json and the NRA (Node Resolution Algorithm) in the first iteration of the minimal kernel (for ESM)

[GeoffreyBooth]

I would like to see this document scoped to just the kernel.

I think this is the big question to work out. My feeling is that the minimal kernel is stalled because people don’t want to release it without their priorities in it, or at the very least a strong assurance that their priorities will be added soon. You already addressed that by putting bullet points on some of the “in the minimal kernel” items noting what other things are coming later that relate to that feature; why hide them in notes when we can be clearer? We can add some language saying that Phases 2+ are only tentatively defined yet. Would that make you comfortable?

I know that adding more text gives people more to argue over, but I think in this case we’ll get a Phase 1 released sooner with a tentative Phase 2 written out, rather than missing. We can always revise this further after my PR is merged into your PR, when it goes back to the larger group for feedback.

[MylesBorins]

My feeling is that the minimal kernel is stalled because people don’t want to release it without their priorities in it

I do not share the sentiment. I have not found the minimal kernel to be particularly stalled, what has happened is that no one has done the work.

I geniunely believe adding more phases to this is scope creep and will not land a PR that includes it. If you wish to open an alternative pull request or build consensus for this please do so on the main repo

[GeoffreyBooth]

I geniunely believe adding more phases to this is scope creep and will not land a PR that includes it.

Okay, I’ve removed it. Is the replacement text acceptable?

I can accept that I might be wrong about this. If no one complains that their priorities aren’t listed, then fine, we run with this and talk about later phases when Phase 1 is done. If my fears are correct, we can have a larger discussion with the full group over what we need to say regarding later phases. Does that sound okay?

[MylesBorins]

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

Unless we want to leave it in for posterity

[GeoffreyBooth]

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

[michael-ciniawsky]

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