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

Managing fork and agreed upon minimal kernel #166

Closed
MylesBorins opened this issue Aug 14, 2018 · 25 comments
Closed

Managing fork and agreed upon minimal kernel #166

MylesBorins opened this issue Aug 14, 2018 · 25 comments

Comments

@MylesBorins
Copy link
Contributor

Making this issue to track discussion and gain consensus around what a minimal kernel would look like in that fork.

@MylesBorins
Copy link
Contributor Author

what would we want to call the fork? node-esm?

@devsnek
Copy link
Member

devsnek commented Aug 14, 2018

how about a branch called esm, i'd like to still get eyes from collaborators who aren't node modules team members on our changes.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Aug 15, 2018 via email

@GeoffreyBooth
Copy link
Member

The other thing to discuss is what should go in Node core. Do we keep the current implementation there? Replace it with a minimal one that grows over time? Remove it and leave nothing, and various implementations are all relative to an ESM-less core?

@giltayar
Copy link

giltayar commented Aug 15, 2018 via email

@giltayar
Copy link

giltayar commented Aug 15, 2018 via email

@giltayar
Copy link

Let me suggest a minimum implementation. The idea of this minimum is to 1) have all ESM functionality included, and 2) avoid a debate on all the thorny issues.

So it includes:

  • the ability to import an ESM either with import from in an ESM file, or using dynamic import from a CJS or ESM

Avoiding the "file extension debate"

  • import will not guess the file extension and will import the file as ESM:
    • e.g. import ... from './foo.js' or import ... from './foo.mjs' will both import an ESM from the corresponding files because the extensions have no meaning. It could be ./foo.foo for all it cares! It is important to note that this will probably not be the end result, but is again, a minimal decision that will likely change, and that avoids discussing the file extension problem till we're ready.
  • If the thing being imported is a folder, it will look for package.json main property and will load that as ESM. Again, will not guess the file extension.
  • It will not look for index.js because that will provoke a "file extension debate", and we all want to evade that.
  • The regular CJS folder searching algorithm will be used (e.g. node_modules in parent dir...), except that it will not look for index.js (or index.mjs).

Avoiding the "transparent interop debate"

  • CJS will be able to dynamically import an ESM, because that is in the JS spec, and is mostly a consensus in the group (I hope...).
  • ESM will not be able to require a CJS file.
    • If this is too harsh, we can add the import.meta.require PR to enable this, as this is easily removable later if another interop mechanism is chosen.

That's it.

@devsnek
Copy link
Member

devsnek commented Aug 15, 2018

It will not look for index.js because that will provoke a "file extension debate", and we all want to evade that.

node looks for index.{any registered file extension} atm, will we still support that with your proposal?

@GeoffreyBooth
Copy link
Member

I think folder lookup can be something else that gets added later. If you really want to be minimal, you could also leave out package resolution, to allow for either the CommonJS require algorithm or the package name maps proposal.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Aug 15, 2018

Minimal kernel imho:

@devsnek
Copy link
Member

devsnek commented Aug 15, 2018

i would still say that any situation where we land without node resolution enabled by default is unworkable, and so it should be part of the kernel.

@MylesBorins
Copy link
Contributor Author

@devsnek I don't agree with you, and thus we don't have consensus. We can't remove it later and thus I think it shouldn't be in the kernel

@giltayar
Copy link

@MylesBorins - I'm assuming your minimal kernel also doesn't assume an extension, and the extension has to be explicitly in the name?

@MylesBorins
Copy link
Contributor Author

@giltayar indeed

@targos
Copy link
Member

targos commented Aug 22, 2018

@MylesBorins thanks for creating the repo. Should we start by pushing node's master branch to it?

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Aug 22, 2018 via email

@MylesBorins
Copy link
Contributor Author

Fork is up and running

--> https://github.com/nodejs/ecmascript-modules

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Sep 12, 2018

edit: to be explicit this is not something that would ever be shipping. The purpose of this kernel is to build a subset implementation that can be used to implement all features we are currently debating to allow pull requests for specific features or full proposals to build on top of the same code base (and likely share more code)

what is a minimal implementation

  • both browser + node need bare imports
    • implementation to be discussed
  • we cannot have 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
  • common.js backwards compat
    • createRequireFunction does
    • import.meta.require does not fail early enough
    • hold off on importing common.js until more progress is made on dynamic modules spec
  • .mjs will be the only way to import ESM
    • intention is to move forward with format databases to map extensions and support multiple use cases
  • only supporting importing ESM
    • no JSON
    • no native modules
    • createRequireFunction used to get these
    • will come back with format database

Steps to get there

  • removing importing of formats other than ESM
    • no common.js
    • no JSON
    • no nativemodules
  • removing of dynamic path searching
    • no extension adding
    • no directory resolution
    • no support for index.js
    • still maintaining support for the main field
  • adding createRequireFunction

@michael-ciniawsky
Copy link

pkg.module ? Or will pkg.main still lookup the file extension (as an exception)? This feels inconsistent/confusing if paths searching is removed...

⚠️ Note I'm not talking about support .js for ESM or the like, just how this is supposed to work

package.json

{
  "main": "./file" // What happens if e.g ./file.js || ./file.mjs is explicitly defined?
}

package.json

{
  "main": "./file.js",
  "module": "./file.mjs"
}

?

@MylesBorins
Copy link
Contributor Author

@michael-ciniawsky I think the intent was to support package.main as a means to specify the entry point of a module, and that this path would need to be explicit (no path searching).

We were debating if this should even be supported, perhaps it should be dropped.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Sep 12, 2018

here's a doc so people can comment and offer suggestions for changes. If you would like to suggest alternative proposals please do so in the issue

https://docs.google.com/document/d/1uq3v1h3dUXmjMAabQCsxdj7Ig2lOpaty1u4HcUi8whQ/edit?usp=sharing

@michael-ciniawsky
Copy link

So no 'dual-mode' packages (for now) ?

@MylesBorins
Copy link
Contributor Author

indeed. It is something that would be part of a future enhancement to the kernel, dependent on the feature set

MylesBorins added a commit to MylesBorins/modules that referenced this issue Sep 12, 2018
@MylesBorins
Copy link
Contributor Author

I've opened a PR to add documentation about our minimal kernel to the repo. Please comment / approve / reject in there rather than the google doc #180

MylesBorins added a commit to MylesBorins/modules that referenced this issue Sep 12, 2018
MylesBorins added a commit to MylesBorins/modules that referenced this issue Sep 12, 2018
@MylesBorins
Copy link
Contributor Author

We have a PR open to solve this, going to close the issue

MylesBorins added a commit that referenced this issue Oct 3, 2018
* doc: first pass at minimal-kernel

Refs: #166
@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
None yet
Projects
None yet
Development

No branches or pull requests

6 participants