nodejs / node-eps

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

Asynchronous module.import(path):Promise #50

Closed WebReflection closed 6 years ago

WebReflection commented 7 years ago

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

WebReflection commented 7 years ago

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

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

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

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

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 notifications@github.com 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 https://github.com/nodejs/node-eps/pull/50#issuecomment-277389744, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFO9Wk-HIEkZPT-c57oL_XEJTHhVwCmks5rY7YYgaJpZM4LrhB6 .

vkurchatkin commented 7 years ago

it enables asynchronous exports

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

WebReflection commented 7 years ago

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 notifications@github.com 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 https://github.com/nodejs/node-eps/pull/50#issuecomment-277390721, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFO9TBrJoriXokdeSV75Ymd0GPbBUrhks5rY7eTgaJpZM4LrhB6 .

evanlucas commented 7 years ago

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

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

There is:

const promise = require('foo');
promise.then(...)
jkrems commented 7 years ago

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

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

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

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.

WebReflection commented 7 years ago

@evanlucas @vkurchatkin please read previous comments. https://github.com/nodejs/node-eps/pull/50#issuecomment-275813109

You both are focusing on the polyfill, which is an implementation detail. This proposal is not about the polyfill.

the proposal of import() has a breaking change Node would need to follow up

There's nothing that this proposal blocks or prevent Node to follow up. Again, this proposal is not a polyfill for dynamic import() so it cannot possibly block anything.

It is entirely possible to be implemented in user land

Nobody answered the question I've asked already: Is this code used by my user-land module acceptable? 'cause that's usually considered a bad practice on Web (extending natives) but it's the only reasonable way to have a project capable of using this pattern.

You require that module on the main.js file and the whole project is enabled.

It depends on a proposal that is still being designed and subject to (breaking) change

It depends on the only part of the proposal that will never change: the name and the Promise as protocol. These are not the part discussed. Not a single issue is about the name or the Promise.

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

It is not possible to polyfill this specification. It can only be transpiled into something reasonable like this proposal that already works on Web.

in this case, Stage 4, or when import() itself is actually implemented in core,

fair enough. So we'll keep this open for other 6 months (I'm optimistic) then I'll change the proposal if there's anything to change?

addaleax commented 7 years ago

@vkurchatkin One valid reason to use something like what is proposed here is that there would be a method which always returns a Promise, no matter what it’s importing. In particular, if next week we make the decision to use an asynchronous loader ES6, I think we’re pretty much stuck with either:

By the way, I feel a bit uncomfortable with the fact that the proposal text is currently encouraging people to export Promise instances from CJS… it will just be really unexpected to receive a Promise from require().

vkurchatkin commented 7 years ago

One valid reason to use something like what is proposed here is that there would be a method which always returns a Promise, no matter what it’s importing

This only matters for someone who always uses module.import instead of require, but that's not going to happen. It's simply too inconvenient to switch to it for no reason. Also when you make just one module in you project "asynchronous" it completely breaks whole project until you change every instance of require to module.import in every file.

In particular, if next week we make the decision to use an asynchronous loader ES6

This doesn't really prevent us from having synchronous loader for CJS compatibility, at least until there is no top-level await.

Introducing something along the lines of this proposal/global import(); or

Something along the lines, but exclusively fro loading ES6 modules from CJS modules. This proposal is about loading CJS modules from CJS modules

Not providing any way to load ES6 from CJS

To be honest, I'm fine with that, but that could undermine adoption

bmeck commented 7 years ago

import() is proposed to come to all parse goals, so there will be a mechanism to get ESM from CJS unless we always reject if caller was from CJS. It is syntax so we could not disable or override it. With that proposal there is always a way to get ESM.

On Feb 4, 2017 4:46 AM, "Anna Henningsen" notifications@github.com wrote:

@vkurchatkin https://github.com/vkurchatkin One valid reason to use something like what is proposed here is that there would be a method which always returns a Promise, no matter what it’s importing. In particular, if next week we make the decision to use an asynchronous loader ES6, I think we’re pretty much stuck with either:

  • Making require return a Promise when it resolves to an ES6 module. That gives require a horrible type signature (and it would break any module exporting a function named then or catch); or
  • Introducing something along the lines of this proposal/global import(); or
  • Not providing any way to load ES6 from CJS

By the way, I feel a bit uncomfortable with the fact that the proposal text is currently encouraging people to export Promise instances from CJS… it will just be really unexpected to receive a Promise from require() .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/node-eps/pull/50#issuecomment-277434914, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOUo-M9b0renLHK9rxsLAg0wuIjykynks5rZFcGgaJpZM4LrhB6 .

addaleax commented 7 years ago

@bmeck Right, sorry, I should have been clearer. But then again this EP is pretty much about providing import() earlier than it would be implemented by the VM.

In particular, if next week we make the decision to use an asynchronous loader ES6

This doesn't really prevent us from having synchronous loader for CJS compatibility, at least until there is no top-level await.

