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

Asynchronous module.import(path):Promise #50

Closed
wants to merge 2 commits into from
Closed

Asynchronous module.import(path):Promise #50

wants to merge 2 commits into from

Conversation

WebReflection
Copy link

@WebReflection WebReflection commented Jan 23, 2017

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

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

@vkurchatkin
Copy link

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

@WebReflection
Copy link
Author

@vkurchatkin have you actually read the proposal?

https://github.com/WebReflection/node-eps/blob/master/XXX-module-import.md

The only advantage asynchronous import provides is reading from fs in parallel.

It's about being able to also export asynchronously, which is not something you can do.

Anything else you can do already

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 import() TC39 is bringing to the language is useless too for NodeJS, and I believe that's not the case.

Best Regards

@vkurchatkin
Copy link

vkurchatkin commented Jan 27, 2017

It's about being able to also export asynchronously, which is not something you can do.

You can export a promise, which is essentially the same thing:

require('foo').then(foo => {
})

vs

module.import('foo').then(foo => {
})

Saying it's unjustified it's like saying native import() TC39 is bringing to the language is useless too for NodeJS, and I believe that's not the case.

import adds dynamic import to static module system. CommonJS is already dynamic

@WebReflection
Copy link
Author

WebReflection commented Jan 27, 2017

You can export a promise, which is essentially the same thing:

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 module.import you're sure it's going to be a Promise. Happy UX for importer, easier to have major updated that switch to asynchronous exports for current libraries.

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
});

CommonJS is already dynamic

and yet it's statically coupled with its synchronous imports, fully solved in 3 lines of extra code via this proposal.

@jkrems
Copy link

jkrems commented Jan 27, 2017

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.

@evanlucas
Copy link

With the I/O still being synchronous (require), I think this could be more confusing to consumers, especially if it's marketed as an async require (as this proposal currently stands). I also don't see much gain from this. I personally think we should wait and see for ES Modules. -1 from me.

@jasnell
Copy link
Member

jasnell commented Jan 27, 2017

Until we have worked out the issues around ES6 modules and the top-level async import() function that will be introduced alongside I'm -1 on making any change such as this for the time being. We need to make sure that whatever we do here improves alignment and closes the gap with the language standard.

@WebReflection
Copy link
Author

WebReflection commented Jan 28, 2017

Well, realistically the difference is:
Promise.resolve(require('foo')).then

Not exactly the same.
This proposal intent, in all its simplicity, is to hide any possible implementation detail.

The fact it can be polyfilled as no brainer in 3 lines, doesn't mean consumers will have forever a synchronous require underneat, it's just how it's going to look from a polyfill prospective.

Accordingly, @evanlucas assumption is also half true

With the I/O still being synchronous (require) ...

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:

CommonJS is already dynamic

Yet confined to a synchronous load, on both client and server.

Last, but not least, citing @jasnell

We need to make sure that whatever we do here improves alignment and closes the gap with the language standard.

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 Promise as definition protocol since compatible already with async and await.

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.

@evanlucas
Copy link

Nope, the I/O is asynchronous from user perspective.

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.

If this won't land in core, it'll be an hazard for anyone to export async modules.

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.

@WebReflection
Copy link
Author

WebReflection commented Jan 28, 2017

Not when it actually blocks ...

Again, this is an implementation detail.

The proposal is about bringing asynchronous module.import to NodeJS, everybody seems to focus on the 3 LOC polyfill.

If this can be done in userland, it should be done in userland.

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.

Encouraging the consumer to assume that a module can be loaded asynchronously ...

This is what dynamic import is about, I'm not encouraging, I'm proposing to "fast-forward" the already agreed pattern

making the actual loading of those asynchronous instead of synchronous breaks a lot of assumptions

From a consumer prospective, nothing is broken. It's really an asynchronous module, Promise takes care of that bit, even in its primordial polyfill-like implementation.

@WebReflection
Copy link
Author

Moreover ... about this:

Encouraging the consumer to assume that a module can be loaded asynchronously ...

I'm loading asynchronously npm modules already on the browser.

I'm not encouraging, it's already possible, and as easy as having Promise based exports, and a way to opt-in for Promise based imports.

@WebReflection
Copy link
Author

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.

@joyeecheung
Copy link
Member

