Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Doc: Phase 2 for new modules implementation #196

Merged
merged 5 commits into from
Oct 25, 2018

Conversation

GeoffreyBooth
Copy link
Member

## Later Phases
## Phase 2

* A `--mode` field to enable ESM support in the cases of `--eval` and STDIN input, _or_ any file: `node --mode=esm index.js`.
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure “or any file” is something that has consensus.

Copy link
Member

Choose a reason for hiding this comment

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

(also this should probably clarify that the mode only applies to the entry point)

Copy link
Member

Choose a reason for hiding this comment

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

I also think that the name and value of “mode” should match whatever’s chosen for a package.json field, so I’m not sure those can be separated.

Copy link
Member

Choose a reason for hiding this comment

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

@ljharb The idea of the --mode item was to cover having a way to signal the parse goal. The specifics (name, values, etc.) are something to be ironed out. The reason the flag was chosen instead of other ways was that a flag is likely the most simple, in that realm, to iron out.

Copy link
Member

Choose a reason for hiding this comment

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

The challenge is that a mode flag would only cover the entry point; but a package.json field would have to cover any file; the former is easier but IMO should be constrained by the latter.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still skeptical of a flag for the entrypoint. This cannot be placed in a shebang. I'm ok with this being in phase 2, but we should add another entry in the Future Phases to resolve the problem Unix shebang usecase.

Copy link

Choose a reason for hiding this comment

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

I this brings us closer to a starting point without favouring any controversial preferences, but obviously this is more like the one equally hated (not preferred) by everyone 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest rewording to clarify we're talking about entrypoints and that the specifics of the flag are not yet defined. Suggested replacement text:

  • A startup flag to enable ESM support for the entrypoint code.
    • This will cover both:
      • STDIN input, e.g. node --mode=esm --eval
      • file input, e.g. node --mode=esm index.js

@MylesBorins
Copy link
Contributor

