whatwg / loader

Loader Standard
https://whatwg.github.io/loader/
Creative Commons Zero v1.0 Universal
610 stars 45 forks source link

Node core dev thinks loader will not work for Node #54

Open domenic opened 9 years ago

domenic commented 9 years ago

See https://github.com/WebAssembly/design/issues/256 where @trevnorris outlines his issues. I think it would be useful to discuss them in this repo. I am pretty sure they are not quite accurate (for example you can definitely do conditional loading at runtime, by customizing the loader), so it would be good to address such misconceptions and to make sure all use cases are covered.

guybedford commented 9 years ago

Yes they mostly seem incorrect arguments. @caridy @dherman it would be great if one of you could answer these questions. I'll chime in the next day or so otherwise.

bmeck commented 9 years ago

@domenic from a lot of what I have heard/talked to people Loader would be instrumented as part of the es6 loading mechanism; which is where these concerns come from, having a module/global Loader object is somewhat benign but does have an odd case of when async scripts get evaluated by loader (on the microtask queue?) vs require doing them at point of require.

bmeck commented 9 years ago

as per the conditional loader, it is more limited since the resolution is part of the loader, you cannot have easy user logic like if (!Promise) require("bluebird") to my knowledge

trevnorris commented 9 years ago

@domenic Conditional loads, which require a customized loader, don't give the same flexibility as the user being able to freely define the path at runtime. Also, that comment was about the "import syntax in general", not about loader specifically.

The point is valid, but it didn't have anything to do with loader specifically. Was only mentioned to point out issues of common practices in node (e.g. lazy loading). It shouldn't have been included in the bullet points. Apologies.

matthewp commented 9 years ago

@bmeck @trevnorris I don't think you guys are wrong, particularly about performance side-effects from an asynchronous loader.

But conditional loading can be done at runtime by the user. They cannot use the import syntax for this, but they could do something like:

if(needThing) {
  let thing = await loader.import("thing");
}
bmeck commented 9 years ago

@matthewp for places that support await , which is not available at top level or module level of the https://github.com/tc39/ecmascript-asyncawait proposal

matthewp commented 9 years ago

Hm, I don't see that in the spec but will take your word for it. Nevertheless all places support Promises and will be able to be load modules with a loader.import form regardless of whether a friendly syntax is available or not.

trevnorris commented 9 years ago

@matthewp If there is no performance advantage, is breakage with the existing ecosystem and added complexity, why would we consider implementing it the server-side? I'm at a loss to find any advantage over the current method.

matthewp commented 9 years ago

@trevnorris Because the ecosystem is JavaScript, not just Node, and that ecosystem is already broken. I would suggest that we find a way to address the legacy needs so that JavaScript can have a module system rather than multiple incompatible ones for each host environment.

trevnorris commented 9 years ago

@matthewp If the spec requires import to be async, then we'll just program around it. Just like we've had to program around other parts of the spec so JS can be reasonably used on the server. The ecosystem uses what works best, and we're not constrained like the browser so the flexibility of getting around useless (to us) parts is easy.

bmeck commented 9 years ago

@matthewp https://tc39.github.io/ecmascript-asyncawait/#modified-productions as per the lack of it at a module level

We would love to have it work with existing code! Which is why we are here, to discuss what measures can be taken to do so. We just need to be informed to what the advantages vs. incompatibilities are. There seems to be a lot of disregard or putting the burden on existing code to change which only makes our concern grow. I think a good first step is to try and find issues that are fairly simple to reproduce like the circular dependencies and discuss them as @domenic said.

matthewp commented 9 years ago

I would think you would be able to keep backwards compatibility with require using the loader.install API.

bmeck commented 9 years ago

@matthewp Still would not fix circular dependencies or the timing concern of task queue vs immediate side effects as I read it. Maybe you can elaborate?

matthewp commented 9 years ago

That's true, any module being required would need to go through the Loader APIs as a module.

I can't think of a reason why the loader hooks have to be async. If you used a synchronous Promise you should see side-effects occur in the same order as they do today.

bmeck commented 9 years ago

@matthewp currently there are multiple stages of promises during the evaluation phase that would have to be placed on the task queue if we use a fulfillment handler :

