catch must re-throw

[xjamundx]

I've heard that catch() should re-throw the error or return Promise.reject(err) anyway familiar with this? It's so you don't break the promise chain.

[chrisui]

Hey @xjamundx

I was just going to suggest this till I saw this issue! :)

I have implemented this via ASTExplorer here! Happy to make a pull request if you would like.

Background: We have found that is definitely the exception in our codebase where you would actually want to opt-out of re-throwing/explicitly handling your errors and our developers should opt-in (via eslint-disable-line) if they really want this behaviour (which is rarely ever the case). Accidental promise swallowing is a real pain! :)

[xjamundx]

Sorry I'm finally getting back to this after like 6 months. I'm going to poke around your astexplorer code. Still seems like a good idea!

[r-murphy]

I'm willing to implement this. I was wondering if it could be included in always-return. I had it working on an older version of always-return but then it got refactored. So I just need to redo it. Shouldn't take too long.

In my implementation, there would be 3 possible failure messages from always-return.

'Each then() should return a value or throw' //the existing one
'Each then() error handler should return a value or rethrow' //2nd argument in .then(..)
'Each catch() should return a value or throw' //.catch

[r-murphy]

I redid my implementation with the refactored always-return. You can have a look on my fork. No doc updates yet. I'll wait to hear back on what you think of this being included in always-return.

I can put a config option for it too, but I think it should be enabled by default. I find missing a return or rethrow in a catch() causes more issues than missing a return in a then(). That's my thinking for including it in always-return.

[xjamundx]

THanks for doing this. Will check it out tomorrow!

[HyperBrain]

@r-murphy A catch should either resolve (to handle recovered errors and follow with the success path) or reject to follow the rejection path for unrecoverable errors.

So the rule should enforce either a resolve or a reject in the catch, but must error if the catch does not return at all. Such errors are very hard to find and track in the code because, if missing, it just resolves implicitly with undefined.

[r-murphy]

@HyperBrain Yes, I agree. And that's why my change was checking for. Are you seeing different behaviour?

[HyperBrain]

At least in the unit tests I'm missing a check that tests for an error like this one: hey.catch(() => { }); or hey.catch(e => { console.log(e) });

They should trigger an error, shouldn't they?

[r-murphy]

@HyperBrain Those error cases are covered in my unit tests. Here's the commit diff. https://github.com/r-murphy/eslint-plugin-promise/commit/370330101e64261e2637ca861e64911c772ef2a3

My commit was from 2016 so it probably won't even merge in anymore anyways.

[macklinu]

@r-murphy @HyperBrain thanks for reviving this discussion and linking to a patch. I would like to do some more reading on this to gain a better understanding. I also wonder where #12 fits into this issue, specifically:

// propagating the error is the default error handler for a promise, it can be omitted

something.then(null, function (err) {
  throw err;
});

// or

something.catch(function (err) {
  throw err;
});

I'm wondering if this should be a part of always-return or an entirely separate rule itself.

[HyperBrain]

@macklinu I think to make it sound, there need be separate rules. An interesting one would also be to restrict the promise error handler pattern itself - e.g. companies would likely restrict their devs to use only catch, but no then with an error handler parameter.

So for that a rule like error-handler with possible option values catch and then would be great to enforce a specific style.

The rule extension discussed here would also fit into it's own rule (instead of always-return) like error-handler-always-return - which would mean either a reject or a resolve must be explicitly given.

[MrLoh]

This is such an essential rule. Catch all blocks have already caused me so much pain. Any chance this will land soon?

[MrLoh]

I am building a little eslint plugin that warns about catch blocks that don't rethrow, this only supports async-await style code though, no Promise.catch though https://github.com/MrLoh/eslint-plugin-no-catch-all