Closed sbdchd closed 5 years ago
This seems like a nice addition, either a separate rule or an extension to no-throw.
Fixed in #116
Sorry I'm a bit late to this. I understand the reasoning for this but I feel it is a bit of an anti-pattern for Promises.
In the example given, the return type is marked as Promise<number>
; this could be though of as being equivalent to something like { value: number } | { error: Error }
. The error state is not hidden here.
When the function returns, it would essential invoke a callback using logic something along the lines of the following.
if (result.error !== undefined) {
handler.catch(result.error);
} else {
handler.then(result.value);
}
Thus promises do not have a hidden error state using Promise.reject()
, the code simply forks (which can still be considered functional).
buzz()
.then(r => {
console.log(r);
})
.catch(err => {
console.error("error:", err);
});
The issue with using Promise<number | Error>
as a return type is that it basically undoes everything Promise were designed for and sends you back into callback land (aka callback hell).
Hidden error state can exist in Promise but this happens with async
and await
, not with Promise themselves.
For example:
async function foo(): Promise<number> {
const x = await buzz(); // If buzz rejects, foo will reject too - i.e hidden error state exist here.
return x + 2;
}
async function buzz(): Promise<number> {
if (Math.random() > 0.5) {
return Promise.reject(new Error("bad"));
}
return 1;
}
Hidden error state can be remove from this in several ways, here is one:
async function foo(): Promise<number> {
const x = await buzz().catch(err => { // Hidden error state is explicitly caught making it no longer hidden.
console.error("error:", err);
return err;
});
return (
x instanceof Error
? Promise.reject(x)
: x + 2
);
}
async function buzz(): Promise<number> {
if (Math.random() > 0.5) {
return Promise.reject(new Error("bad"));
}
return 1;
}
Creating a linting rule to catch hidden error state in Promises does not seem at all like an easy task.
I feel this addition to the rule should be reverted. Or possibly just made a option users can manually enable.
I think maybe it should be a separate rule. The name no-throw
implies it applies to throw
only. So maybe we could move it to a new rule called no-reject
, no-promise-reject
or something like that.
@sbdchd @RebeccaStevens Could you please check if #118 seems OK to solve this?
LGTM
Seems good to me. I haven't tested it but I assume you have :stuck_out_tongue_winking_eye:
There was already a test added by @sbdchd so I just moved it :-).
The new no-reject rule is now released in 5.3.0 and the no-throw rule is reverted to the old behaviour. The previous version should probably have been a major release since it was not strictly additive (it added more checks to no-throw but also it altered its default behaviour). And following that, a new major version to change it back (altering the default behaviour back). However, since we are back to old behaviour I did not bother to do a major release for the latest change either.
TL;DR: suggestion to update
no-throw
to ban usage ofPromise.reject()
Essentially this would make promises behave more like non-async code.
Currently with
no-throw
,throw
is banned, so the following errorsand if we use async await we also get an error
but we don't get an error if we explicitly use
Promise.reject()
This is a problem as it represents the same hidden error state as the previous examples, which the rule errors on.
A more functional approach would be to instead return a union or result.
Then when used, both failure and success are warned about