caridy commented 9 years ago

@trevnorris @bmeck ...

@domenic Conditional loads, which require a customized loader, don't give the same flexibility as the user being able to freely define the path at runtime.

Conditional loading does not requires a customized loader or any sort, you can perfectly use the runtime built-in loader which will be accessible from any module executed in the runtime, and available as global somehow.

caridy commented 9 years ago

Can someone provide more details about the circular dependencies issue, I don't see the problem :)

bmeck commented 9 years ago

Other thread that you have been replying to contains the comments and relevant links:

On Fri, Jul 10, 2015 at 6:45 PM, Caridy Patiño notifications@github.com wrote:

Can someone provide more details about the circular dependencies issue, I don't see the issue :)

— Reply to this email directly or view it on GitHub https://github.com/whatwg/loader/issues/54#issuecomment-120551704.

trevnorris commented 9 years ago

@caridy

you can perfectly use the runtime built-in loader

Let's work through a hypothetical scenario.

Take modules A, B and C. Such that C imports B and B imports A.

C declaratively imports B B declaratively imports A

Simple enough. Until:

A must now imperatively import another module to finish its export B needed to create a new instance of A immediately, and now must wait C is wondering what happened and why it has to change from declarative to imperative

When attempting to simulate synchronous imports it only takes one module in the chain to require an imperative import to then force the remaining to do the same. This means developers will have mixed declarative and imperative imports in the header to accommodate each module. This is added complexity that devs won't want to deal with.

caridy commented 9 years ago

A must now imperatively import another module to finish its export

@trevnorris I don't fully understand this statement. export declaration are "always" declarative, and bindings per export declaration are created during parsing, not need to wait for something to be imported (declarative or imperative) like node does. I don't see why B or C will have to change at all. Can you elaborate more?

Are you talking about something like this for A.js?

let thing = await import.default('thing');
export {thing};
trevnorris commented 9 years ago

It's becoming difficult to track the discussion both here and on WebAssembly. So I'm migrating all my comments here.

I would like to clarify something. There are comments about loading the files synchronously from disk, but will that still be done within a Promise executor?

guybedford commented 9 years ago

@trevnorris sure, I mentioned that the pipeline allows hooking any loading style for CommonJS (including synchronous loading and execution). The way that would work is that instantiate gets called for a top-level module, that the loader knows from other means (meta configuration) is CommonJS. It then calls into the custom CommonJS loader, which doesn't need to communicate at all with the ES loader pipeline - cjsLoad(entryPoint) or whatever. Once the module (and all its dependencies) are loaded, executed and defined by that custom pipeline, it just returns the defined module. The only promise wrapping is the standard one around the top-level entry point. The same principle applies for ES modules and CJS without a dynamic loader.

bmeck commented 9 years ago

There is miscommunication going on about all of this; I will be on a Google Hangout on Air today from 1PM CDT 14 Jul 2015 to discuss with anyone who wishes to.

Event Link: https://plus.google.com/u/0/events/cin8ppflalbfnsi9ilegfugjnu8

Hangout Link: https://plus.google.com/hangouts/_/hoaevent/AP36tYfG4iSLuHeoT4iah5E_Yeqz6eFnsGFOVCHOMrlySzEIdy4T8A?authuser=0&hl=en

On Tue, Jul 14, 2015 at 5:17 AM, Guy Bedford notifications@github.com wrote:

@trevnorris https://github.com/trevnorris sure, I mentioned that the pipeline allows hooking any loading style for CommonJS (including synchronous loading and execution). The way that would work is that instantiate gets called for a top-level module, that the loader knows from other means (meta configuration) is CommonJS. It then calls into the custom CommonJS loader, which doesn't need to communicate at all with the ES loader pipeline - cjsLoad(entryPoint) or whatever. Once the module (and all its dependencies) are loaded, executed and defined by that custom pipeline, it just returns the defined module. The only promise wrapping is the standard one around the top-level entry point. The same principle applies for ES modules and CJS without a dynamic loader.

— Reply to this email directly or view it on GitHub https://github.com/whatwg/loader/issues/54#issuecomment-121192353.

caridy commented 9 years ago

I will be there...

guybedford commented 9 years ago