joyeecheung commented Jan 30, 2017

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:

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.

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 import() even gets landed in ES, and Node.js core is probably not the best place for a still-blocking-but-looks-asynchronous API which can be done in the user land without any problem. At the very least, it adds burden to maintenance, evangelism and support on the issue tracker(I can foresee a bunch of issues like "why is module.import blocking my process?!", or, when it does become non-blocking, a bunch of issues about weird races caused by module loading order).

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(fs.readFile instead of readFileSync) could be more attractive to users, although they must be aware of this to avoid subtle bugs if their modules have side effects when being required.

@WebReflection
Copy link
Author

This proposal has its merits, but it's probably not the right time to jump the gun before import() even gets landed in ES

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 import("module") which will require a module with native export default {module} instead of module.exports = {module};

Moving forward, developers will ditch CommonJS import/export through the module object but this will take years.

Meanwhile, transpilers can start exporting modules as Promises already and use module.import(..) to lead them without breaking backward or future compatibility.

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 module.import future friendly ability.

Regardless, the current proposal as it is still respect what's been agreed on standard side.

Best Regards

@joyeecheung
Copy link
Member

joyeecheung commented Feb 3, 2017

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.

@WebReflection
Copy link
Author

I'd agree if not for the fact that this proposal cannot possibly break anything

@WebReflection
Copy link
Author

WebReflection commented Feb 3, 2017

OK, let me expand a bit on this one:

this proposal cannot possibly break anything

  1. async and await works with Promise and are already shipping as ES2017. They are final
  2. accordingly, no matter what happens to global import, it's going to be compatible with Promise pattern
  3. this proposal does not replace or polyfill the global import 'cause that's just not possible. This proposal brings asynchronous compatiblity to CommonJS
  4. even if import gets dropped at Stage 3, and most likely we know it won't happen, this proposal will be the de-facto fallback for transpilers that targets engines that are not fully compatible with ES2015 static export and import, simply because there already landed in WebKit and JSC, are standard as defined in 2015, and are asynchronous on the Web
  5. this proposal doesn't ask anything to anyone, it's backward and future compatible. How can this break? It's a minor, and that's it
  6. the day we can remove this proposal is the day we can remove CommonJS module. I can't wait for that day but this is nothing different from current module.exports that is still there and there will be for some time
  7. this is a migration pattern for transpilers and/or developers that would like to export already asynchronous modules, since these are already compatible with latest standard.

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 module.exports or require will get dropped.

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

@jkrems
Copy link

jkrems commented Feb 3, 2017

It might not break anything, technically, but it could create wrong assumptions and prevent a future module.import with different semantics. For example, if I'm following the discussions correctly, the latest draft for ES6 module support in node core would consider only the default export of CommonJS modules. So module.import('async') in this proposal would not do the same as import('async'). The former would resolve to the CommonJS export objects, the second would resolve to - skipping over details - { default: require('async') }.

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 import('x'), module.import('x'), and import 'x' looking very similar but not doing the same thing.

EDIT: Fixed my require.import vs. module.import typos.

@WebReflection
Copy link
Author

prevent a future require.import

If I might ask, with static ES2016 modules already defined and a dynamic import you are thinking about a future require.import? what's for? to make node more coupled with require beside the module?

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?

So module.import('async') in this proposal would not do the same as import('async')

Nothing can do the same because import is a reserved word and without major engines version upgrade you're making every LTS stuck with CommonJS modules only and no async pattern or transpiler target.

second would resolve to - skipping over details - { default: require('async') }.

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.

import('x'), require.import('x'), and import 'x' looking very similar but not doing the same thing.

That's inevitable already. import(x) is dynamic, import x is static, require.import I don't know what that is or why would even exist while here it's module.import, since module.exports is how modules are already defined so, on the Web, there's no need to be coupled with both module, mandatory to export, and require, a pseudo global that's never been mandatory to create a module, while module is, hence the target choice for the import.

These parts were also explained in the initial proposal.

Putting this into core before ES6 module landed

If you are talking about ES6 modules, nothing to do with dynamic import() proosal, Safari 10.1 already has them, and so does jsc.
https://twitter.com/WebReflection/status/826849353885171713

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.

@vkurchatkin
Copy link

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

@WebReflection
Copy link
Author

WebReflection commented Feb 3, 2017 via email

@vkurchatkin
Copy link

it enables asynchronous exports

no, it doesn't. In your examples you simply export a promise. That's something you can do now

