nodejs / node-eps

Node.js Enhancement Proposals for discussion on future API additions/changes to Node core
443 stars 66 forks source link

Rewrite 002 - esm #39

Closed bmeck closed 6 years ago

bmeck commented 7 years ago

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:

unional commented 7 years ago

What about transitivity between the two?

bmeck commented 7 years ago

@unional it will not be possible unfortunately.

unional commented 7 years ago

cc/ @blakeembrey

trevnorris commented 7 years ago

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

bmeck commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

bmeck commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

xjamundx commented 7 years ago

@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 commented 7 years ago

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

martinheidegger commented 7 years ago

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 commented 7 years ago

@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 , https://github.com/nodejs/node-eps/pull/33#issuecomment-229679824). 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 commented 7 years ago

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

HyeonuPark commented 7 years ago

Desclaimer: I saw eps-002 so please read it before close it.

Not really sure this is the right place to write it, please tell me if not.

Currently dynamic import() proposal landed stage 2 in tc39 process. This spec allows to use import() expression even within script context.

So my idea is simple: import() for es module, require() for others.

In this case, there's really no additional boilerplates are required, for both legacy codes and bleeding-edge codes. You can just require() to load scripts, jsons, and native modules as usual. You also can import {foo} from 'module.js' to statically load es module within another module code. And also, you can even use import('module.js') within script code as an expression which evaluated to promise.

That's it. No .mjs, no "module": "index.js". I think this is the most elegant way to handle it.

I know this dynamic import proposal is not yet finalized. But as eps-002 is also stay in draft and not even started to be implement, I think we should discuss this before further implementation occurrs.

ljharb commented 7 years ago

@HyeonuPark That would force asynchronous evaluation across the module boundary, and it would not allow existing unchanged code that has require('foo') to continue working unchanged forever when the "foo" author refactors to provide ES modules. In addition, even with your suggestion the problem still remains of node needing to know whether a file is a module or a script, which brings us right back to .mjs.

HyeonuPark commented 7 years ago

@ljharb the semantic of require should not be changed for that reason.

If user types require(), traditional module loader is triggered, detects json/native module/es script via file extension, all operation unchanged.

If user types import ... from ... or import(), engine-native es module loader will parse given file as an es module.

And yes, users should update their codes when the library changed to the es module, just like when major version is updated. If they do not want to update their code, they can stuck in older versions. Of course libraries can support legacy modules via require('libname/legacy') like name.

ljharb commented 7 years ago

That's a dealbreaker. Old code must not need updating when a module switches module formats.

HyeonuPark commented 7 years ago

IMHO, updating library from script to module should be considered as breaking change(semver major version update) as it changes how user consume the library. This literally means api change - not only in semantically, but also in syntactic form.

If you don't agree with the statement above, please answer why changing syntax should be treated as non-breaking change.

ljharb commented 7 years ago

Because the syntax is an implementation detail that the consumer need not care about in the majority of cases. If I change from a function expression to a function declaration, but either way export the same function, then that change is not semver-major - it's unobservable. If upgrading to ES modules requires a breaking change, then a) many people will never do it, and b) the ecosystem will be forked between "modules that require a new node or browser" and "modules that work everywhere" - we don't want a Python 3 situation here.

jkrems commented 7 years ago

@ljharb There's no Python 3 situation unless there's no interop. Switching to ES modules will drop support for a bunch of browsers, many versions of node, older (all?) versions of browserify... It's almost necessarily a breaking change for every library.

And it's not like "switching to the ES6-equivalent has observable consequences" is without precedence. ES6 classes behave differently than the usual, hand-written ES5 equivalent. Differences that can be observed by users of the class. And switching to class syntax almost always requires a major version bump (for the compatibility reasons mentioned above). And every library depending on your code will also do a major version bump.

None of that means there's a Python 3 situation.

ljharb commented 7 years ago

@jkrems fair counterpoint. but every breaking change is added friction that interferes with adoption. Also, switching to class syntax only requires a major bump if you weren't properly using inheritance in ES5 (which is admittedly easy to do wrong) - you can also make the major bump by fixing your ES5, and then have a patch bump to switch to class syntax. Without the interop I'm talking about, there's no way to avoid the breaking change.

jkrems commented 7 years ago

Sorry, I should've been more explicit in my classes example: Even if you were doing ES5 class inheritance perfectly, you might have relied on things like enumerating methods on the prototype chain. And suddenly all methods "disappear" because a library switched to ES6 classes. Or you were expecting that the __proto__ of a class is always Function.prototype (bad code - but possible to write). Thankfully ES6 made some improvements to default semantics of JavaScript, I don't think those should be treated as annoyance that can or should be "optimized" away.