I am under the impression that we want to avoid that kind of future breaking change if it’s foreseeable (i.e. choose one or the other and then stick with it).

joyeecheung commented 7 years ago

There's nothing that this proposal blocks or prevent Node to follow up. Again, this proposal is not a polyfill for dynamic import() so it cannot possibly block anything.

What I mean is, it is the timing that could block Node from following up, unless it is implemented in master but never released to any branch until import() is implemented, but IIUC this is not your initial intention for this proposal? Or unless any breaking changes to this is considered an exception and released to maintained branches as semver-patch (by breaking I mean any changes that would be backwards incompatible to itself, like making it non-blocking when a previous version is blocking i.e. readFileSync underneath).

Also this can be a whole different story if it doesn't advertise itself as a official migration to import() implemented in core, because by advertising it that way, Node is responsible to follow up spec changes, even if the changes doesn't play well with the LTS schedule. But if it is just an async addition to NCJS, whoever uses it will take the responsibility to follow up spec changes, and they won't be bound by LTS schedule. From the users' perspective, upgrading to a semver-major userland package is much easier than upgrading to a semver-major Node.js release.

Nobody answered the question I've asked already: Is this code used by my user-land module acceptable? 'cause that's usually considered a bad practice on Web (extending natives) but it's the only reasonable way to have a project capable of using this pattern.

The pattern itself (without taking how it is exposed into account) can be accomplished without extending natives (https://github.com/nodejs/node-eps/pull/50#issuecomment-277391710) though. People can import this explicitly as a function.

It is not possible to polyfill this specification. It can only be transpiled into something reasonable like this proposal that already works on Web.

Yeah, I actually meant "spec text, polyfill, or transpilation..stuff that don't require implementation of a platform"

So we'll keep this open for other 6 months (I'm optimistic) then I'll change the proposal if there's anything to change?

I think for the sake of this proposal itself, no matter how it is gonna be advertised/released, this is the best way to push things. In the meantime you can promote this in the userland and address concerns raised by other people in this thread and from real world users :)

chicoxyzzy commented 7 years ago

Some import() alternatives above are incorrect:

// Currently (ignoring that sync require may throw):
Promise.resolve(require('foo')).then(foo => { /* do stuff */ });
function fakeImport(path) {
  return new Promise((res) => {
    res(require(path))
  })
}

should be

function fakeImport(path) {
  return Promise.resolve().then(require(path));
}

UPDATE: fix a typo should be

function fakeImport(path) {
  return Promise.resolve().then(() => require(path));
}
joyeecheung commented 7 years ago

Also there is another possibility for this proposal, if it want to be advertised as a official migration path: if it can be done entirely in userland, why not do it in the userland, put it under the nodejs organization, and release it as a npm package? That way it is not bound by LTS schedule either.

vkurchatkin commented 7 years ago

But then again this EP is pretty much about providing import() earlier than it would be implemented by the VM.

It will only work with CJS modules, so it's not the same as import(). That's exactly my point: asynchronous dynamic import only makes sense in context of ES6 modules.

@chicoxyzzy last to are equivalent, but it doesn't really matter. If your path is static then you are not going to catch anyaway

chicoxyzzy commented 7 years ago

@vkurchatkin no, they aren't

let lol;
new Promise(resolve => {
  resolve(NaN);
  lol = 'lol';
});
// lol === 'lol' here
WebReflection commented 7 years ago

@chicoxyzzy

Some import() alternatives above are incorrect:

Check my first version careful because it considers already throwing require and the promise will be rejected indeed.

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

promiseImport('shenanigans').catch(console.error);

@joyeecheung

release it as a npm package

I need to import relatively to the module that imports this proposal, unless developers would be OK in writing the following on their modules.

module.import = require('import');

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!

@vkurchatkin

This proposal is about loading CJS modules from CJS modules

Moreover, this proposal enables transpilers to export Promises as target syntax because it's not about what @addaleax thinks:

I feel a bit uncomfortable with the fact that the proposal text is currently encouraging people to export Promise instances from CJS

It's about enabling a pattern to both import and export asynchronously.

About asynchronous exports

It's inevitably needed by the moment you have asynchronous imports. How else are you going to asynchronously import dependencies for your module, if not by making it usable once everything is fine?

chicoxyzzy commented 7 years ago

@WebReflection

let's replace require call with console.log for example

function promiseImport(path) {
  return new Promise(res => res(console.log(path)));
}

promiseImport('whatever');

it will execute on same tick, but import() is always async as of proposed spec text

WebReflection commented 7 years ago

You mean something like this?

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

I can update the proposed polyfill

joyeecheung commented 7 years ago

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

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

const promiseImport = path => Promise.resolve().then(require(path));
WebReflection commented 7 years ago

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

chicoxyzzy commented 7 years ago

oops I mean

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

sorry

WebReflection commented 7 years ago

I've updated the polyfill as such:

Module.prototype.import = function (path) {
  return Promise.resolve().then(() => this.require(path));
};
WebReflection commented 7 years ago

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

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

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