I would prefer to see the top level bullet points focus on high level functionality as opposed to the specifics of the implementation. For example, improving cjs interop in esm vs. ``createRequireFromURL`.

Specific implementations are going to be contentious but we can likely reach consensus around areas to focus faster than the implementation.

- Using this in `package.json` or being scoped to packages is put off for a later phase.
- This will be supplemented/replaced by more robust configurability such as designed in ([#160](https://github.com/nodejs/modules/pull/160)) in a later phase.

* `createRequireFromURL`, to complement the just-added `createRequireFromPath`.
Copy link

@SMotaal SMotaal Oct 9, 2018

Choose a reason for hiding this comment

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

@MylesBorins were you looking to rewrite this to something like:

  • Provide complimentary functions to existing ones in core which more suited for use in ESM modules in order to promote a level of interoperability with existing CJS modules.

## Later Phases
## Phase 2

* A `--mode` field to enable ESM support in the cases of `--eval` and STDIN input, _or_ any file: `node --mode=esm index.js`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest rewording to clarify we're talking about entrypoints and that the specifics of the flag are not yet defined. Suggested replacement text:

  • A startup flag to enable ESM support for the entrypoint code.
    • This will cover both:
      • STDIN input, e.g. node --mode=esm --eval
      • file input, e.g. node --mode=esm index.js

* Re-introduce VM module integration.
- Implemented in: https://github.com/nodejs/ecmascript-modules/pull/8.

## Future Phases
Copy link
Contributor

Choose a reason for hiding this comment

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

We now have two PRs in-flight that define future phases. The wording here seems good for Phase 2.

For future phases, I much prefer the intent-driven descriptions of Phase 3 & 4 in #193 rather than the feature-list style we have here because it allows people to first agree on and rally around why we the phases exist before we decide what to put in them. The actual wording doesn't matter so much - it's more about ensuring we're all clear on the purpose of the future phases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Re rewording the entrypoint, sure, I agree with that change.

Re the other PR, this PR is meant to supersede the other one. We included the Phase 2 items from that one in this one.

I don’t mind removing the Future Phases section, but I don’t think having vague language about the topics for each phase helps us much. I think phases can include PRs/features that span different topics, just as Phase 2 does.

Copy link

Choose a reason for hiding this comment

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

I honestly think that "future phases" make it possible to safely consider and discuss things so that when it is time to write "Phase n" we are collectively more comfortable with locking into those priorities.

* Remove current VM implementation

* Remove current Loader implementation

These changes are implemented in https://github.com/nodejs/ecmascript-modules/pull/6

## Later Phases
## Phase 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested intro line:

The purpose of Phase 2 is to introduce uncontentious UX enhancements before we deal with user-land extensibility ("loaders").

@MylesBorins MylesBorins added the modules-agenda To be discussed in a meeting label Oct 24, 2018
@MylesBorins
Copy link
Contributor

I've updated the PR with the changes we agreed upon in the meeting. We had consensus to land this PR in the current state. Can those from @nodejs/modules who were on the call add an LGTM that this reflects what we agreed upon in the meeting

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@zenparsing zenparsing left a comment

Choose a reason for hiding this comment

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

LGTM

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Oct 24, 2018

Maybe it doesn’t need to be added to the document, but here are the champions/interested parties for each point (subject to change, please edit this comment to add/remove your name):

Speaking for myself at least, if you’re planning on doing any design or coding work on any of these issues please reach out to the other interested members to collaborate and hopefully forge consensus.

@devsnek
Copy link
Member

devsnek commented Oct 24, 2018

i'd like to be included on the "virtual module from source" topic

## Phase 2

* Explore design space for virtual module from source
- Potential implementation in: https://github.com/nodejs/ecmascript-modules/pull/8.
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct link? Its title is 'Revert "esm: Remove --experimental-vm-modules."'.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed. We had an implementation that is available upstream that was removed in phase 1. A potential implementation is to simply revert removing it

@MylesBorins MylesBorins merged commit 329405e into nodejs:master Oct 25, 2018
@GeoffreyBooth GeoffreyBooth deleted the phase-2 branch October 26, 2018 00:05
@GeoffreyBooth
Copy link
Member Author

Can anyone opposed to working on a method for setting ESM mode for --eval/STDIN please explain your objections here? The end of the meeting was a bit rushed and I didn’t get what exactly needed to be changed to move forward on this. I think we all agree that there needs to be some way to use import and export in code input via --eval or STDIN, so what concerns do people have about starting work on this? Is there something else that needs to be implemented first?

@jkrems
Copy link
Contributor

jkrems commented Nov 4, 2018

Can anyone opposed to working on a method for setting ESM mode for --eval/STDIN please explain your objections here?

My basic issue was: I would assume this would reuse the same mechanism also used in shebang. If we add something that just works in the eval case but not the shebang case, it would be a temporary workaround or we'd end up with two solutions for eval which seems confusing. And for a temporary workaround something like run-esm.sh seemed good enough.

EDIT: To clarify, the only solution for shebang (outside of extensions) I'm aware of is "2nd binary [name]" unless somebody found another solution. Or we agree that extension-less, single file node executables would no longer exist post-CommonJS.

@GeoffreyBooth
Copy link
Member Author

@jkrems Are you saying that you want the same solution for --eval, STDIN, extensionless files and shebangs? I suppose a pragma in the code itself could do that. But why is it important that we have one solution for all these cases, as opposed to say a CLI flag for --eval and STDIN and another solution for extensionless or shebang files?

I feel like it should be okay to implement something and revise it later. If we add a --mode flag now, say, but then add that "mimes" configuration block later, we could rename --mode to --entry-mime or something for consistency. Or we could add a pragma and remove the CLI flag altogether. I don’t think each phase is engraved in stone, and we don’t need to worry about breaking changes between phases.

How about something like this?

  • An out-of-band mechanism for command-line disambiguation of source entry point parse goal. The method should cover --eval and STDIN, and ideally also shebang files and/or extensionless files.

If we can come up with a solution that works for all four, and people agree on it, great! If not, we come up with something for two or three of the cases maybe and move on, and maybe revise it later. What do you think?

@jkrems
Copy link
Contributor

jkrems commented Nov 5, 2018

@GeoffreyBooth The reason I'm pushing for the shebang case to be answered first is because it's the one with the smallest design space. And if we have #!/usr/bin/env node-esm, then a --mode flag becomes much less important. Because it would be relegated to "only important for non-JS entry points w/o extension". And I'm not convinced that --eval needs to work for WebAssembly binary code.

@GeoffreyBooth
Copy link
Member Author

I think we would want something like cat foo.wasm | node to work, i.e. passing in WASM via STDIN, because I can see a WASM-generating tool wanting to pipe its output into Node for execution; and enabling that is essentially the same as --eval. That’s also basically what happens for coffee foo.coffee—CoffeeScript transpiles the file into JavaScript source code, then runs it via node --eval. So --eval and STDIN aren’t just for debugging, they’re also used to link Node into chains with other tools.

So I think what we need is one or more solutions for all four cases, is that fair? If it’s one method that works everywhere, that would be ideal, but I think it’s more important to solve all four than have the same work for all four. They seem simple enough and related enough to be tackled together, would you agree?

  • One or more out-of-band mechanisms for command-line disambiguation of source entry point parse goal. The method(s) should cover --eval, STDIN, shebang files and extensionless files.

@jkrems
Copy link
Contributor

jkrems commented Nov 5, 2018

Do we know how often people asked to pipe a .node file to node (which is today's equivalent of the "pipe WASM" case). I'm a bit concerned about scope creep. Getting feature parity is nice but this feels like it goes beyond that. Would this also imply that node --content-type=text/purescript --some-param=purescript-transform -e "main :: IO () {}" would be supported? Today eval/stdin don't support .json or .node.

@devsnek
Copy link
Member

devsnek commented Nov 5, 2018

i think this problem will resolve itself when we figure the rest out. node stdin eval sits on top of the rest of the system, not the other way around.

@GeoffreyBooth GeoffreyBooth removed modules-agenda To be discussed in a meeting labels May 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.