The reason why I objected to the Python 3 comparison is that it effectively turns this discussion into "my solution or you want to break the ecosystem". Which I don't think is fair. There's a tradeoff here between various degrees (and kinds) of pain and getting the most out of the changes that ES6 brings.

I'm honestly concerned about the "let's just magically switch the implementation of a library based on the version of node you're on, deep in the dependency tree". That to me sounds like a potentially ecosystem-breaking change on the day that new version of node rolls out. Because there's no way to predict what it will break (because that code never ran before). And at the same time it's not clear to me what benefits it would still have. If your ES6 module is "proxied" by a CommonJS one, no bundler can properly tree-shake it (not anymore than local dead code elimination could anyhow). So yes, you're running that code in node and it behaves exactly like before. But the only thing you gained is a bit of syntactic sugar. Which, to me, doesn't sound all that compelling given the risk implicit magic adds.

paulwalker commented 7 years ago

Can someone please tell me what the hold-up is now w/ ES6 module support? It seems as if there is argument about "breaking changes" just for "syntactic sugar," and that this diminishes the real desire of developers to move things forward without transpilation. Is it not a matter of when and how node supports ESM, correct?

bmeck commented 7 years ago

@paulwalker you might want to jump towards the end of the broadcast for https://github.com/nodejs/CTC/issues/59 . There are breaking changes that would require specification changes to JS to support the currently accepted draft. That draft is a synchronous loader that supports named imports from CJS. This PR is a secondary option should those things which act like babel no longer be viable. There were 2 main holdups until last CTC meeting, now there is 1.

bmeck commented 7 years ago

ah, to note: for a synchronous loader to be possible https://github.com/caridy/proposal-dynamic-modules needs to land in JS spec.

WebReflection commented 7 years ago

I am not sure if I should file a new EP document or add my proposal to this thread but since this is very similar to what I've done I'll give it a try.

module.import(path):Promise

It seems that everyone agreed at least in two points: the import name, and the Promise as "protocol".

Since it's impossible to polyfill a reserved word like import is, my solution is to bring the import method to the module object, and for the following reasons:

Implementation

Accordingly, the following 3 LOC are theoretically all it takes to enable asynchronous module import, as well as asynchronous module exports:

// Module as module.constructor
Module.prototype.import = function (path) {
  return new Promise(res => res(this.require(path)));
};

Considering a generic, good old fashioned, NodeJS module like the following one:

// modules as we know: converter.js
const crypto = require('crypto');
module.exports = {
  sha256: (str, secret = '') =>
    crypto.createHmac('sha256', secret)
          .update(str)
          .digest('hex')
};

It'll be completely transparent for developers to either include it synchronously:

// same good old synchronous flavor
const converter = require('./converter');
console.log(converter.sha256('yolo'));

or asynchronously:

// module.import asynchronous flavor
module.import('./converter')
.then((converter) => {
  console.log(converter.sha256('yolo'));
});

Asynchronous exports

The pattern automatically enables the ability to also export module asynchronously, simply returning a Promise as exports.

// generic async export
module.exports = new Promise(define => {
  // some asynchronous operation ...
  define({my:'module'});
});

// asynchronous converter.js
module.exports = module.import('./crypto')
.then(crypto => {
  // return the module to export
  return {
    sha256:(str, secret = '') =>
      crypto.createHmac('sha256', secret)
            .update(str)
            .digest('hex')
  };
});

As previously mentioned in this thread too, it's straight forward to require multiple modules too:

module.exports = Promise.all([
  './awe',
  './some'
].map(
  m => module.import(m)
)).then(modules => {
  const [awe, some] = modules;
  return {awe, some};
});

Last, but not least, I've already implemented a tiny utility that brings this ability to both NodeJS and browsers. You can use it as playground also importing by default from unpkg CDN whenever a module is imported, or required, without relative or absolute path.

Last part, is the only one I'd like to better think about, since current implementation works well but it's coupled with unpkg service, but I believe there should be a better mechanism to intercept upfront non relative paths and eventually map them to specific files.

I hope any of this can help moving forward with asynchronous module loader, this could also be the best way to polyfill, whenever TC39 will finalise it, the global import through transpilers.

Best Regards

bmeck commented 7 years ago

@WebReflection I would make a separate PR since that is not related to supporting ESM itself. It sounds like it just wants a standard polyfill for import(), doesn't appear to have any different features? To discuss:

WebReflection commented 7 years ago

Will open a new PR then. To quickly answer your questions:

Will try to file the EP ASAP, thank you.

targos commented 7 years ago

@nodejs/ctc Should we vote here about the asynchronicity of the ESM module loader? My vote is +1.

