-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Node core dev thinks loader will not work for Node #54
Comments
@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. |
as per the conditional loader, it is more limited since the resolution is part of the loader, you cannot have easy user logic like |
@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 " 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. |
@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 if(needThing) {
let thing = await loader.import("thing");
} |
@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 |
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 |
@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. |
@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. |
@matthewp If the spec requires |
@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. |
I would think you would be able to keep backwards compatibility with |
@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? |
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. |
@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 :
|
@trevnorris @bmeck ...
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. |
Can someone provide more details about the circular dependencies issue, I don't see the problem :) |
Other thread that you have been replying to contains the comments and
On Fri, Jul 10, 2015 at 6:45 PM, Caridy Patiño notifications@github.com
|
Let's work through a hypothetical scenario. Take modules
Simple enough. Until:
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. |
@trevnorris I don't fully understand this statement. Are you talking about something like this for A.js?
|
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? |
@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 |
There is miscommunication going on about all of this; I will be on a Google Event Link: Hangout Link: On Tue, Jul 14, 2015 at 5:17 AM, Guy Bedford notifications@github.com
|
I will be there... |
@bmeck thanks for bringing a discussion together. I won't be able to make it unfortunately, but will follow the notes! |
@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. |
@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 (
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 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. |
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. |
@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 |
@guybedford Thanks for example. So you're saying that |
Sure, hope it is making some sense. In the example, |
@guybedford this causes a problem as having 2 pipelines means if you make a module built with |
Meeting notes 14 July 2015: CommonJS module loading of a WHATWG moduleAssume 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";
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 allowedPreventing 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 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 dependencyIt appears no circular dependencies are possible in the current text: Line 664 in 2946478
Seems like an error. More talk about CommonJSSee above. TalkThere 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 nodeIt 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 @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). 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 |
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 |
@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. |
@guybedford can you clarify "static checking" we will not be parsing to see how |
@bmeck exactly it hits some edge cases, but can be allowed for non-variable requires. |
@guybedford there are a fair number of module/plugin systems that use |
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 This will completely mess with how modules currently work. Because devs use 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. |
This might be its own topic, but where is the mandate that we must use 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
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 As for linking, we have the This sounds like a big change, but I doubt it is since all of the changes would be moving from |
@bmeck that's a fair question, we will look into that. |
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. |
Follow up call will be at 1PM-2PM CDT Thursday, adjusted event accordingly. |
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. |
What is EWM? |
@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. |
partially addressed by #65 by adding the explicit separation. Todo:
|
Is this still relevant in 2021/2022? |
See WebAssembly/design#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.
The text was updated successfully, but these errors were encountered: