Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Is it expected that module namespace objects are treated as thenables? #47

Closed
anba opened this issue Aug 4, 2017 · 20 comments
Closed

Comments

@anba
Copy link

anba commented Aug 4, 2017

Module file "m.js":

export function then(resolve, reject) {
    resolve("Is it expected that module namespace objects are treated as thenables?");
}
import("./m.js").then(v => print(v));

When loaded with jsc -m ./m.js or d8 --harmony_dynamic_import --module ./m.js, it prints "Is it expected that module namespace objects are treated as thenables?". So, is it expected?

@domenic
Copy link
Member

domenic commented Aug 4, 2017

Yep!

@ljharb
Copy link
Member

ljharb commented Aug 4, 2017

oh wow, that's very surprising - so i could export default x; export function then() { return x; } if i wanted to avoid having to import('path').then(x => x.default)?

@anba
Copy link
Author

anba commented Aug 5, 2017

oh wow, that's very surprising - so i could export default x; export function then() { return x; } if i wanted to avoid having to import('path').then(x => x.default)?

Yes, almost. Your current version will keep the import() Promise indefinitely pending, but this should work:

export default "Default-Export";

export function then(resolve) {
    // |this| is bound to the module namespace object when calling during
    // promise resolution.
    resolve(this.default);
}

import("./t.js").then(v => print(v)); // Prints "Default-Export"

@ljharb
Copy link
Member

ljharb commented Aug 5, 2017

Is that specifically called out? "then" functions usually don't get called with a resolve function, that's typically only for the Promise constructor executor.

@anba
Copy link
Author

anba commented Aug 6, 2017

Is that specifically called out? "then" functions usually don't get called with a resolve function, that's typically only for the Promise constructor executor.

What happens here is, we end up in 25.4.1.3.2 Promise Resolve Functions, where resolution is the module namespace object. Step 8 gets the then export and step 12 enqueues a PromiseResolveThenableJob. 25.4.2.2 PromiseResolveThenableJob will then create the default resolving functions and call the exported then function in step 2.

@caridy
Copy link

caridy commented Aug 6, 2017

hmm, this is confusing! @domenic can you elaborate more on the motivation for this to work like that?

@ljharb
Copy link
Member

ljharb commented Aug 6, 2017

I'm doubly confused now. One that ModuleRecords would be treated as thenables, and two that .then would ever get passed a resolver function as an arg.

@robpalme
Copy link
Contributor

robpalme commented Aug 6, 2017 via email

@domenic
Copy link
Member

domenic commented Aug 6, 2017

One that ModuleRecords would be treated as thenables

All objects are treated as thenables when you use them as the resolution value of a promise.

and two that .then would ever get passed a resolver function as an arg.

That's always the case when assimilating thenables. Remember that Promise.resolve(x) is equivalent to new Promise(res => x.then(res)) when x is a thenable.


hmm, this is confusing! @domenic can you elaborate more on the motivation for this to work like that?

This all comes from the usual invariant that you can't have promises for thenables. That's enforced by the promise machinery, and not something we could break if we wanted to.

To verify my understanding: does this mean that importing a single module's namespace statically could result in a different value compared to importing that same module dynamically?

I don't know what these terms mean. But maybe let me answer a related question. Doing Promise.resolve(x).then(x2 => ...) vs. just x will sometimes give you different values for x2 and x. This is true for any x, including module namespace objects. And it is true for variations of Promise.resolve(), such as using service worker cache APIs, IndexedDB (now that it's been promisified), or any other promise-returning API. Including, of course, import().

@robpalme
Copy link
Contributor

robpalme commented Aug 7, 2017

(deleted previous comment as I misread the spec)

The specs says that the module namespace gets resolved every time import() is called. This means you could write a then() export that rejects first time and fulfills second time. So the first import() caller see a rejection and the second does not.

This seems at odds with the spirit of the recent changes to store and rethrow module evaluation errors.
If a module fails to be imported, any subsequent import of that same module ought to result in the same stored error being observed.