@WebReflection
Copy link
Author

WebReflection commented Feb 3, 2017 via email

@evanlucas
Copy link

Couldn't you just do this:

function fakeImport(path) {
  return new Promise((res) => {
    res(require(path))
  })
}

Then we wouldn't have to touch module?

@vkurchatkin
Copy link

You won't because there's no official pattern to import a promise.

There is:

const promise = require('foo');
promise.then(...)

@jkrems
Copy link

jkrems commented Feb 4, 2017

require.import I don't know what that is or why would even exist while here it's module.import

Sorry for any confusion - I meant module.import, it was just a typo that I wrote require.import.

Nothing can do the same because import is a reserved word and without major engines version upgrade you're making every LTS stuck with CommonJS modules only and no async pattern or transpiler target.

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 import { series } from 'async' (assuming that async is a CommonJS module) will not work. And import('async').then(({ series }) => { /* use series */ }) will not work either (because dynamic import resolves to the module record, not to the default export). But with your proposal module.import('async').then(({ series }) => {}) will work.

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 import('x') and module.import('x') looking similar but doing different things.

@joyeecheung
Copy link
Member

this proposal doesn't ask anything to anyone, it's backward and future compatible. How can this break? It's a minor, and that's it

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 import() has a breaking change (or in a unlikely but still possible senario, gets dropped), Node would need to follow up, and that would be semver-major. I am not very sure about this being future compatible, because future is, unfortunately, very unpredictable.

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.

I think it depends on which subsystem a proposal touches, and what "dependency" it has. This proposal is different in that

  1. It is entirely possible to be implemented in user land
  2. It depends on a proposal that is still being designed and subject to (breaking) change

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

  1. People are already using URL on the Web, multiple major browsers have already implemented it and shipped it without flags. The ES import() is neither shipped without flags (not that I know of) nor adopted by many users in the wild.
  2. Because of 1, the URL spec won't be introducing changes that would "break the Web", so it would work well with the LTS schedule. However, import() is only in Stage 3, which in the context of ECMAScript process, as I understand, is not "we have designed this and vendors should start implementing it", but "we have done everything we can to design it with spec text and polyfills only, to refine this further, we need feedback from real implementation, and from real world users".
  3. The URL implementation in Node conforms to the spec, not depends on it as a migration pattern. IMHO migration should happen at least when the thing you want to migrate to is somewhat finalized, in this case, Stage 4, or when import() itself is actually implemented in core, whichever happens first. If the thing you want to migrate to changes, a premature migration pattern, and a one subject to the LTS release schedule, would not be ideal.

@joyeecheung
Copy link
Member

joyeecheung commented Feb 4, 2017

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 import() is implemented in core and when a lot of users already use this pattern or a similar pattern in their code, in production. Not that it should happen in this order, just to clarify things.

@joyeecheung
Copy link
Member

joyeecheung commented Feb 4, 2017

It doesn't require much maintenance as it is, being 3 lines of code that just work

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.

@joyeecheung
Copy link
Member

joyeecheung commented Feb 4, 2017

However, that means there's no mechanism that grants async import so that exported modules need a dependencies in domains they don't belong yet ... yak!

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 require('global-import').

@chicoxyzzy
Copy link
Contributor

@WebReflection yes, but there is no need in Promise constructor. It can be just

const promiseImport = path => Promise.resolve().then(require(path));

@WebReflection
Copy link
Author

WebReflection commented Feb 4, 2017

ehr ... @chicoxyzzy if that throws it throws synchronously ... (and it won't be a rejected)

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Feb 4, 2017

oops I mean

const promiseImport = path => Promise.resolve().then(() => require(path));

sorry

@WebReflection
Copy link
Author

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.
@WebReflection
Copy link
Author

@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

@WebReflection
Copy link
Author

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.

@vkurchatkin
Copy link

Moreover, this proposal enables transpilers to export Promises as target syntax because

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 import()

@joyeecheung
Copy link
Member

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:

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;

Does this even work? import() at the time being is not an actual function, it is just a syntatic form (kinda like a macro), import itself is a keyword so this should throw SyntaxError.

@Fishrock123
Copy link

Fishrock123 commented Feb 4, 2017

What benefit does this have over the proposed global import()?

@joyeecheung
Copy link
Member

@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 module.import to load existing CJS modules, don't have to wait until we've sorted out the ES2015 import part and then wait for their dependencies to provide ES flavor distributions.

@WebReflection
Copy link
Author

WebReflection commented Feb 4, 2017

@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 import(x) definition.

The difference is between a call expression, and static one.
https://tc39.github.io/proposal-dynamic-import/#sec-left-hand-side-expressions

As you can see, since ES2015 already uses import , they decided to have a globally available import(x) as function call.

There are already similar things in JS such typeof obj VS typeof(obj) and same goes for void

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) import

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
https://tc39.github.io/proposal-dynamic-import/#sec-hostresolveimportedmodule

