-
Notifications
You must be signed in to change notification settings - Fork 46
Warn on then
able required
#333
Comments
@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")); |
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:
|
Yes! That's essentially the same thing - a |
No, that's no the same thing. With |
I made a pr for this a while ago: nodejs/node#20951
I brought that idea to tc39 and it was turned down: https://github.com/devsnek/proposal-symbol-thenable |
Even if TC39 drops the ball and doesn't fix this, Node should still do something like the above to help protect its users. |
we can do warnings and such, but we can't change the esm behaviour. |
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. |
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:
|
@TimothyGu thanks, I didn't realise this - I will update the original post.
What if the symbol only opts out of |
@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 |
@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 |
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. |
Oh, thanks, can you name one or two packages that do this |
mixture of horror and interest here: https://github.com/search?q=%22export+function+then%22&type=Code |
@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 |
closing due to lack of movement, lint rules exist now as well https://eslint.org/docs/rules/no-restricted-exports |
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
then
ables beingimport
ed?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:
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:
Symbol
that allows opting out of this behaviour and importing the module as an object.import
s athen
able.The text was updated successfully, but these errors were encountered: