💭 [lint] propose new rules around the correct usage of Promise

[ematipico]

I'd like to propose some rule to enforce some best practice around the usage of Promise.

1. Always use .catch if using .then:

 something.then(val) // invalid

2. Do not use new when using static methods:

new Promise.resolve(val) // invalid

3. Always return inside the .then:

foo.then(val => {
   processVal(val); // invalid, no return
})

[yassere]

1. Always use .catch if using .then:

There are promises that never reject. I don't think that Promise.allSettled() ever does, for example.

3. Always return inside the .then:

What's the argument against the example you posted? Isn't that just a normal Promise<void>?

[ematipico]

There are promises that never reject. I don't think that Promise.allSettled() ever does, for example.

We can limit the rule to cases that we know require .catch. After all also Node.js doesn't like unhanded promises.

What's the argument against the example you posted? Isn't that just a normal Promise?

Having return inside the .then will return a new promise, which would allow to chain other promises. Example:

promiseAction().then((value) => {
   processValue(value); 
   // here the chain is broken because we didn't return
}).then(() => {
  doSomethingElse();
});

In this example, doSomethingElse won't be called because we didn't return.

With the advent of async/away this pattern is less used but doing this is quite a standard:

sideEffect()
	.then(foo)
	.then(bar)
	.then(lorem)

In order achieve that, we have to make sure that we always return something.

[ematipico]

There are promises that never reject. I don't think that Promise.allSettled() ever does, for example.

I think we should force the .catch anyway. Take the following example:

Promise.allSettled([sidEffect1, sideEffect2]).then([result1, result2] => {
   	if (result2) {
		throw new Error();
		// alternatively, we call a function that throws an error by mistake
   	}
});

In this example the error gets swallowed because there's no .catch. Also, we might have a return and chain other promises. Hence, I would like to always promote the .catch

[yassere]

In this example, doSomethingElse won't be called because we didn't return.

I don't think that this is true. The then method always synchronously returns a promise that you can chain off of. That promise will just resolve with undefined if your callback completes without returning anything.

I think we should force the .catch anyway. Take the following example:

It may be possible to implement a narrow version of this rule that detects the specific mistake in that example, but I think it would feel very strange to enforce code like this:

promiseThatNeverRejects.then((data) => {
  functionThatNeverThrows(data);
}).catch(() => {
  // unreachable code that Rome insists on
});

[ematipico]

I don't think that this is true. The then method always synchronously returns a promise that you can chain off of. That promise will just resolve with undefined if your callback completes without returning anything.

Well it's partially true. Check this simple sandbox I made. Without return, the last log is printed first. With the return, the chain of promises is correctly executed.

https://codesandbox.io/s/mystifying-mclaren-14n8g?file=/src/index.js

I am not saying that what you're saying is false, actually my example was bad. So sorry. I think to enforce the return would help to keep the chain of promises inline, without having side effects like my small example.

It may be possible to implement a narrow version of this rule

A promise without a .catch is a bad promise. It means that as a programmer I am 100% sure that that code will never throw when it's not true, it's not possible to determine it (especially for a program that analyse a third party code). Also, not having the .catch will swallow errors which is something that caused lot issues to me in the past, when I was a junior with promises. Enforcing a .catch is a safety net and a good practice, I think.

[ljharb]

Even Promise.allSettled can reject, if the receiver’s constructor lacks the resolve method.

[yassere]

Check this simple sandbox I made

It's just showing some placeholder code for me. Can you check if it wasn't saved or if the problem is on my end?

A promise without a .catch is a bad promise.

While I still don't think that this is always true, I agree that it's usually true, so it does seem plausible that the overall code quality improvements from enforcing .catch could be worth any minor inconveniences.

[yassere]

Even Promise.allSettled can reject, if the receiver’s constructor lacks the resolve method.

Ah, I didn't know that. Thanks for the info.

[ematipico]