@bmeck thanks for bringing a discussion together. I won't be able to make it unfortunately, but will follow the notes!

matthewp commented 9 years ago

@guybedford I think it's still a problem that ES modules can't be loaded synchronously. Users are used to doing:

if(needThing) {
  exports.thing = require("thing");
}

With ES modules they could do this with:

var thing;
export thing;

if(needsThing) {
  thing = await loader.import("thing");
}

So any code importing this module can't use "thing" right away.

Maintaining compatibility with CommonJS is not enough. We need to maintain compatibility with Node coding style.

caridy commented 9 years ago

@matthewp I used multi-lines just to facilitate the discussion, but you can always go 1-liner. Btw, just a side note: I haven't seeing a lot of that (if (needsThing) at the top level) in the wild, that's more of conditional loading. Instead, I see a lot of this:

module.exports.thing = function () {
     var something = require('something');
     something();
}

That's the one that I'm more interested to, since it represents the ability to load things on demand.

matthewp commented 9 years ago

That's the one that I'm more interested to, since it represents the ability to load things on demand.

If that were an ES module that would become:

export function thing(){
  let something = await loader.import('something');
  return something();
}

So any code consuming this module must await the result of calling thing(). Please correct me if I'm really wrong here, loader.import will always return a Promise or no?

It's the usual problem of once something becomes async there is a snowball effect causing everything else to need to as well. I don't know if the Node community will like/accept this.

trevnorris commented 9 years ago

I don't know if the Node community will like/accept this.

I appreciate this concern for the module ecosystem. You're assessment of the async snowball effect is correct. This would be a significant deterrent preventing adoption.

It needs to be understood that even after this lands in V8, the existing module loading mechanism will continue to work and to be used. If this spec breaks interoperability with the now 170k modules it's likely to gain little adoption.

Any type of asynchronous behavior (i.e. needing to use a callback or await) breaks current behavior. By synchronously loading a dep in the Promise executor it may possibly work if dependencies are only one level deep, but after that it's hosed.

While Loader loads one level of modules at a time, currently modules are loaded along each dependency branch until it reaches the leaf. Where it then unwinds until another dependency is required. The difference between the two is significant.

guybedford commented 9 years ago

@trevnorris certainly any minor break in behaviour is not acceptable for an upgrade path and will inhibit adoption of ES modules in Node. If it helps to clarify, here is an example loader hook implementation that would be backwards-compatible in Node:

hook('resolve', function(key, parent) {
  // check file, file.js, file.json, package.json etc etc
  // this can be sync, but yes it is wrapped in an async wrapper function
  return nodeLookup(key, parent); 
});
hook('instantiate', function(key) {
  // already determined during lookup process from package.json
  if (isCJS(key)) {
    // exact Node require, returns Module instance object
    return Module(nodeRequire(key));
  }
  else {
    return undefined; // ES6 module
  }
});

The above pipeline would support loading CJS from within ES6 and dynamic imports of CJS via the module loader.

The nodeRequire above is exactly the NodeJS require function. In no way does it have to expose its internals to the loader. That is, the CJS loading pipeline remains synchronous and backwards compatible. The loader pipeline only loads one module - entrypoint.js, the CJS require does the rest. In the browser we load CJS asynchronously yes, and with the limitations of that approach, but that is only because of the nature of the browser environment. Sorry if I'm explaining this badly, feel free to ask any questions further.

trevnorris commented 9 years ago

@guybedford Thanks for example.

So you're saying that require() will basically take over how modules are imported? From what I understand the entire application will essentially be loaded from nodeLookup().

guybedford commented 9 years ago

Sure, hope it is making some sense. In the example, require as called above would only call on CommonJS modules. nodeLookup only applies to any CJS module loaded through an ES import or via a dynamic loader import. nodeLookup would also apply to all ES modules (based on assuming for simplicity that ES modules follow the current Node resolution algorithm in Node). The nodeRequire would only apply to a top-level CJS module (which in turn would load the CJS tree outside of the loader pipeline).

bmeck commented 9 years ago

@guybedford this causes a problem as having 2 pipelines means if you make a module built with import/export that uses require all modules that require uses must be CommonJS Node require: and we will end up with: "this module works if it was loaded via import"

