Skip to content
This repository has been archived by the owner on Jul 31, 2018. It is now read-only.

Rewrite 002 - esm #39

Closed
wants to merge 4 commits into from
Closed

Rewrite 002 - esm #39

wants to merge 4 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Aug 26, 2016

some discussions and problems with supporting various behaviors on a VM level, races with browsers, and upcoming spec changes have led to a drastic change in direction for the interop bridge.

https://gist.github.com/bmeck/52ee45e7c34d1eac44ce8c5fe436d753 has some relevant notes

notable:

  • back to file extension
  • more explicitly stated that ESM will not respect require.extensions in the algorithm
  • import is url based
    • if passed full URL, no path searching is performed
    • else if starts with ../ , ./, or / use as base URL against current script and search
    • else search in node_modules
      • added guards for potential web compat around ../ escaping node_modules
  • ESM cache won't be shared / placed in require.cache at all
  • ESM and import ALWAYS unwind stack prior to any evaluation
    • import('old.js') (spec being written) would always unwind the stack prior to evaluating.
  • require of ESM returns Promise<ModuleNamespace> always, never synchronous
  • no named imports from current modules (JSON / C++ / Node CommonJS)
    • all produce a single default export
  • changes to what module scoped variables
    • removed __filename, __dirname, require, module, exports, arguments
    • link to tc39 spec on this value

