loader: warn when the user imports a module with a thenable namespace #20951

devsnek · 2018-05-24T21:55:53Z

A module with a thenable namespace will most likely make the import fail
so warn the user when it happens. As an example, if your entry file
imports a thenable namespace and expects to continue execution after
getting it, node will currently exit with no message.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

guybedford

I think this sort of thing is good for user experience, although that said it will likely only come up for those who intentionally want a thenable top-level import... so I wonder if that might be seen as restricting.

I guess a lot of this depends on how likely we are to get a Symbol.thenable on module namespaces. What was the news from TC39 on this this week?

guybedford · 2018-05-24T22:02:44Z

lib/internal/modules/esm/loader.js


 const FunctionBind = Function.call.bind(Function.prototype.bind);

 const debug = require('util').debuglog('esm');

+const thenableNamespaces = new SafeWeakSet();
+const thenableWarning = (x) => `The namespace of ${x} is thenable. \
+See https://mdn.io/thenable for more information.`;


I'm not sure how obvious this is to users - perhaps The module ${x} cannot be resolved through a dynamic import because it has a "then" method.

the first paragraph of the linked article explains it pretty well, do you feel strongly about adding more detail here?

This should probably be updated now to reflect the new warning status - The namespace of ${x} exports an invalid thenable, resulting in top-level imports of this module never resolving..

guybedford · 2018-05-24T22:04:15Z

lib/internal/modules/esm/loader.js