bmeck commented 9 years ago

Meeting notes 14 July 2015:

CommonJS module loading of a WHATWG module

Assume WHATWG loader semantics with the following 2 files:

main.js

let dep = require('dep.js');
console.log(dep.a);

dep.js

import thing from "thing";
export const a = "a";
  1. The synchronous require in main.js will load dep.js which in turn would use the Loader pipeline.
  2. The loader pipeline would need to wait on the promises [[Fetch]], [[Translate]], and [[Instanciate]] prior to returning from require.
  3. Doing so uses the PromiseJobs Job Queue which requires unwinding of the stack prior to execution of a new job.

This is unable to be performed safely or synchronously according to spec and concerns about side effects from global mutation.

@caridy mentioned unknown communique about potentially using an empty set of exports during synchronous loading. This breaks CommonJS expectations and is not a viable solution.

Potential convergence specs violation allowed

Preventing run to completion in order to allow the PromiseJobs job queue to perform work is a possible solution to the CommonJS incompatibility. Currently V8 does allow interruption, but would not allow draining of the PromiseJobs.

This would require breaking existing constraints of VM and spec in order to accomplish.

Timing concerns from @trevnorris as node environment requires some knowledge of timing in order to load module system piece by piece.

Promise timing

@bmeck expressed concerns about the order of operations for PromiseJobs with respect to Loader fulfillment versus fulfillment of promises queued during [[Instanciate]]. Current spec does not appear to differentiate the promises and the following code is an example:

main.js

import foo from "foo";
console.log("main");

foo.js

Promise.resolve("foo-promise").then(console.log);
console.log("foo");

Would appear output:

foo
foo-promise
main

But it is unclear from current text if this pre-emption is intentional.

Circular dependency

It appears no circular dependencies are possible in the current text: https://github.com/whatwg/loader/blob/29464782eef807547fb0dd444f4eef1c3680580d/index.bs#L664

Seems like an error.

More talk about CommonJS

See above.

Talk

There was a comment by @dherman about web needing async loading, dissent from @bmeck related to HTTP2/existing workflows.

Discussion about if the current spec is a viable target for node with the current text. Disagreement.

Synchronous alternative for node

It was @bmeck and @trevnorris 's understanding that ECMA/TC39 was not involved in the WHATWG Loader. Using the information from ES spec, https://people.mozilla.org/~jorendorff/es6-draft.html#sec-source-text-module-records : The realization that a 1-1 mapping of require to was possible in ES2015 modules. Under that assumption a synchronous loading mechanism was internally discussed under similar semantics to the WHATWG loader. The pipeline workflow is compatible with WHATWG, but would use synchronous process that does not require unwinding of the stack in order to reach the job queue.

@dherman pointed out that this may be a good approach to look into. Thinks there might be an inability to use the Loader based pipeline hooks (needs more conversation).

Video Diagram

There appears to be a need for further discussion and a time next week should be determined for such. Send schedules if you wish to participate in a follow up next Tuesday-Friday; email bradley.meck at gmail

bmeck commented 9 years ago

Regarding the spec violation possibility, having a separate Promise Job Queue that does not mandate the stack to unwind would put a very clear possibility of this, but would require all node usages to use some form of Promise creation that includes the fulfillment handler at construction time (none known to me). Might be too complex.

guybedford commented 9 years ago

@bmeck great points - this is the hard part :)

For the case where CommonJS loads ES6, exactly this would require extending the CommonJS loader to introduce its own static checking, which when detecting that the tree has an ES6 dependency, will call back to the loader to load the ES6 module, before then executing the module synchronously along with its CJS requires.

bmeck commented 9 years ago

@guybedford can you clarify "static checking" we will not be parsing to see how require is called and the exact parameters as they could be a variable.

guybedford commented 9 years ago

@bmeck exactly it hits some edge cases, but can be allowed for non-variable requires.

bmeck commented 9 years ago

@guybedford there are a fair number of module/plugin systems that use require('fs').readdirSync to determine loading. We cannot support a system that requires such, browserify and noda face the same problem (solved via different means).

trevnorris commented 9 years ago