addaleax commented 7 years ago

I’m +1 on an asynchronous loader (sigh).

misterdjules commented 7 years ago

My vote is: abstain.

MylesBorins commented 7 years ago

+1 for async loader for import.

Primary motivation is to avoid behavior differences between node + browser

Is it reasonable to keep the door open for a sync loader in compatibility apis such as require('esm') or require.import('esm') In these instances we are not dealing with potential behavior differences, especially if the sync operation is using the async import implementation under the hood.

evanlucas commented 7 years ago

I'm -1 for async loader for import. IMO, large differences between node and the browser already exist and I feel that an async loader makes the transition from ESM syntax with babel more difficult. I think we should stick with a sync loader.

Trott commented 7 years ago

Vote count for async vs sync loader. (I'll try to keep this comment up-to-date)

Async: @targos @addaleax @MylesBorins @ofrobots

Sync: @evanlucas

Abstain: @misterdjules @Trott @mscdex @rvagg @bnoordhuis @Fishrock123 @thefourtheye

Minimum number of additional CTC votes needed to come to resolution: 3 CTC members who have not yet voted: @ChALkeR @chrisdickinson @cjihrig @indutny @jasnell @mhdawson @shigeki @trevnorris

(Probably doesn't make sense to add a vote count at the top because it will look like a vote count for the EP and not for the async vs. sync issue specifically. So putting it here.)

mscdex commented 7 years ago

My vote: abstain

ljharb commented 7 years ago

(non-CTC/TSC member here) I don't actually care if the loader is sync or async - async seems nice in that it maintains more parity with browsers. What I do think is very critical is that require('foo') work the same whether foo is CJS, or ESM - iow, I want "migrating to ESM" to be as seamless as possible, and not a breaking change. If this (ie, require('esm') not returning a promise, but just returning the default export or a Module Record) can be achieved with an async loader, then I think that's clearly the far superior approach (also, assuming that top-level await requires an async loader, which isn't something I grok yet).

@bmeck is there a possibility that async loading could allow require('esm') to return the default export or Module Record? Can you explain why top-level await is impossible with a sync loader?

ofrobots commented 7 years ago

My vote for 'should the module be sync or async?': 'async'.

Although we are not debating this issue yet, IMO require('esm') should throw. There is expectation of sync behaviour with require. A dynamic import() of esm from cjs would be fine by me.

ljharb commented 7 years ago

@ofrobots if it throws, then the net effect will be that nobody ever refactors their modules to use ESM, and ESM will be DOA.

rvagg commented 7 years ago

Abstain from me, I've had @Fishrock123 explain it in more detail to me but I still can't form an opinion because it seems the tradeoffs are too large either way.

Fishrock123 commented 7 years ago

if it throws, then the net effect will be that nobody ever refactors their modules to use ESM, and ESM will be DOA.

I do not think that the behavior of require() under an async loader matters at all. If anything, throwing would be better because it is explicit and we can tell the user what to do to use ESM.

ljharb commented 7 years ago

@Fishrock123 why would any module author (such as myself) want to migrate my module to ESM if it means that users using n ESM-supporting node wouldn't be able to use it with require? (ie, they'd be forced to refactor to ESM in order to consume my package)

I'll simply never refactor to ESM, and everything will continue to work perfectly, and that's what the majority of authors will likely do as well (since they tend to care about their users' ability to use their packages).

Fishrock123 commented 7 years ago

@ljharb If we implement an async loader you will have to refactor regardless.

So what is better then? Two possible behaviors of require() depending on what is in the file you are asking for? Or two APIs for for each type of module with explicit throws?

Fishrock123 commented 7 years ago

I'm going to abstain. All the options are pretty awful IMO and I don't really want to be responsible for signing off on any of this really.

ljharb commented 7 years ago

I don't know what's the best path - I'm just convinced that if consumers have to know or care about what module format I'm using, it's going to be a disaster for everyone. I'd rather node never ship ESM, than fork the ecosystem by forcing users to handle my chosen module format.

Fishrock123 commented 7 years ago

I'd rather node never ship ESM, than fork the ecosystem by forcing users to handle my chosen module format.

This is part of the reason why I had/have been a proponent of a Sync loader for so long...

Keep in mind you need to chose regardless if you are using an ESM with an Async loader because you will need to convert your entire load order to async.

Trott commented 7 years ago

No official vote resolution on the async vs sync loader, but the sense of the group at the meeting today is that CTC members (or at least the ones who feel informed enough to have an opinion) are horribly conflicted but are mostly leaning towards async. We would like to tentatively move that way to at least get a better idea of what an implementation would look like.