@yassere Could you try again with this URL please? https://codesandbox.io/s/gifted-goodall-qdtgh?file=/src/index.js

[yassere]

@ematipico Ah, I understand what you're saying now. If you want your promise chain to wait on a promise, then you need to return that promise from your handler. That's true, but that's not always necessary and I think that the following code is normal:

getData().then((data) => {
  const result = processSynchronously(data);
  dispatch(result);
}).catch((err) => {
  handleError(err);
});

I'm not trying to be negative. I do think these could be useful rules that would catch errors. But I also want to be cautious about rules that produce diagnostics on technically valid code and don't have reliable autofixes. I think we have a limited "budget" for how much we can expect people to manually change valid code to appease Rome, and I just want to be careful with it.

[ematipico]

Yeah this makes sense. What if the rules checks for the usage of at least two .then and if so, it checks for return inside the first .then. Does it make sense? Is it something that we could implement?

Also wonder if we'd be able to check this:

function a() {
  console.log()
}

promise.then(a).then(somethingElse);

In this small example the function a should have a return in order to have a correct execution of somethingElse AFTER a. Probably this is a case where we are not able to check/fix with the linter.

[ljharb]

That's incorrect, though. There's zero reason whatsoever that a needs to return anything; if somethingElse doesn't expect an argument, everything will work perfectly, as-is.

[ematipico]

Yes, but how many people know that? That's why I was wondering if we can have some linting around promises.

I mean, if I know that somethingElse doesn't need anything from a in terms of returned value OR it doesn't care about execution order, then why having it inside the second .then? It's confusing - although correct - from my point of view. Do I miss something?

Instead, if somethingElse is inside the second .then I would expect that the function needs to be executed after a AND/OR needs some value returned by the function a.

[ljharb]

That sounds like something a type system would handle, not a linter. There’s nothing about promises that suddenly enables static analysis of JS to know when a function is being called improperly.

[yassere]

Instead, if somethingElse is inside the second .then I would expect that the function needs to be executed after a

It does get executed after a

promise.then(a).then(somethingElse); is equivalent to:

const aPromise = promise.then(a);
aPromise.then(somethingElse);

somethingElse won't execute until aPromise resolves.

If a returns a promise, then aPromise will resolve when that returned promise does.

If a doesn't return anything, aPromise will resolve when a has completely executed.

If a internally does something like const promise = getData() but doesn't return that promise, that's the situation where somethingElse might execute before you want it to. But I agree with @ljharb that that's a mistake that would be difficult to lint for.

[ematipico]

Thank you for the feedback!

Nobody said anything around new Promise.resolve so I'd assume that it's a reasonable suggestion that could be implemented as lint rule, right?

Regarding the other two suggestions, let's forget about the return thing.

Is having a .catch a reasonable lint rule?

[ljharb]

A generic lint rule that could be useful is warning on new used on any non-constructor builtin, but that wouldn't be specific to Promises.

[yassere]

Nobody said anything around new Promise.resolve so I'd assume that it's a reasonable suggestion

That sounds reasonable to me, although I agree with @ljharb that a generic rule could be useful. We have lists of builtins in internal/compiler/scope/globals, so I think we could check if the callee of a JSNewExpression matches one of those (as either an identifier or member expression) and then compare it with a list of known builtin constructors.

Is having a .catch a reasonable lint rule?

Here are a few more things to consider if we want to implement a .catch rule.

promise.then(onFulfilled, onRejected) is a valid alternative to .catch, so we'd need to decide whether or not to allow it.

I think these would all work:

const promise = getData().then(doSomething);
promise.catch(handleErr);

function foo(str) {
  return getData(str).then(doSomething);
}

foo("api").catch(handleErr);

function addCatch(promise) {
  promise.catch(handleErr);
}

addCatch(somePromise.then(doSomething));

We can't determine if the .catch handler is actually added in those situations, but it's worth considering whether or not we would produce diagnostics on promises that are assigned, returned, or passed to a function.

Also, this comment is relevant.