Modules: Warn on `then`able required

[benjamingr]

Hey,

First of all sorry if this was discussed before - I haven't been an active modules team member for a while :]

How does everyone feel about warning on thenables being imported?

I raised this up after discussing this case with Kyle Simpson last year in the promise use cases session in the summit and then completely forgot about it. Basically something like:

// myModule.mjs
const thenable = {
  then(onFulfilled, onRejected) {
    // do something
  }
}
export default thenable;

Then on the other side if someone does import('./myModule.mjs') they will get (arguably) surprising results. It's probably not a common problem but a very annoying one when you do get it probably.

We can:

• Ignore this, as this is technically working as intended.
• Warn when this happens by default. (I've tried looking for places this is done intentionally and haven't found any).
• Expose a Symbol that allows opting out of this behaviour and importing the module as an object.
• Throw wheh someone imports a thenable.

[targos]

@benjamingr I don't think your example gives a surprising result.

I'm more concerned about something like this:

myModule.mjs:

export function then() {
  console.log("I'm executed during import");
}

app.mjs:

import('./myModule.mjs').then((mod) => console.log("I'm never executed"));

[getify]

I think the main issue is with dynamic imports, where 'import()' always gives a promise, which you then 'await' to get the module. The problem is, the await/then resolution completely unwraps a promise (of a promise of a promise of a...), so if your module export is a thenable, you're unexpectedly resolving "too deeply".

var x = await import("./myModule.mjs");

Here you want 'x' to just be your module's export, but it won't be if your module export has a 'then()' on it.

If your module is a well-behaving thrnable, or promise, that will be fully resolved before being assigned to 'x'. May be what you want, but likely not.

If your module has a 'then()' that never resolves (calls the fn), the dynamic import will hang forever.

But worst of all, if your export just has a 'then()' on it but it's not a thenable or promise at all, it's some entirely different kind of functionality that just happens to use that "special" name 'then', now your import fails in a most unexpected way, where the result of calling your export's 'then()' -- whatever that is, if not an exception -- is just subsumed and assigned to 'x'.

For all of the reasons, it's likely the dev who imports such a module would need to be warned that this dynamic import is probably going to misbehave.

Of course, if the module owner WANTS to have their export resolved by 'await', they should be able to make that work, too.

The ship already sailed long ago on avoiding accidental-thenables by having a promise brand symbol. But, we can (and should) still do the reverse: an anti-brand symbol, like 'Symbol.notThenable', which means that 'await' / 'then' do NOT resolve it, even if a 'then' is present.

Of course, that requires TC39 to take action. I think they will, as I've. seen @allenwb discuss this recently.

In the interim, for Node to start this exploration of the viability of such a solution, I think/propose:

• dynamic imports of a thenable do NOT throw warnings by default (since JS currently allows them).

• consumer of the module can turn on a flag in node like "--warn-import-thenable" that starts reporting these warnings, like "you may not realize what this import is doing..."

• with that flag on, if the owner of the module includes a "Symbol.notThenable" on their export, the warning changes to "this module should not be dynamically imported, because it has identified itself as not being compatible with await/then"

[benjamingr]

@targos

I'm more concerned about something like this:

Yes! That's essentially the same thing - a then export.

[targos]

No, that's no the same thing.

With export default thenable;, the thenable is not unwrapped because it's just a default property on the namespace object.
The only way to trigger this issue is by explicitly exporting a then binding. I'm not aware of any way to do this with an exported Promise or thenable object.

[devsnek]

I made a pr for this a while ago: https://github.com/nodejs/node/pull/20951

Expose a Symbol that allows opting out of this behaviour and importing the module as an object.

I brought that idea to tc39 and it was turned down: https://github.com/devsnek/proposal-symbol-thenable

[getify]

Even if TC39 drops the ball and doesn't fix this, Node should still do something like the above to help protect its users.

[devsnek]

we can do warnings and such, but we can't change the esm behaviour.

[getify]

I'm not suggesting Node change JS. I'm suggesting there be a mode with flags and optional symbols that devs can opt into, which allows them to be warned of inappropriate design/behavior, since TC39 has thus far failed to do so.

[benjamingr]

Even if TC39 drops the ball and doesn't fix this,
since TC39 has thus far failed to do so.

This form of interaction does not encourage collaboration well in Node.js and I would recommend against it here. We really value constructive and positive communication in the Node.js org and I would recommend focusing on what Node.js should do to protect its users.

Most of the answers to "why hasn't TC39 fixed X" is that X is hard and no one took the time to fix X while enduing the pain of dealing with all the edge cases.

For process, I recommend:

• Discussion with the module team (this is happening, great!)
• Making a PR to https://github.com/mcollina/make-promises-safe (cc @mcollina ) for this.
• The module team forwards concerns to our TC39 representatives and champions in Node.js (like @MylesBorins ) to bring to the committee and explore fixes at the spec level.
• At the same time, Node.js addresses the issue by emitting an event when this happens so that APMs can catch it, we can also turn on the warning by default at the next semver-major release of Node.js .

[benjamingr]

@TimothyGu thanks, I didn't realise this - I will update the original post.

---

@devsnek

I brought that idea to tc39 and it was turned down: https://github.com/devsnek/proposal-symbol-thenable

What if the symbol only opts out of thenable imports rather than generally unwrapping promises?

[ljharb]

@benjamingr that was rejected initially, which is why the proposal was brought forth - many delegates felt that differentiating thenable module namespace objects from thenable values would be inconsistent.

(note that Symbol.thenable was not rejected outright, but it was rejected for stage 1 - however it's unlikely we'll be able to bring it back in a form that gets consensus for stage 1)

Note as well, https://github.com/pemrouz/proposal-promise-result which hopes to find a way to generically handle import() of thenable modules differently.

[benjamingr]

@ljharb thanks! I didn't realize this discussion has already happened! Given the state of things - how do you propose we move foward? Would printing a warning be a good path forward to help users with this?

[ljharb]

I doubt it, given that one of the reasons Symbol.thenable didn't get consensus for stage 1 is that there already are legitimate use cases on the web of people using thenable modules, such as:

export default Something;
export function then(resolve) { resolve(Something); }

which allows:

import Something from 'path';
import('path').then(Something => {});
// instead of `import('path').then(({ default: Something }) => {});`

In other words, the general thought seemed to be that this was the realm of linters, not the runtime.

[benjamingr]

In other words, the general thought seemed to be that this was the realm of linters, not the runtime.

Oh, thanks, can you name one or two packages that do this export function then thing?

[devsnek]

mixture of horror and interest here: https://github.com/search?q=%22export+function+then%22&type=Code

[dnalborczyk]

mixture of horror and interest here

@devsnek I went through the list, pretty much almost no one is exporting "then" in their javascript code. and even then, some of it are test cases, some of it is a different spelling of then (e.g. Then, thenBy, thenAll etc.) all in all, it's a very tiny amount of people/projects actually using it.

more specific: https://github.com/search?p=2&q=%22export+function+then%22+extension%3Ajs&type=Code (can alter with mjs, ts as well)

[bmeck]

closing due to lack of movement, lint rules exist now as well https://eslint.org/docs/rules/no-restricted-exports