Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle promise errors using second argument of then() #524

Merged
merged 1 commit into from
Jun 29, 2018

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Jun 29, 2018

What

This commit changes the way Subjects and Microcosm action resolution handle promises such that the error state is passed into the second argument of promise.then().

This prevents a case where callbacks following the success case could raise an exception, but it would be trapped by the .catch() API call. So errors would "disappear" in the following case:

const repo = new Microcosm()

repo.addDomain('test', {
  register() {
    return {
      action: () => {
        throw 'Oops'
      }
    }
  }
})

await repo.push('action') // => where did the error go?

@codecov-io
Copy link

codecov-io commented Jun 29, 2018

Codecov Report

Merging #524 into master will increase coverage by 0.25%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #524      +/-   ##
==========================================
+ Coverage   93.88%   94.13%   +0.25%     
==========================================
  Files          32       32              
  Lines         801      802       +1     
  Branches      162      162              
==========================================
+ Hits          752      755       +3     
+ Misses         42       40       -2     
  Partials        7        7
Impacted Files Coverage Δ
packages/microcosm/src/scheduler.js 95.34% <0%> (+4.43%) ⬆️
packages/microcosm/src/coroutine.js 100% <100%> (ø) ⬆️
packages/microcosm/src/subject.js 100% <100%> (ø) ⬆️
packages/microcosm/src/observable.js 89.16% <93.33%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed603ac...f645efc. Read the comment docs.

This commit changes the way Subjects and Microcosm action resolution
handle promises such that the error state is passed into the second
argument of `promise.then()`.

This prevents a case where callbacks following the success case could
raise an exception, but it would be trapped by the `.catch()` API call.
@nhunzaker nhunzaker changed the title Fix trapped error when responding to promises Handle promise errors using second argument of then() Jun 29, 2018
Copy link
Member

@dce dce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! Especially cool to see how the tests work.

@nhunzaker
Copy link
Contributor Author

Thanks, @dce !

@nhunzaker nhunzaker merged commit 9d8741d into master Jun 29, 2018
@nhunzaker nhunzaker deleted the error-handling branch December 14, 2018 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants