Fix issue with catching exceptions thrown by engage #50
Conversation
coltfred
requested review from
BobWall23,
giarc3 and
skeet70
and removed request for
a team
|
Commit SHA:419801d2031de141302a115ee49c7df3286512ec |
self review self review fix removed type
coltfred
force-pushed
the
fix-multiple-handleWith
branch
from
4623fc7 to
3e5a5fa
Compare
|
Commit SHA:26620d6af0c423cc2ca8015915c97b1cd61e9942 |
| } | ||
| ); | ||
| } catch (e) { | ||
| expect(handleWithCalledTimes).toBe(1); |
There was a problem hiding this comment.
I'm not sure I understand based on the description, where does this test show that it the handleWith wasn't run? It looks like it shows that it does, and it's not clear if it's from its own rejection or the rejection thrown in the engage.
There was a problem hiding this comment.
I changed the test to show that it's the error that came from the Future.reject
Future.ts
Outdated
| */ | ||
| toPromise(): Promise<R> { | ||
| return new Promise<R>((resolve: Resolve<R>, reject: Reject<L>) => this.engage(reject, resolve)); | ||
| return new Promise<R>((resolve: Resolve<R>, reject: Reject<L>) => this.engageAndCatch(reject, resolve)); |
There was a problem hiding this comment.
I think this can be engage? A throw from inside a Promise like this should get caught by the promise. Actually why does engageAndCatch exist with the safe flag like this at all? If we want it, can't we have noThrowEngage or engageAndCatch without the safe argument and always do the safe logic? Anywhere you would write engageAndCatch(x, y, false) you can write engage(x, y) instead right?
There was a problem hiding this comment.
You're right on both accounts. Fixed and added a test that shows that an error thrown in the future will be caught by the promise if in a chain.
|
Commit SHA:9564bb9d804936873833f4325fe34d747393029a |
This fixes #44. The test that's commented out is one that I don't believe should pass, but I'm open to discussion on it.
I would really like a more elegant solution, but I didn't find one that didn't result in an entire rewrite of the class besides this. I'm open to pursuing different ideas if we think this doesn't fix it appropriately. Also totally open to additional test cases.