Quick example of why allowing the job queue to be processed normally during script evaluation is dangerous and littered with black magic:

(function runner() {
  Promise.resolve(1).then(function() {
    process.nextTick(runner);
  });
}());
setImmediate();  // never reached

node forcefully runs the job queue before the stack has unwound completely. This is because we need to 1) make sure if any promise resolvers run nextTick(), they will again be evaluated before the event loop continues; and 2) we have 'unhandledRejection' callbacks that are called before the event loop is allowed to continue.

This will completely mess with how modules currently work. Because devs use nextTick() to queue callbacks to be run after all synchronous code execution has completed but before the event loop has proceeded.

It also isn't something we can simply turn off. Script evaluation is evaluation. Whether it's the first or last run. And event if it were somehow turned off for the first execution, it wouldn't prevent further collateral damage on a late require.

What I'm basically getting at is running any part of module loading within the async job queue is fundamentally impossible.

bmeck commented 9 years ago

This might be its own topic, but where is the mandate that we must use Promises when making a spec? Many comments appear to want to use Promises, but as stated above this causes semantic issues due to enforced temporal locality after the stack unwinds.

This is going to take a bit, and cause some knee jerk reactions, but please read this thoroughly.

I think the pipeline, hooks, and registry can work without Promises.

This would mean interoperability with Node could be very close.

Why are [[Fetch]], [[Translate]], and [[Instanciate]] on registry entry Promises? Do we need to keep the cached value for future usage after the first fulfillment handler, in what case is the fulfillment handler not setup prior to fulfillment? Is there a need for fulfillment to happen on the PromiseJobs job queue? Is there a requirement on using Promise that is not written down publicly?

Is it possible to use a callback for the loader hooks, to specify a well known way to generate a value/error? This would not define temporal locality of fulfillment, and would allow both synchronous and asynchronous contextually based upon if we are in a synchronous module loading situation without resorting to static analysis or breaking specifications. Under the hood you could even use Promises (in the async case), but the key is that unwinding the stack would not be required.

As for linking, we have the [[State]] which can be used to determine if the value (currently [[Instanciate]]) is "ready" or if a function needs to be registered for invocation on fulfillment. Right now we mandate a Promise fulfillment handler callback which defines async temporal locality.

This sounds like a big change, but I doubt it is since all of the changes would be moving from Promise to a different fulfillment system.

caridy commented 9 years ago

@bmeck that's a fair question, we will look into that.

bmeck commented 9 years ago

Have not gotten any emails about scheduling, setup a Doodle to figure out best time for follow up. If anyone intends/wants to be involved in the follow up please fill out quickly so we can schedule the call in the next day.

bmeck commented 9 years ago

Follow up call will be at 1PM-2PM CDT Thursday, adjusted event accordingly.

CC: @caridy @guybedford @trevnorris

bmeck commented 9 years ago

Links for todays call:

Event: https://plus.google.com/u/0/events/cpvmf3joqg50m0794vlm71li7g4

Hangout: https://plus.google.com/hangouts/_/hoaevent/AP36tYfwbz2vFfvssSRQYhQn90vGrLIWOlihNjBr5PcVqeWtYiAZzA?hl=en

dherman commented 9 years ago

Here's what we came to on the call:

The main areas where Node/Browser module portability is valuable are:

The intercession API is less important to be portable:

I'll reach out to a few stakeholders to help work out a good path for Node to implement ES6 module support without needing to support the async pipeline. And we'll want to refactor the registry introspection API to avoid the coupling to the promise pipeline, so that Node can support a compatible API for that.

matthewp commented 9 years ago

the principal use case of the hooks is really EWM

What is EWM?

wycats commented 9 years ago

@matthewp The Extensible Web Manifesto. I think what @dherman is saying is that the loader hooks make it possible to self-host functionality that otherwise would be hardcoded into the browser. This makes it possible for libraries to prototype functionality that could go into a future version of JavaScript or the DOM APIs, and is a key way that the web has remained a usable platform despite periods of stagnation in various areas.

caridy commented 9 years ago

partially addressed by https://github.com/whatwg/loader/pull/65 by adding the explicit separation. Todo:

valera-rozuvan commented 2 years ago

Is this still relevant in 2021/2022?