-
Notifications
You must be signed in to change notification settings - Fork 66
Asynchronous module.import(path):Promise #50
Conversation
This proposal doesn't really seem justified. The only advantage asynchronous import provides is reading from fs in parallel. Anything else you can do already |
@vkurchatkin have you actually read the proposal? https://github.com/WebReflection/node-eps/blob/master/XXX-module-import.md
It's about being able to also export asynchronously, which is not something you can do.
You cannot export asynchronous code, after a read in parallel, after a DB connection, after an asynchronous curl, after ... you name it. This proposal enable through a single entry point asynchronous modules for both clients and server. For instance, Electron on the client can benefit the same. Saying it's unjustified it's like saying native Best Regards |
You can export a promise, which is essentially the same thing: require('foo').then(foo => {
}) vs module.import('foo').then(foo => {
})
|
Absolutely, but you don't have a way to tell from the import side if that's the case. My proposal drops any ambiguity, if you import modules via Otherwise, you'll have this ugly pattern: var foo = require('foo');
if (foo instanceof Promise) {
foo.then(init);
} else {
init(foo);
}
function init(foo) {
// do whatever you need to
} With this proposal, you have the boilerplate in core. module.import('foo').then(foo => {
// do whatever you need to
}); Such boilerplate works for exports as well module.exports = module.import('foo').then(foo => {
// do whatever you need to
});
and yet it's statically coupled with its synchronous imports, fully solved in 3 lines of extra code via this proposal. |
Well, realistically the difference is: // With this proposal:
module.import('foo').then(foo => { /* do stuff */ });
// Currently (ignoring that sync require may throw):
Promise.resolve(require('foo')).then(foo => { /* do stuff */ }); The difference in terms of ergonomics doesn't seem too big. Are there any other changes planned to make the actual loading async? Maybe this would be a good candidate for publishing a polyfill as an npm module to see how much interest there is in the community? It doesn't seem to depend on any node internals. |
With the I/O still being synchronous ( |
Until we have worked out the issues around ES6 modules and the top-level async |
Not exactly the same. The fact it can be polyfilled as no brainer in 3 lines, doesn't mean consumers will have forever a synchronous Accordingly, @evanlucas assumption is also half true
Nope, the I/O is asynchronous from user perspective. How much time we spend on core side to make it async it's up to us but ideally, all it takes to move forward for everyone here is to do the following in few months: // import as global one
Module.prototype.import = import; The whole idea is indeed to fast-move forward and provide evidence dynamic async import is already possible in node, like @vkurchatkin stated already:
Yet confined to a synchronous load, on both client and server. Last, but not least, citing @jasnell
We also need to make sure TC39 don't fully ignore what worked for the last 7+ years without a glitch, which is the dynamic CommonJS. On top of that, whenever TC39 will finalize the proposal NodeJS will be ready to go because if there's one thing they already agreed on, is the In the worst case scenario, this pattern will fade out with its 3 lines of implementation code. In the best one, it'll pave the way to move forward on both client and server, Electron-like or just Browserify, way to include asynchronous modules in a fully backward compatible way, zero risk, future friendly way to load modules. If this won't land in core, it'll be an hazard for anyone to export async modules. If this lands in core, it'd be a pattern to move forward. |
Not when it actually blocks the process for the duration of the time it takes to actually read the file from disk. With the current proposal, that is what will happen. Introducing something new is the easy part. Getting rid of something that we no longer want to keep in core is the hard part. That being said, we generally don't add things for the sake of adding them. If this can be done in userland, it should be done in userland.
Encouraging the consumer to assume that a module can be loaded asynchronously when it is really synchronous is not a good thing IMO. A module can export an asynchronous function, but making the actual loading of those asynchronous instead of synchronous breaks a lot of assumptions (especially related to circular dependencies) I'm still -1 on adding another way to import a module. |
Again, this is an implementation detail. The proposal is about bringing asynchronous
So you are happy with this module fix ? It works already so if that's the suggested way to fix this in user land, works for me.
This is what dynamic
From a consumer prospective, nothing is broken. It's really an asynchronous module, |
Moreover ... about this:
I'm loading asynchronously npm modules already on the browser. I'm not encouraging, it's already possible, and as easy as having |
To be honest, it sadden me I have the feeling people jumped in after the first post 3 LOC polyfill implementation example, instead of reading the proposal I've tried to fully explain. There are rules to follow this process, which is about producing a detailed document, but at this point I wonder if I've missed something in the process. I've tried to follow such rules, but here I answering to questions about a polyfill example and I'm not sure this is going in the right, or meant, direction. How these PR are usually handled? Anything I can do to be sure the conversation consider the proposal and not just its quick and dirty initial facade? Thanks for any possible extra clarification. |
Sorry if you feel disheartened, your intentions are very good but I believe this is not a problem about the process, just about people seeing the potential workload and what should be done in core differently. As a fairly new contributor to Node myself, after getting more familiar with core by reading a lot of mailing list posts and issues, I think @evanlucas summed this up very well with:
The preferred way in core at the moment, if I understand correctly, is to let things grow in the user land(i.e. npm), if it can be done in user land. When it becomes mature enough and gets enough traction, then it's possible to get incorporated back into core (although the idea of a "small core" is still open to debate). This is contrary to the intention of this proposal (fast-forwarding things). This proposal has its merits, but it's probably not the right time to jump the gun before IMHO if you want to push this forward, the best way is to promote this idea in the user land with a polyfill-like package (which is what you have already been doing?), and see how the ecosystem adopt it and benefit from it. Also making the loading actually non-blocking( |
To be honest, this proposal is based on what's been agreed on standard side already. It accepts one argument, it uses as string to resolve the path, it returns a Promise. The proposal is also a Stage 3 one. The difference from the standard is that this proposal will fix the ambiguity between globally available Moving forward, developers will ditch CommonJS import/export through the Meanwhile, transpilers can start exporting modules as Promises already and use How well we'd like to eventually implement the HostImportModuleDynamically logic is up to us and how much time we want to wait to have Regardless, the current proposal as it is still respect what's been agreed on standard side. Best Regards |
IIUC, stage 3 doesn't really guarantee anything, it can still change significantly, and even be dropped(no precedence though). Node's release schedule is very different from browsers, I think that's the major concerns on "jumping the gun". Browsers can put something behind a flag, release it in a 6-week cycle, and then suddenly take it back if things go south, but Node.js can't, because adding something is semver-minor, but removing something or introducing breaking changes is semver-major, even behind flags. That's why this proposal would be better promoted in the user land, where you are not bound by a LTS scheme. |
I'd agree if not for the fact that this proposal cannot possibly break anything |
OK, let me expand a bit on this one:
The only bad thing that could happen if it lands on node, is that it's not used, and it takes nothing to remove it once It doesn't require much maintenance as it is, being 3 lines of code that just work, and it doesn't add magic at all. It simply brings a slightly different pattern to the curent module, as alternative, giving already developers the ability to: const mod = await module.import('async'); Accordingly, if this proposal is considered risky, I guess we could also say the whole nodejs core is frozen and incapable to bring anything slightly different to the plate, but I'm sure that's not the case. Regards |
It might not break anything, technically, but it could create wrong assumptions and prevent a future Putting this into core before ES6 module landed or at least there's really strong consensus about the details of how ES6 modules will land risks ending up with EDIT: Fixed my |
If I might ask, with static ES2016 modules already defined and a dynamic I didn't know that was even an option for the future of node. If that is, how's that any different from this one?
Nothing can do the same because
AFAIK the second will asynchronously import what's been exported as ES2015 native export module, either synchronous or asynchronous. async function myModule() { ... }
export default await myModule() On WebKit on web, which once again is already shipping this even on Safari 10.1, and jsc (JavaScript Core for WebKit) modules load asynchronously already and I've polyfilled everything there already. It'd be great if for once Web and Node could be aligned with a migration pattern that just works.
That's inevitable already. These parts were also explained in the initial proposal.
If you are talking about ES6 modules, nothing to do with dynamic Accordingly, there's already consensus on ES6, it's dynamic import which is at Stage 3, ES6 modules are there since 2015 ... which means, I am not sure I am following anymore. |
Here is a simply question I can't see an answer to: why would anyone use this API instead of |
It's answered already, it enables asynchronous exports through a well
defined pattern to import asynchronously.
…On Fri, 3 Feb 2017 at 23:19, Vladimir Kurchatkin ***@***.***> wrote:
Here is a simply question I can't see an answer to: why would anyone use
this API instead of require? There are no benefits whatsoever
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#50 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFO9Wk-HIEkZPT-c57oL_XEJTHhVwCmks5rY7YYgaJpZM4LrhB6>
.
|
no, it doesn't. In your examples you simply export a promise. That's something you can do now |
You won't because there's no official pattern to import a promise. This
proposal makes such pattern obvious, aligned with dynamic import, enabling
await and async within modules.
…On Fri, 3 Feb 2017 at 23:26, Vladimir Kurchatkin ***@***.***> wrote:
it enables asynchronous exports
no, it doesn't. In your examples you simply export a promise. That's
something you can do now
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#50 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFO9TBrJoriXokdeSV75Ymd0GPbBUrhks5rY7eTgaJpZM4LrhB6>
.
|
Couldn't you just do this: function fakeImport(path) {
return new Promise((res) => {
res(require(path))
})
} Then we wouldn't have to touch module? |
There is: const promise = require('foo');
promise.then(...) |
Sorry for any confusion - I meant
I think you missed the difference that I meant. I didn't mean the technicalities of reserved words vs. injected by node. I was talking about what they are importing / resolving to. E.g. module record vs. default export. According to the current state of things To be compatible with the current state of ES6 discussions in node it maybe would be something like: module.import('fs').then(moduleRecord => {
// do something with fs module
moduleRecord.default.readdir('.', console.log);
}); But that is assuming that nothing about all of this will change. Which is what I meant by |
Yes, the proposal doesn't break anything when it is added into core, that would be a semver-minor. What is breaking, is if the proposal of
I think it depends on which subsystem a proposal touches, and what "dependency" it has. This proposal is different in that
FWIW, the WHATWG URL implementation is in a somewhat similar situation, but it is OK to do it in core as we are speaking, because
|
Also, IMHO this proposal is only risky in the context of early 2017, in the universe are are in. It would definitely not be risky when |
3 LOC as a private API doesn't require much maintenance, yes, but 3 LOC as a public API in the most used subsystem and is subject to change, I am not sure...maintenance is not just about code, I mean. |
Do you mean module writers would need to add it into their own dependency? I think it is not really necessary because if you don't use it, you can leave it as a CJS module exporting Promises anyway. if people want to use it, they can explicitly add it to package.json, and use: module.import = require('import');
module.import('other-modules').then(..); If module writers themselves need to use it to import another module that use this protocol, then they can add this to their own package.json files, and let npm sort out the common dependencies + semver compatibility(that's kinda the point why it would be better than implementing it in core though). Also for people writing applications (i.e. they won't be publishing these code), they can do the polyfill part by altering the prototype of Module, or the polyfill part can be provided as another module, like |
@WebReflection yes, but there is no need in Promise constructor. It can be just const promiseImport = path => Promise.resolve().then(require(path)); |
ehr ... @chicoxyzzy if that throws it throws synchronously ... (and it won't be a rejected) |
oops I mean const promiseImport = path => Promise.resolve().then(() => require(path)); sorry |
I've updated the polyfill as such: Module.prototype.import = function (path) {
return Promise.resolve().then(() => this.require(path));
}; |
As discussed in the dedicated thread, `module.import` should always be async.
@joyeecheung I've written dozen modules and if this proposal was good as module, I wouldn't have wasted anyone time. If you keep suggesting module, I'll insist this should be core 'cause if not core, it won't work. So let's agree to disagree 😄 and move forward with other possible concerns |
P.S. there's also already an npm module called import that has nothing to do with anything discussed here or anywhere else during these days in JS-land. Yet another friction to the "should be user-land" dispute. |
In which scenario? There is nothing (yet) that could be transpiled to this. Moreover, transpiliers already generate tons of helpers, they can easily add another one for |
I don't think the name is a friction really, any name clear enough would be fine for this. But then since you are the writer of this proposal and you don't want to push it as a userland module, I will just consider my suggestion not suitable for the intention of this proposal. Also you've mentioned in #50 (comment), which I believe is the one of the reasons to put it into core:
// import as global one
Module.prototype.import = import; Does this even work? |
What benefit does this have over the proposed global |
@Fishrock123 If you are talking about the dynamic import ES proposal, that is for CJS modules loading ES modules. This proposal is for CJS modules loading CJS modules, so existing CJS modules can use |
@joyeecheung you are reading the front page README which is not updated. This is current import and it's a function at Stage 3. Any other loader has been somehow abandoned or it's not part of the dynamic The difference is between a call expression, and static one. As you can see, since ES2015 already uses There are already similar things in JS such In this case they defined the invoke a dynamic async import based on Promises, and regular ES2015 a static synchronous (or at least synchronous looking since WebKit shipped it asynchronous behind the scene) TL;DR soon, JS will be like the following equivalent: import mod from './module.js';
// equivalent of
const mod = module.require('./module.js');
// and dynamic
const mod = await import('./module.js');
// equivalent of
(async () => {
const mod = await module.import('./module.js');
})(); All the Promise based bits are in the semantic to resolve the import I hope I've answered your question which in the ideal scenario is: Yes, |
please read the proposal. This part is covered here: https://github.com/WebReflection/node-eps/blob/master/XXX-module-import.md#why-as-core-feature--why-not-transpilers- Transpilers won't fix older modules published as ES5 so transpilers are not a solution. |
I'm answering to this:
This is not true: they can't do this without completely breaking existing interop, and even if they could and wanted to, they don't need module.import in core for that.
Solution to what problem. As far as I can see there is no problem at all. No one wants to load modules asynchronously just for the sake of loading asynchronously alone. |
This is a bit rushed and personal conclusion? I do want to load module asynchronous, including the ability to export asynchronously, since one thing inevitably needs the other. And this is also the language direction, so yes, apparently many want to load modules asynchronously. |
It's based on evidence. We already had asynchronous modules on the Web, AMD, and it lost to synchronous CommonJS. Loading asynchronously is objectively less convenient then loading synchronously. If you want it, you have to explain why. Asynchronous export is a separate question because its not really handled by both your proposal and current ES spec.
import() is asynchronous because it's designed with web in mind. Modules are supposed to be loaded over network, so there is no other option. |
Not really, as in the link you provide, only the syntatic form
If that's the case I believe we would have seen the proposal using CreateBuiltinFunction or touching section 18.2. To be precise, by "import is not a function" I mean |
It's not exactly a function, like you said, so maybe we cannot assign later
on but just drop the module. prefix, even easier.
Did I mention this is also out of current proposal scope, which is not to
polyfill the unpolyfillable?
And again, I believe there are threads talking about 'import()' as agreed
name for that expression.
In any case this is speculation for the future , not about this proposal.
…On Sat, 4 Feb 2017 at 18:42, Joyee Cheung ***@***.***> wrote:
@WebReflection <https://github.com/WebReflection>
This is current import and it's a function at Stage 3
Not really, as in the link you provide, only the syntatic form import(..)
is a left-hand-side expression, so import itself is not a function, kinda
like super is not a function inside a constructor(the form super(...) is
a left-hand-side expression too), that's echoing what the README says. The
name import itself is still just a keyword, you can't assign it to
anything, the parser would be confused and just throw a SyntaxError.
You've mentioned typeof and void, they are keywords for operators so they
can't be assigned to anything either.
As you can see, since ES2015 already uses import , they decided to have a
globally available import(x) as function call.
If that's the case I believe we would have seen the proposal using
CreateBuiltinFunction
<https://tc39.github.io/ecma262/#sec-createbuiltinfunction> or touching section
18.2
<https://tc39.github.io/ecma262/#sec-function-properties-of-the-global-object>.
To be precise, by "import is not a function" I mean import is not
re-introduced as a bound name for a Function Object
<https://tc39.github.io/ecma262/#sec-function-objects> in the spec.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#50 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFO9dwRSwQzVRDE3r42BgCjRwYTiP2Hks5rZMaTgaJpZM4LrhB6>
.
|
|
||
# Asynchronous `module.import(path)` | ||
There is [a standard effort](https://github.com/tc39/proposal-dynamic-import) | ||
to bring asynchronous module loading to JavaScript. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look closely, this proposal is about bringing dynamic loading, not asynchronous loading. It is motivated by examples, where module specifier is not statically known. That's something that is handled by CommonJS as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples in the same page shows asynchronous imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happens to be asynchronous, but it's not what proposal is about. Static ES6 imports can be loaded asynchronously already, so it's not what's lacking
|
||
## Unleashing asynchronous exports | ||
While good old modules will keep being usable, | ||
new modules might define themselves as asynchronous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't require new API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It requires a standard way to load them as such, hence the proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no better then doing require('foo').then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is. It hides the implementation details that could be asynchronous require behind the scene, enabled already on the web. Your one is still an explicit synchronous intent that has no future compatibility as moving forward pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works if you use module.import
instead of require
all the time, but no one will do that. There is no "synchronous intent". Module exports a promise and I need to call then
. It will work exactly the same with ES modules, so I don't understand what "future compatibility" and "moving forward" are you talking about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but no one will do that.
It's like saying: this only works if everyone will use import()
instead of require()
but no one will do that.
I don't like argumentations based on speculations. This is a moving-forward pattern, someone might do it, specially on the web.
One of the clear goal is improving universal modules.
Synchronous require
is why we need bundlers, AMD was born before Promise were there and now we have a standard proposal based on Promise.
I won't comment further about speculations on what you think everyone does or want.
I hope this can be kept as regular conversation and exchange of ideas.
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only works if everyone will use import() instead of require() but no one will do that.
Not true, this will never work with import()
at all, so it's irrelevant.
import()
is supposed to be used either when you don't know module specifier. If you don't you just use synchronous-looking import foo from 'foo'
. So yes, you are not supposed to use import()
for everything, and no one will use module.import
for everything.
I won't comment further about speculations on what you think everyone does or want
This is a very bad position when proposing some API changes. You could propose something useless and ugly-looking and argue that someone might want to use that and saying anything else is speculation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've explained already why this is needed so you already have someone that wants to do things this way and did it already.
Patterns are usually abused by developers so if there's a common one that works with everything, that pattern will be for the sake of consistency.
require
is an old pattern that worked well but will be inevitably replaced by static import
not because I say so, because that's ES standard
dynamic import()
exist because somebody wants to load dynamically modules and it enables asynchronous exports too, explained in this proposal why are needed, efficient, better, etc.
Transpilers could target this method to fallback, developers can easily migrate to asynchronous modules simply using this method, which works already in NodeJS 0.12
It's a migration pattern, and it doesn't exist yet, so it's pointless to say "nobody and no one" since few already liked the proposal (through the blogpost, through the repository).
As summary, mine is not a position at all, is a way to have a conversation based on facts or possible issues, and speculating about the future is, in my opinion, a non-argument.
asynchronous initialization behind databases connections, | ||
remote API calls, and other non blocking operations. | ||
|
||
New modules created with async `import(...)` in mind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they can't , until top level await is in the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proposal works already with async export and is future friendly with top level async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you propose has no standard equivalent, i.e. someone who relies on module.import
won't be able to switch to import()
easily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't mater, this is not, and cannot be, a polyfill for import
This is a moving-forward, backward compatible, future friendly, migration pattern.
The day import
will works in all negines you target, if it's compatible 1:1 with this proposal, you'll just drop module.
prefix.
If it's not, it'll be rather about how you export, than how you import, since the protocol Promise based has been confirmed at Stage 3 and won't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top level await is bad
https://gist.github.com/Rich-Harris/0b6f317657f5167663b493c722647221
"_require an import of a module_" since they never | ||
"_required an export of a module_". | ||
|
||
## Why as core feature ? Why not transpilers ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, since you insist this should be core instead of being a userland package, I think this section should be amended to address that question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is about transpilers. I can add another one about modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was under the impression that this section is more about "Why as core feature " than "Why not transpilers"
After reviewing the spec, it seems that your proposal is not really compatible with it. For example, if you have this: // foo
module.exports = Promise.resolve(42);
// bar
module.import('foo').then(foo => ...) using standard + // foo
export default Promise.resolve(42);
// bar
import('foo')
.then(foo => foo.default)
.then(foo => ...) As you can see, there is a big difference. ES modules don't allow to hide that something asynchronous is going on. |
I'd like to underline once again that this is not meant to be a polyfill for
My proposal works with the example I've proposed which was not meant to speculate on how ES modules + dynamic import() works. If you'd like to have same behavior then you'd transpile eventually the ES module into module.exports = {defualt: Promise.resolve(42)}; Using a different export, and please remember this proposal doesn't specify how to export anything, it simply specifies how to
I don't follow here. Dynamic |
Since we are going round in circles, I'm going to summarize my objections and refrain from participating in further discussion:
ES6 import foo from 'foo';
export function bar() {
return import(foo());
} CJS const foo = require('foo');
module.exports = function bar() {
return require(foo());
}
// foo
export default connectToDb()
.then(db => {
return ...
});
// bar
import foo from 'foo';
foo.then(foo => ...);
// or
import('foo')
.then(foo => foo.default)
.then(foo => ) In both cases you have to know that you are importing a promise, so this must be a future-proof way to export something asynchronous.
CJS // foo
const arr = ['a', 'b', 'c'].map(a => require(a));
module.exports = arr;
// bar
const arr = require('foo');
module.exports = arr[0]; This proposal: // foo
// dynamic import, so module has to become "asynchronous"
module.exports = Promise.all(['a', 'b', 'c'].map(a => module.import(a)));
// bar
// foo is "asynchronous", so bar has to become "asynchronous" too
module.exports = module.import('bar').then(bar => bar[0]);
|
`Promise.all` method to retrieve at once every needed module. | ||
```js | ||
Promise.all([ | ||
module.import('fs'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proposal should be clear about whether module.import()
is blocking or not, if not, people will be aware that they are not supposed to mutate shared external state in the module when it is being imported, and they will probably need to change these behavior in their code. If it is blocking(behavior of the old require
), then people can be sure they don't have to change their code, and they would know the order of these modules code being executed is deterministic. And this behavior should be in line with the future import()
implementation in core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea is to have a non blocking module.import()
now ensured by the updated code that uses Promise.resolve().then
upfront so the import is always asynchronous.
What the implementation does behind the scene shouldn't be a developer concern: they must be aware the import
returns asynchronously the content, no matter which content it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is intended, then this should be pointed out in the proposal IMO. For example, it should point out that developers must check for race conditions in cases like this:
Promise.all([
module.import('a'), // mutate shared external state when imported
module.import('b') // mutate shared external state when imported
module.import('c') // export something depends on the mutated state
]).then(function([a, b, c]) { ... })
as this won't happen if c
is a ESM being import()
ed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you give me a concrete example of what kind of race condition you are talking about ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a does:
process.env.foo = 'a';
and b does
process.env.foo = 'b';
and c does:
if (process.env.foo === 'a') {
module.exports = { foo: 'b is executed first' };
} else if (process.env.foo === 'b') {
module.exports = { foo: 'a is executed first' };
} else {
module.exports = { foo: 'c is executed first' }
}
The export of c
in nondeterministic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't happen if c is a ESM, because ESM must export at the top level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESM must export at the top level.
once dynamic import
will be in place, considering that such feature is proposed with the Web in mind, and considering that the Web will inevitably have asynchronous dependencies, are you sure ESM won't be capable of doing the following?
export default async () => Promise.all([import('a'), import('b')])
.then(([a, b]) => ({
c: x => a(x) + b(x)
}));
'cause this proposal idea is to enable asynchronous exports too.
However, being the current logic based on synchronous require and the Promise.resolve()
called sequently, there should never be a race condition case if modules are exported synchronously.
The scheduling is still incremental.
Promise.all([
Promise.resolve(1).then(console.log),
Promise.resolve(2).then((v) => {
let t = Date.now();
while (Date.now() - t < 1000);
console.log(v);
}),
Promise.resolve(3).then(console.log)
]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, being the current logic based on synchronous require and the Promise.resolve() called sequently, there should never be a race condition case if modules are exported synchronously.
What I mean is, is this behavior intentional, or is it just a coincidence due to the polyfill? Is the choice of the blocking and synchronousrequire
intentional? Will it ever decide to switch to something else, like another loader, which could, for example, usefs.readfile
to separate the reading and the parsing/execution? If it is intentional, this should be pointed out so people know the cost of transition to this pattern, if not, this should be pointed out too, so people would know this is just a coincidence and there is no guarantee about it.
Another reason this is relavant is that the order of the ESM loader in Node.js is still in discussion: https://docs.google.com/presentation/d/1xZILfv5WeUBKyQ9s1w8zArNgzTMGRFQXyRseSskjcjw/view#slide=id.g1bfb475df0_0_166 If this proposal wants to stay inline with the decision on that side, it would probably create less confusion to future users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the reply by email didn't work that well.
I am waiting for this #39 (comment) to be decided/agreed to better answer your question.
Right now the polyfill as it is grants order. If async, the order won't be granted.
As suggested in the official repository first, and in the node-esp one after, I'd like to propose the following concept to the NodeJS core:
Please Note this is an implementation polyfill example, not the proposal itself