@@ -73,7 +78,13 @@ class Loader {
  async import(specifier, parent) {
    const job = await this.getModuleJob(specifier, parent);
    const module = await job.run();


Job.run is only called for top-level imports, so if you provide the warning in the implementation of job.run you shouldn't need to separately cache the errors with a weak map.

i put it here to catch the entry file and dynamic imports, do you think i should go for all imports?

@devsnek I don't think we should do all imports, but as far as I'm aware, job.run will be called iff it is an entry file or dynamic import?

devsnek · 2018-05-24T22:08:29Z

@guybedford it got turned down, although they're considering some sort of "new dynamic import" that won't coerce module namespaces

guybedford · 2018-05-24T22:15:52Z

Ahh, ok, well you very nearly helped fix thenables, perhaps some other opportunities will open up here someday.

If import('x') will resolve thenables in the browser I guess we should get this working in Node actually and that sounds like a bug that it isn't. I'm not sure we can tell users off for expecting that to work though, feels a bit like policing.

Could we just restrict this warning to say the vm cases that might fail somehow?

devsnek · 2018-05-24T22:19:49Z

@guybedford normally i would agree with you about the policing but because v8 will just unref the promise if it knows it won't resolve, and if your entry file looks like

import('thenable').then((ns) => { /* rest of that code */ });

then node will exit 0 no errors no messages

guybedford · 2018-05-24T22:30:53Z

@devsnek oh you mean if the thenable isn't implemented correctly and doesn't call fulfill? I think that is ok actually, as I believe the same would happen in browsers - the dynamic import would just never resolve or reject.

devsnek · 2018-05-24T22:34:56Z

@guybedford i think its more likely they just don't know what thenables are and therefore have no basis for debugging this as node will just exit without giving any information

targos · 2018-05-24T22:47:51Z

lib/internal/safe_globals.js

@@ -23,4 +23,5 @@ const makeSafe = (unsafe, safe) => {

 exports.SafeMap = makeSafe(Map, class SafeMap extends Map {});
 exports.SafeSet = makeSafe(Set, class SafeSet extends Set {});
+exports.SafeWeakSet = makeSafe(WeakSet, class SafeWeakMap extends WeakSet {});


typo: class SafeWeakSet

guybedford · 2018-05-24T22:55:09Z

@guybedford i think its more likely they just don't know what thenables are and therefore have no basis for debugging this as node will just exit without giving any information

The same argument applies for any halting behaviours, and this is a problem for any API using promises. We cannot arbitrarily stop a use case just because there is a risk of an unexpected termination. And the use case is effectively being endorsed by TC39 so we can't just disable it.

How about we restricting this warning to only cases where the function length of the "then" function is zero?

devsnek · 2018-05-24T23:41:23Z

How about we restricting this warning to only cases where the function length of the "then" function is zero?

fantastic idea, i can do that.

A module with a thenable namespace will most likely make the import fail so warn the user when it happens. As an example, if your entry file imports a thenable namespace and expects to continue execution after getting it, node will currently exit with no message.

TimothyGu

I am -1 on this idea. Exporting a then function that takes no parameters is an effective and standard way of preventing a module from loaded dynamically, and as such a valid use case.

/cc @nodejs/modules @bmeck

benjamingr

While it might be a valid way to prevent dynamic import - it feels like quite a hack and breaks user expectations.

So I'm a big +1 on this - and I want to discuss a better way to do what @TimothyGu is asking for in the next modules meeting

mcollina

I'd prefer it we crashed in this case.

guybedford · 2018-05-25T10:58:50Z

I am -1 on this idea. Exporting a then function that takes no parameters is an effective and standard way of preventing a module from loaded dynamically, and as such a valid use case.

I believe the behaviour here would still be to exit Node silently, but there would at least be a warning at the same time.

ljharb · 2018-05-25T12:09:51Z

I don’t agree that preventing a module from being dynamically imported is a valid use case, first of all. Second, no new type of import is being considered or ever was.

However, the current suggested workarounds - which no then export can block - is to make a module that statically imports *, and re-export it with a name, and dynamically import that.

I’m not sure a warning is helpful, and i also think that the community will just land on the best practice being, never export a then function (just like the best practice is not to have a then method on your non-Promise object).

ljharb · 2018-05-25T12:12:04Z

I think a different approach might be, issue a warning when someone dynamically imports a thenable module that does not resolve after a period of time?

benjamingr · 2018-05-25T12:35:54Z

In either case I'd want this to be discussed by the modules team and decided upon with quorom.

Since this is contended here (and even if it wasn't) I think we should not land this before discussing it there anyway.

bmeck · 2018-05-25T13:22:23Z

I think that importing things with then should not error since spec doesn't say it should error / people can use it for things like having import() give a specific default as the resolved value rather than the full namespace:

const foo = {};
export {foo as default};
export function then(r = ()=>{}) {
  r(foo);
};

const foo = await import('foo');
// instead of
// const {default: foo} = await import('foo');

They can also be used to prevent import() entirely like stated above, and to emulate top level await. Since all of these seem like possible use cases and that it is now being shipped in a variety of places, I think we should keep consistency unless we have a compelling reason to remove these use cases, and a way to solve them without then() and import().

I do think that a warning would be a good signal of sorts like events.defaultMaxListeners. The signal seems like a good way to alleviate (not prevent) confusion about what might be going on. Doing it without any signal that the exporting of a thenable is unintentional seems a bit less clear. Time is one way to detect when to signal that something seems out of the ordinary, but probably should be configured somehow, just like defaultMaxListeners if we ship a warning. Also, we should probably ensure that we don't warn when someone statically imports these modules, but that does mean exposing the difference in static and dynamic import hooks a bit more clearly than I am comfortable with.

Overall, I am fairly neutral but think that enabling users to have some relief for this potential confusion is good. I don't think it should be an error, but do think we can signal a warning or have some recommendation about this even if it just ends up linting based. I did have an old PR for eslint that didn't gain traction: eslint/eslint#9180 . Either solution seems preferable to me than the status quo.

devsnek · 2018-05-25T13:27:21Z

@bmeck with the current code your example won't trigger this warning, since your then takes arguments.

bmeck · 2018-05-25T13:30:02Z

@devsnek fixed

benjamingr · 2018-05-25T13:32:31Z

Is there any reason we're having this discussion here and not in the modules repo?

If this is OK with the other participants - I would prefer to open an issue in the modules repo so other members of the modules team get a ping about this and are better informed for the meeting.

Otherwise - I would like to @-ping the modules team so everyone is aware of this.

I will let this comment live for a day and if no one objects will open an issue there.

bmeck · 2018-05-25T13:36:23Z

@benjamingr I think it is just here as a PR since it doesn't introduce features. CC'ing seems good to me though.

devsnek · 2018-05-25T13:39:14Z

/cc @nodejs/modules

on that subject, is there a way to request a review from the modules team the way a request from tsc can be requested in the ui?

benjamingr · 2018-05-25T13:41:00Z

on that subject, is there a way to request a review from the modules team the way a request from tsc can be requested in the ui?

I think adding it to the agenda (which I requested with the module-agenda tag) is the way we go about this. We could discuss the process around it in the meeting too if you'd like.

xtuc · 2018-06-05T06:52:11Z

Sorry if that's has already been discussed. What if a package takes advantage of that technique? The user will be warned even if it's the expected behavior.

devsnek · 2018-06-05T13:04:34Z

@xtuc right now it warns on arguments.length === 0 such that no resolve could be passed (it might also be rest args but I think it's fair to assume no one does rest args when they intend to thenable)

ljharb · 2018-06-05T14:00:42Z

it could also be arguments[0]

bmeck · 2018-06-05T14:05:46Z

@xtuc This is a warning about peculiarities that could cause unexpected consequences, I see it as similar to warnings when event listeners keep getting added and a warning is produced. You can silence warnings and even redirect them with command line flags: https://nodejs.org/api/cli.html .

Fishrock123 · 2018-06-06T19:16:26Z

I'm not really sure what the problem is here.

Does it treat the example module in the test case as if it were a promise chain?

jkrems

I'm -0.5 on this one unless this will realistically also trigger a warning in browsers. It seems like an early optimization (too early) before we know that this is actually an issue for people in practice.

Ah, I forgot about Timothy's comment...

targos · 2018-06-06T19:38:05Z

@TimothyGu

Exporting a then function that takes no parameters is an effective and standard way of preventing a module from loaded dynamically, and as such a valid use case.

To me, this looks like an argument in favor of this PR: If, as a user, I'm trying to import() a module that does that, I would be happy to see this warning. Without it, I would just lose time trying to understand why the import doesn't work.

benjamn · 2018-06-06T19:43:55Z

@targos @TimothyGu Yes, and if you don't like the warning, you can just add a dummy parameter to your exported then function, and never call it, or (better yet) call the second parameter with a rejection reason. So I agree that preventing dynamic import() is a valid use case, but one that can coexist with the proposed warning.

jdalton · 2018-06-06T20:20:43Z

One of the ideas raised at the Module WG meeting today is that this could be applied more generally. Instead of focusing on thenable namespace objects the warning could be applied to unresolved promises in general (much like uncaught promise exceptions today).

ljharb · 2018-06-06T21:29:59Z

… and additionally, perhaps that we shouldn't solve the problem of "a promise that never resolves" without solving it in a general way?

benjamingr · 2018-06-06T21:32:33Z

perhaps that we shouldn't solve the problem of "a promise that never resolves" without solving it in a general way?

That problem is literally the halting problem.

Solving it generally isn't what's important (or possible) IMO - being useful to users and making developers happy is. I'd like us to think of our users first.

This PR makes developers happier by telling them when there's an issue with their code.

jkrems · 2018-06-06T21:38:24Z

That problem is literally the halting problem.

That's rarely actually correct (in practice). There's multiple things you can statically detect. E.g. "hey, this promise can never resolve because nobody keeps a reference to its resolve function" can be detected. A similar mechanism could be used to detect bad then-ables. Yes, there's always the possibility that the promise will be later passed into some native function that manages to resolve it via some sort of backdoor. Or somebody might keep a reference to then but never actually call it. While the halting problem has no solution, most control flow analysis problems have useful approximations.

benjamingr · 2018-06-06T21:43:09Z

@jkrems you're agreeing with me, my point is that we shouldn't solve the general problem but be useful to users. One way would be to detect promises that were pending when they were GCd or when the process ended and log them for example.

One way would be to warn users when they wait for a module that's never ready to load. That's this PR :)

devsnek · 2018-06-06T21:45:30Z

@benjamingr it's not quite the halting problem but I think you raise an important point in that regard. nothing we as node can do will be able to 100% accurately detect this. we could get a lot closer if v8 added tooling for this but it still wouldn't be perfect.

I will always disagree with those saying preventing intended language operations is a valid use case. we shouldn't be trying to fight JavaScript.

I'm sympathetic to making this more of a general case but I don't think we currently have the methods to do so. seems like something for another pr?

jkrems · 2018-06-06T21:49:09Z

One way would be to warn users when they wait for a module that's never ready to load.

But my question above was - is that really a common enough use case for maintaining code in core? It seems like an endless code golf / nerd snipe opportunity for a feature that - afaict - nobody is actually asking for or needs. It only "helps" people who happen to export a function called then that takes no arguments. What function can you imagine that really fulfills that? This seems like we're solving for a user population of 0 or maybe 1.

benjamingr · 2018-06-06T21:55:09Z

@jkrems

It's not a lot of code to maintain on our end and for the few users that hit this (hopefully just a few) I hope it helps. I concede this isn't a "must" for Node.js.

@devsnek

it's not quite the halting problem

Assume by contradiction that there exists an automaton "given a program C and a promise P, does P settle in C?". Let A be an automaton that solves the question.

We construct automaton B that takes a program D. B runs the following program:

Create a new promise P
Run D
After D halts, resolve P with the value 1

Given a program, we call the automaton B on it and pass it and the created P to A. We claim that this automaton solves the halting problem. If the program halts then the promise resolves according to what B does, if the program does not halt then the promise never resolves according to what B does.

Thus, by assuming we can decide the "does a promise resolve?" problem we have been able to construct a decider for the halting problem which we know is not decidable. Contradiction.

So - not decidable, and not word-by-word the halting problem but very close and quite equivalent.

I agree with everything else you've written :)

ljharb · 2018-06-06T22:32:29Z

It doesn't have to be as academic as the halting problem :-) we could warn on any promise that goes unresolved for some number of minutes, for example (iow, an implicit, internal Promise.race)

benjamingr · 2018-06-07T07:27:29Z

@ljharb which is why I don't understand the objection to this PR, it doesn't aim to solve the general case but rather it helps the developer experience in this particular case.

I'm not sure "number of minutes" is a good metric - someone might have a promise for the server finishing and shutting down for example.

Feel free to open an issue in https://github.com/nodejs/promise-use-cases/issues to discuss heuristics - would love to hear any ideas that'd work well in your opinion.

devsnek · 2018-06-07T13:06:29Z

@benjamingr I figured out the discrepancy in how we are viewing this. I want to detect if it's possible that the then arguments will never be called. if someone exports a then and does some operation and then resolves it, we don't need to know that operation halts or doesn't halt, we can in theory just see that the intention was to resolve. it gets a bit more complex with argument reassignment and eval but I think in both those cases you're asking for a worse debug experience. this general idea seems like something we could approach v8 with if we thought it was worth it.

benjamingr · 2018-06-07T14:06:58Z

@devsnek I think the approach you took here on this PR is fine.

TimothyGu · 2018-06-07T20:55:08Z

@targos / @benjamn

Without [the warning], I would just lose time trying to understand why the import doesn't work.

We don't warn when people try to do any other odd thing that's part of the JavaScript language. If I were a beginner in JavaScript, I would love a warning every time I try to use == instead of ===. What makes exporting then special?

From my perspective, pointing out potential bugs in exporting then, just as any other JavaScript quirk, is the job of tooling (esp. linters), not of Node.js.

Better promise diagnostics are surely helpful, but outside of the scope of this PR. I'd prefer the discussion here be focused on exporting then specifically.

targos · 2018-06-07T21:10:23Z

@TimothyGu I was responding to your argument about someone willingly exporting a then to prevent dynamic import. The warning would be useful to the consumer of the module who didn't write the export and couldn't know what's wrong without looking at the source of the dependency.

guybedford reviewed May 24, 2018

View reviewed changes

targos reviewed May 24, 2018

View reviewed changes

devsnek force-pushed the feature/warn-on-thenable-namespace branch from eb5dc14 to fc2ad59 Compare May 25, 2018 00:26

devsnek added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 25, 2018

TimothyGu suggested changes May 25, 2018

View reviewed changes

TimothyGu requested a review from bmeck May 25, 2018 07:38

benjamingr approved these changes May 25, 2018

View reviewed changes

benjamingr added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label May 25, 2018

mcollina requested changes May 25, 2018

View reviewed changes

MylesBorins mentioned this pull request Jun 5, 2018

Node.js Foundation Modules Team Meeting 2018-06-06 nodejs/modules#124

Closed

jkrems reviewed Jun 6, 2018

View reviewed changes

Fishrock123 previously approved these changes Jun 6, 2018

View reviewed changes

devsnek closed this Jul 9, 2018

devsnek deleted the feature/warn-on-thenable-namespace branch July 9, 2018 19:24

devsnek mentioned this pull request May 24, 2019

Warn on thenable required nodejs/modules#333

Closed

loader: warn when the user imports a module with a thenable namespace #20951

loader: warn when the user imports a module with a thenable namespace #20951

Conversation

devsnek commented May 24, 2018

Checklist

guybedford left a comment

Choose a reason for hiding this comment

guybedford May 24, 2018

Choose a reason for hiding this comment

devsnek May 24, 2018

Choose a reason for hiding this comment

guybedford May 25, 2018

Choose a reason for hiding this comment

guybedford May 24, 2018

Choose a reason for hiding this comment

devsnek May 24, 2018

Choose a reason for hiding this comment

guybedford May 25, 2018

Choose a reason for hiding this comment

devsnek commented May 24, 2018

guybedford commented May 24, 2018

devsnek commented May 24, 2018

guybedford commented May 24, 2018

devsnek commented May 24, 2018

targos May 24, 2018

Choose a reason for hiding this comment

guybedford commented May 24, 2018

devsnek commented May 24, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

guybedford commented May 25, 2018

ljharb commented May 25, 2018

ljharb commented May 25, 2018

benjamingr commented May 25, 2018

bmeck commented May 25, 2018 • edited Loading

devsnek commented May 25, 2018

bmeck commented May 25, 2018

benjamingr commented May 25, 2018

bmeck commented May 25, 2018

devsnek commented May 25, 2018 • edited Loading

benjamingr commented May 25, 2018

xtuc commented Jun 5, 2018

devsnek commented Jun 5, 2018

ljharb commented Jun 5, 2018

bmeck commented Jun 5, 2018

Fishrock123 commented Jun 6, 2018

jkrems left a comment

Choose a reason for hiding this comment

targos commented Jun 6, 2018

benjamn commented Jun 6, 2018

jdalton commented Jun 6, 2018

ljharb commented Jun 6, 2018

benjamingr commented Jun 6, 2018

jkrems commented Jun 6, 2018 • edited Loading

benjamingr commented Jun 6, 2018

devsnek commented Jun 6, 2018

jkrems commented Jun 6, 2018

benjamingr commented Jun 6, 2018

ljharb commented Jun 6, 2018

benjamingr commented Jun 7, 2018

devsnek commented Jun 7, 2018

benjamingr commented Jun 7, 2018

TimothyGu commented Jun 7, 2018 • edited Loading

targos commented Jun 7, 2018

bmeck commented May 25, 2018 •

edited

Loading

devsnek commented May 25, 2018 •

edited

Loading

jkrems commented Jun 6, 2018 •

edited

Loading

TimothyGu commented Jun 7, 2018 •

edited

Loading