I hope I've answered your question which in the ideal scenario is:

Yes, Module.prototype.import = import should just work, but this is not what the proposal is about, and you explained already pretty well.

@WebReflection
Copy link
Author

@vkurchatkin

transpiliers already generate tons of helpers, they can easily add another one for import()

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.

@vkurchatkin
Copy link

I'm answering to this:

Meanwhile, transpilers can start exporting modules as Promises already and use module.import(..) to lead them without breaking backward or future compatibility.

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.

Transpilers won't fix older modules published as ES5 so transpilers are not a solution

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.

@WebReflection
Copy link
Author

No one wants to load modules asynchronously

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.

@vkurchatkin
Copy link

vkurchatkin commented Feb 4, 2017

This is a bit rushed and personal conclusion?

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.

And this is also the language direction, so yes, apparently many want to load modules asynchronously.

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.

@joyeecheung
Copy link
Member

@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 or touching section 18.2. To be precise, by "import is not a function" I mean import is not re-introduced as a bound name for a Function Object in the spec.

@WebReflection
Copy link
Author

WebReflection commented Feb 4, 2017 via email


# Asynchronous `module.import(path)`
There is [a standard effort](https://github.com/tc39/proposal-dynamic-import)
to bring asynchronous module loading to JavaScript.

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.

Copy link
Author

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

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

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

Copy link
Author

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.

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

Copy link
Author

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.

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

Copy link
Author

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

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.

Copy link
Author

@WebReflection WebReflection Feb 5, 2017

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

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

Copy link
Author

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.

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

Copy link
Author

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.

Copy link

Choose a reason for hiding this comment

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

"_require an import of a module_" since they never
"_required an export of a module_".

## Why as core feature ? Why not transpilers ?
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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"

@vkurchatkin
Copy link

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 + import() this would be

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

@WebReflection
Copy link
Author

WebReflection commented Feb 5, 2017

I'd like to underline once again that this is not meant to be a polyfill for import, but it could be a possible target for transpilers. If there's some amend to do to make it better, I'd be happy to do so.

your proposal is not really compatible with it

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 module.import(...), is allowed, and it makes this proposal compatible with your expectations.

ES modules don't allow to hide that something asynchronous is going on.

I don't follow here. Dynamic import() works with Promises, so it's clear that something asynchronous is going on.

@vkurchatkin
Copy link

Since we are going round in circles, I'm going to summarize my objections and refrain from participating in further discussion:

  1. In ES modules import() is supposed to be a dynamic counterpart of import from. In Node, require is already used for both static and dynamic imports, new API is not needed:

ES6

import foo from 'foo';

export function bar() {
   return import(foo());
}

CJS

const foo = require('foo');

module.exports = function bar() {
   return require(foo());
}
  1. "Transparent" asynchronous exports are not possible with ES modules: static import won't unwrap promises and dynamic import() return namespaces object, so it won't unwrap promises either:
// 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.

  1. Making a module "asynchronous" will require making all if its direct and indirect dependents "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]);
  1. Transpilers don't need this to transpile import() and they won't use it to transpile static import, because it's going to break existing interop.

`Promise.all` method to retrieve at once every needed module.
```js
Promise.all([
module.import('fs'),
Copy link
Member

@joyeecheung joyeecheung Feb 5, 2017

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.

Copy link
Author

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

Copy link
Member

@joyeecheung joyeecheung Feb 6, 2017

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

Copy link
Author

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 ?

Copy link
Member

@joyeecheung joyeecheung Feb 6, 2017

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.

Copy link
Member

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.

Copy link
Author

@WebReflection WebReflection Feb 6, 2017

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)
]);

Copy link
Member

@joyeecheung joyeecheung Feb 7, 2017

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.

Copy link
Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.