// esm ~= Promise => {
// default => result_from_evaluating_foo;
// }
es_namespace.then(

Choose a reason for hiding this comment

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

esm.then?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

@unional
Copy link

unional commented Aug 26, 2016

What about transitivity between the two?

@bmeck
Copy link
Member Author

bmeck commented Aug 26, 2016

@unional it will not be possible unfortunately.

@unional
Copy link

unional commented Aug 26, 2016

cc/ @blakeembrey


```js
// use.mjs
import {__dirname} from './expose.js';

Choose a reason for hiding this comment

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

Can't pluck __dirname from the default export :D

module.exports = __dirname;
import __dirname from './expose.js'

@trevnorris
Copy link
Contributor

trevnorris commented Aug 26, 2016

@bmeck

import is url based

What about the case where one can require('fs/') to get a module named 'fs' from node_modules instead of from the native set?

EDIT: nm. This looks to be covered with the import 'abc/123'; example.


### 3.1. Determining if source is an ES Module

A new file type will be used for ES Modules, `.mjs`. Files with this extensions
Copy link
Contributor

Choose a reason for hiding this comment

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

"Files with this extension" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@bmeck
Copy link
Member Author

bmeck commented Aug 27, 2016

@trevnorris ah, need to make explicit what looks up things in core... are we trying to preserve being able to load non-core using that trick? several rules are already changing here, possible to block people doing such.

@trevnorris
Copy link
Contributor

@bmeck I'm not sure if doing require('fs/') is officially supported or not. When I'm on my computer, will look at existing tests.

@vanwagonet
Copy link

All this use of ESM for modules makes me think if there is a new filename extension, shouldn't it be .esm?

@bmeck
Copy link
Member Author

bmeck commented Aug 30, 2016

@thetalecrafter ES is the moniker for the specification of JS, but people use JS in conversation when not talking about the specification itself. Keeping the file extension similar to .js with the JS moniker was taken into account while making the decision on file extensions. Some considerations were listed in previous versions of this EP: https://github.com/nodejs/node-eps/blob/5dae5a537c2d56fbaf23aaf2ae9da15e74474021/002-es6-modules.md#511-reason-for-decision

@martinheidegger
Copy link

@bmeck With https://github.com/bmeck/UnambiguousJavaScriptGrammar being accepted isn't the file-ending part obsolete? The problem now however is not that we can't identify what type a file is. It becomes a Ecosystem problem and I am not sure that is something Node/Core should be fixing.

@bmeck
Copy link
Member Author

bmeck commented Sep 6, 2016

@martinheidegger that was merged into the draft of the ESM proposal. The problem of determining [source text] type remains, and as stated in the current text of the proposal lack of acceptance by TC39 is in play here. The standard way to determine the contents of a file on any file system is to use a file extension. I have very little hope that my agenda item on TC39 will make any progress and am moving to what I see as the more realistic option.

@bmeck
Copy link
Member Author

bmeck commented Sep 6, 2016

I would also like to note existing talking points about differing from browsers being bad, accidental mode swaps (in part related to differing behavior) being bad, and security implications if there exists ambiguity.

@matthewp
Copy link

matthewp commented Sep 6, 2016

Would you be open to a flag that enables "everything is an ESM mode"?

@xjamundx
Copy link

xjamundx commented Sep 6, 2016

@matthewp from my understanding currently there are issues with the spec that would mean for example: native code doesn't work at all with es6 modules, many core modules can't work at all with es6 mode. I may be way off here, but I think those are some of the reasons why that could not happen.

@matthewp
Copy link

matthewp commented Sep 6, 2016

Ah, I wasn't thinking about core modules, ok then. :(

@martinheidegger
Copy link

martinheidegger commented Sep 7, 2016

I have very little hope that my agenda item on TC39 will make any progress and am moving to what I see as the more realistic option.

@bmeck I find it extremely sad to believe that. I have more hope tbh. Aside from that: If Node.js just assumes your proposal as accepted it might increase the chance for it to actually be accepted.

@bmeck
Copy link
Member Author

bmeck commented Sep 7, 2016

@martinheidegger Node can't move it to accepted unless the intent is to implement it that way, Node can implement the "spec extension" of requiring an import as noted in various places (https://twitter.com/awbjs/status/744700272190971904 , #33 (comment)). However, this has implications that are problematic and Node very much would not like to have, including the ones listed above. If accepted I highly doubt the stance would change given there are already people stating the lack of need to change things.

@martinheidegger
Copy link

martinheidegger commented Sep 7, 2016

I want to publically state that there is a need to change things. It is an embarrassingly simple solution that would fix a major problem by creating a little effort for a minority of users and makes everyone's life better. Worth to champion for. (I am not sure I said this enough: thanks for your efforts)

Edit: Also posted it on es-discuss

Fishrock123 added a commit that referenced this pull request Sep 8, 2016
@Fishrock123 Fishrock123 mentioned this pull request Sep 8, 2016

There is knowledge of breakage for code that upgrades inner package
dependencies such as `require('foo/bar.js')`. As `bar.js` may move to
`bar.mjs`. Since `bar.js` is not the listed entry point this was considered
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, a common community "best practice" (although not a universal one) is to always omit extensions when requiring - to allow for "bar.js" to become "bar/index.js" transparently, for example - and require('foo/bar') would not break under this proposal.

@michael-ciniawsky
Copy link

@bmeck Will something like module.import() or require.import() be implemented ? 😕 I think they are adding even more noise and confusion. Nice work so far, the proposal is becoming much cleaner, still emphasizing to reconsider .mjs if possible 😛

as long as poly packaging works

? Is package.json not 100% of the table, or I'm not getting it 🙃 ?

@bmeck
Copy link
Member Author

bmeck commented Mar 1, 2017

@michael-ciniawsky any solution that encapsulates use cases and is not prohibitive to adoption or education is open for discussion. Defense of .js is a very well thought out solution, but remains complex in order to achieve high use case coverage.

As per *.import() I don't think it will be shipped in light of import() coming down the pipe.

[edit] note that a goal is hopefully to allow a path that all new code can be written in ESM without causing a perpetual burden to either Node or Browsers.

@michael-ciniawsky
Copy link

As per *.import() I don't think it will be shipped in light of import() coming down the pipe.

👍

What parts of Defense for '.js' are prohibitive ?

but remains complex in order to achieve high use case coverage

?

@bmeck
Copy link
Member Author

bmeck commented Mar 1, 2017

What parts of Defense for '.js' are prohibitive ?

  • Not all packages have "main" (somewhat rare)
  • Hesitancy towards package.json changing how files are interpreted. Means checking package.json becomes something to do if you ever are in doubt. If code uses import() the intention is still unknown.
    • part of hesitancy is that not all CJS can upgrade
  • "module" swaps the whole package unless you have a "main". kind of odd if you add a "main" later on to support CJS.
  • "module.root" changes pathing behavior in odd ways sometimes, like how import("../") wouldn't escape your package it would escape the "module.root"
    • this seems a bit confusing as it does add a pathing manipulation, confusion when reading import "foo/bar" and it not mapping to disk.
    • doesn't allow easy mixing in the same directory (if porting a large app you have to place all ESM in the root and all CJS outside the root)
    • an interesting and useful feature even w/o "module", question of if it is in scope
  • File based CLI usage without package.json
  • Learning behaviors
    • differing way to determine/express how to run things w/ node and browser that are not readily visible (must learn both)

As has been stated many times, the decisions here are about weighing cost/advantage of all the things. There are also downsides to .mjs that have been discussed as well. The prevailing thoughts have been that it is the simple and does not leave lasting persistent burden when moving between environments or learning for the first time.

@unional
Copy link

unional commented Mar 1, 2017

"module" swaps the whole package unless you have a "main". kind of odd if you add a "main" later on to support CJS

IMO as things moving forward, this scenario would be rare. 🌷

@michael-ciniawsky
Copy link

michael-ciniawsky commented Mar 1, 2017

"module" swaps the whole package unless you have a "main". kind of odd if you add a "main" later on to support CJS.

That shouldn't be the case of course, module applies where it is set. Everything else is treaten like main (CJS)

"module.root" changes pathing behavior in odd ways sometimes, like how import("../") wouldn't escape your package it would escape the "module.root"

module.root is awkward. Would it be mandantorily needed? e.g => module: [ entry.js, lib ] etc.

doesn't allow easy mixing in the same directory (if porting a large app you have to place all ESM in the root and all CJS outside the root

kk, is separation not even better ? Why in root only, is in different directories not enough?

File based CLI usage without package.json

node --module module.js

$HOME/.noderc

node module.js

{
   "module": true // node => node --module
}

@bmeck
Copy link
Member Author

bmeck commented Mar 1, 2017

@michael-ciniawsky

module.root is awkward. Would it be mandantorily needed? e.g => module: [ entry.js, lib ] etc.

There were discussions of using glob like patterns to whitelist which files should be treated as ESM. This is the "modules" field I didn't bring up in last comment.

$HOME/.noderc

Would be adding a file in a manner that we haven't seen before. Unsure how core would feel about making a settings file standard. You could bring it up though, might be useful even w/o ESM.

kk, is separation not even better ? Why in root only, is in different directories not enough?

This is complicated, it depends on what you setup in "modules" if you have "main". Reading the proposal would explain this.

File based CLI usage without package.json

--module is also needed for .mjs for STDIN/-e/-p, but .js for package.json based approaches do not give information on how files are supposed to run. Reading the file to figure out if it is ESM or not is necessary, even then if ambiguous, check the docs.

On package.json: we add 3 new fields in package.json (which are interdependent), potentially a settings file, and still gotchas about figuring out if a .js is supposed to be one mode or the other.

On file extension: we add a character to your file, and make sure to update code editors etc. to know about the MIME.

On pragma / grammar change: we are similar to file extension (w/o MIME question), but won't get it through standards. Questions remain about ambiguity if not mandatory.

@Vanuan
Copy link

Vanuan commented Mar 12, 2017

if consumers have to know or care about what module format I'm using, it's going to be a disaster for everyone

As a module consumer (I've never written any npm packages), I would rather be aware which module format I'm using than be bitten by unexpected bugs. For example, webpack supports AMD module format and it has already caused issues for me (leaking global).

If I'm maintaining an old codebase (requires all over the place), I would rather expect module authors to pubish a commonjs version if there's a will to support older version of node. If module author doesn't support new versions of node, I'd still be forced to not upgrade to a new node since there's no guarantee it will work the same.

If I'm mainaining a new codebase, I would rather expect new version of an npm package to contain es6 version of a module rather than having to append -es in every import line.

Would it be reasonable to expect a --disable-commonjs flag for people that want the cleanest and freshest version of node? Even if it means that their choice of libraries would be limited.


The above is from a consumer perspective. Producers would probably want their packages to work indefinitely without any maintenance.

@bmeck
Copy link
Member Author

bmeck commented May 1, 2017

web UI problem caused me to squash, old branch is at https://github.com/bmeck/node-eps/tree/rewrite-esm-bak

@targos please re-review


1. Apply the URL parser to `specifier`. If the result is not failure, return the result.
2. If `specifier` does start with the character U+002F SOLIDUS (`/`), the two-character sequence U+002E FULL STOP, U+002F SOLIDUS (`./`), or the three-character sequence U+002E FULL STOP, U+002E FULL STOP, U+002F SOLIDUS (`../`)
1. Let `specifier` be the result of applying the URL parser to `specifier` with importing location's URL as the base URL.

Choose a reason for hiding this comment

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

github's markdown renderer is confused by 2-space indent... this renders as 3. instead of an indented 1. 😅

maybe 4 spaces makes it happier?

1. Let `dir` be a new URL from `specifier`.
2. If `dir` does not have a trailing `/` in its pathname append one.
3. Let `searchable` be the result of applying the URL parser to `./package.json` with `dir` as the base URL.
5. If the resource for `searchable` exists and it contains a "main" field.

Choose a reason for hiding this comment

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

Missing step 4 (renumbered?)

5. If the resource for `searchable` exists and it contains a "main" field.
1. Let `main` be the result of applying the URL parser to the `main` field with `dir` as the base URL.
2. If it does not throw an error, return the result of applying the package main search algorithm to `main`.
6. If it does not throw an error, return the result of applying the index main search algorithm to `dir`.

Choose a reason for hiding this comment

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

The section this references is named just "Index search"

4. If it does not throw an error, return the result of applying the file search algorithm to `searchable`.
5. If it does not throw an error, return the result of applying the directory search algorithm to `searchable`.
6. Let `parent` be the result of applying the URL parser to `../` with `package` as the base URL.
7. If it does not throw an error, return the result of applying the module search algorithm to `specifier` with an importing location of `parent`.

Choose a reason for hiding this comment

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

/../ is still a valid path (.. in / just points to itself)

Copy link
Member Author

Choose a reason for hiding this comment

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

/../ in URL parser becomes / in step 6.

Choose a reason for hiding this comment

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

It still won't throw an error, though, so this looks like it could loop forever (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Would fail in step 1 when it tries to grab the directory of parent which is /. Can clarify in step 1 by saying if the parent is same as importing location it should error.

When a `package.json` main is encountered, file extension searches are used to
provide a means to ship both ESM and CJS variants of packages. If we have two
entry points `index.mjs` and `index.js` setting `"main":"./index"` in
`package.json` will make Node pick up either, depending on what is supported.

Choose a reason for hiding this comment

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

I take it that there is no intent to support the ad-hoc module field.

(it's better that way, tbh; module is "specified" to require conversion to ES5 for example)

Copy link
Member Author

Choose a reason for hiding this comment

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

no plan

1. if there was an error, place the error in the ESM cache and return
2. let `export` be the value of `module.exports`
3. if there was an error, place the error in the ESM cache and return
4. create an ESM with `{default:module.exports}` as its namespace

Choose a reason for hiding this comment

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

Will this be a bound export, or will it snapshot the value of module.exports at caching time?

Copy link
Member Author

Choose a reason for hiding this comment

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

snapshot right then and there, grabs the value just like step 4. does not make a getter or anything.

Choose a reason for hiding this comment

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

So it's as if it was wrapped with a module that did export default require('CJS')

Copy link
Member Author

Choose a reason for hiding this comment

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

roughly


These have `[[Set]]` be a no-op and are read only views of the exports of an ES
module. Attempting to reassign any named export will not work, but assigning to
the properties of the exports follows normal rules.

Choose a reason for hiding this comment

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

It's also not possible to add any extra keys


### 3.2. Determining if source is an ES Module

A new file type will be recognised, `.mjs`, for ES modules. This file type will be registered with IANA as an official file type, see TC39 issue. There are no known issues with browsers since they [do not determine MIME type using file extensions](https://mimesniff.spec.whatwg.org/#interpreting-the-resource-metadata).
Copy link
Member

Choose a reason for hiding this comment

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

Link for the TC39 issue?

@targos targos dismissed their stale review May 2, 2017 07:33

outdated

@targos
Copy link
Member

targos commented May 2, 2017

What about direct execution of ESM code from the CLI? Should we specify this here?
I'm thinking about node -e "import foo from 'bar'; foo();"

@bmeck
Copy link
Member Author

bmeck commented May 2, 2017

@targos I've left the CLI flags out of this EP as it is not something I think belongs in the EP itself. The general consensus is that you must include a --module flag to change goal of source passed via argv or STDIN

@bmeck
Copy link
Member Author

bmeck commented Jul 19, 2017

landed in 6cc060e

@bmeck bmeck closed this Jul 19, 2017
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.