I would also suggest that the modules-as-thenables pattern will not remain an unused corner-case. It provides the unique ability to encapsulate arbitrary multi-tick async work (including nested dynamic imports) beneath a single top-level import().

@Jamesernator
Copy link

I feel like there should at least be a way to dynamically get the actual module given that statically you can (even if its part of a future proposal).

It just seems weird that this:

import * as scene from "./scene.js"

async function displayAnimation() {
    while (true) {
        const currentFrame = scene.then()
        display(currentFrame)
    }
}

Couldn't be safely refactored into this to use lazy-loading:

async function displayAnimation() {
    const scene = await import('./scene.js')
    while (true) {
        const currentFrame = scene.then()
        display(currentFrame)
    }
}

A few possible ideas to getting the real module:

  1. meta-function on import
// terrible name, but I couldn't think of anything else
import.boxed('./thenableModule.js')
// -> Promise{ { module: thenableModule } )
  1. optional callback argument to import()
// rather not see this, seems like there'd be more interesting
// uses of the second argument
import('./thenableModule.js', module => {
    ...
})
  1. callback variant of the meta-function
// also terrible name
import.callback('./thenableModule.js', module => {
    ...
})
  1. Allow modules to be non-thenable but would require changes now to be done to the Promise.resolve logic.
import('./thenableModule.js', module => {
    module[Symbol.preventThenable] === true // Or something like that
                                            // possibly even an internal
                                            // field [[IsThenable]]
})

@domenic
Copy link
Member

domenic commented Aug 14, 2017

Given that this is working as intended, and the thread has turned into a place for people to start creating new proposals, I think it's time to close this.

@caridy
Copy link

caridy commented Aug 14, 2017

I understand that this is probably working as intended from the thenable object perspective, but I have two main concerns:

  1. a change in module foo, to add an export function then is a breaking change, while adding any other export is never a breaking change.
  2. people might use this to prevent others from importing their modules using import(), for whatever reason... e.g.: export function then(resolve, reject) { reject(\you can't use this module dynamically`) }`

@domenic
Copy link
Member

domenic commented Aug 14, 2017

Yep. These are all valid concerns about the thenable protocol. They apply to every API that stores and returns objects via promises. (For example, it's a breaking change to add a then method to an object stored in an in-memory get/set store; and people could add then methods to such objects to prevent them from being retrieved.)

A new proposal to solve that general issue would be a reasonable thing to bring the committee, but I believe it's out of scope for dynamic import.

@ljharb
Copy link
Member

ljharb commented Aug 14, 2017

Specifically for dynamic import, however, with modules the idea is that the ModuleRecord is reusable - that's why it's frozen. When it's thenable, however, this means that two different import()s can (and likely will) get two different resolution values.

Would it be unreasonable for import() to have special handling for when the ModuleRecord is a thenable (like perhaps only ever calling the expected "then" once, and caching the result)?

@domenic
Copy link
Member

domenic commented Aug 14, 2017

Yes, I believe that would be unreasonable. import() is like every other promise-returning/value-retrieving function. All of them can get different resolution values in the face of malicious then()ables.

@ljharb
Copy link
Member

ljharb commented Aug 14, 2017

It's quite different, though - it's function-like syntax, per the readme.

@domenic
Copy link
Member

domenic commented Aug 14, 2017

Let me rephrase: "its behavior is like every other promise-returning/value-retrieving function when a thenable is the value retrieved".

@ljharb
Copy link
Member

ljharb commented Aug 14, 2017

I'm not arguing against thenable coercion here, that's surprising but well-established - I'm asking whether new Set(await Promise.all([import(path), import(path)])).size === 1 can be made true for all modules, not just non-thenables. (Something another promise-returning function could certainly choose to ensure via memoization)

@domenic
Copy link
Member

domenic commented Aug 14, 2017

Right. And I'll say again: I think the answer should be the same for import(path) as for indexedDB.get(path). That is, always true if what you're getting behaves as a normal thenable, but if you customize the behavior of your thenable in strange ways, then you can get 2. I think it would be very surprising to make import() behave specially here.

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

No branches or pull requests

6 participants