Skip to content

Commit

Permalink
Handle promise errors using second argument of then()
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nhunzaker committed Jun 29, 2018
1 parent ed603ac commit f645efc
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 28 deletions.
3 changes: 2 additions & 1 deletion packages/microcosm/src/coroutine.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ export function coroutine(
}

if (isPromise(body)) {
body.then(action.complete).catch(action.error)
body.then(action.complete, action.error)

return
}

Expand Down
48 changes: 26 additions & 22 deletions packages/microcosm/src/observable.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,28 +263,32 @@ function notifySubscription(subscription: *, type: *, value: *) {
let observer = subscription._observer

// This is what triggers subscription callbacks
switch (type) {
case NEXT:
observer.next(value)
break
case ERROR:
closeSubscription(subscription)
observer.error(value)
break
case COMPLETE:
closeSubscription(subscription)
observer.complete()
break
case CANCEL:
closeSubscription(subscription)
observer.cancel()
break
default:
assert(
false,
`Unrecognized type ${type}. This is an error internal to Microcosm. ` +
`Please file an issue: https://github.com/vigetlabs/microcosm/issues`
)
try {
switch (type) {
case NEXT:
observer.next(value)
break
case ERROR:
closeSubscription(subscription)
observer.error(value)
break
case COMPLETE:
closeSubscription(subscription)
observer.complete()
break
case CANCEL:
closeSubscription(subscription)
observer.cancel()
break
default:
assert(
false,
`Unrecognized type ${type}. This is an error internal to Microcosm. ` +
`Please file an issue: https://github.com/vigetlabs/microcosm/issues`
)
}
} catch (error) {
scheduler()._raise(error)
}

if (subscription.closed) {
Expand Down
4 changes: 1 addition & 3 deletions packages/microcosm/src/scheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,7 @@ class Scheduler {
this._errorCallbacks.forEach(callback => callback(error))
this._errorCallbacks.length = 0
} else {
setTimeout(() => {
throw error
}, 0)
throw error
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/microcosm/src/subject.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export class Subject extends Observable {
function fromPromise(promise: Promise<*>): Subject {
let subject = new Subject()

promise.then(subject.complete).catch(subject.error)
promise.then(subject.complete, subject.error)

return subject
}
Expand Down
12 changes: 12 additions & 0 deletions packages/microcosm/test/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,15 @@ export function asTree(history) {
}

export const delay = n => new Promise(resolve => setTimeout(resolve, n))

export const withUniqueScheduler = () => {
let oldScheduler = global.__MICROCOSM_SCHEDULER__

beforeEach(function() {
global.__MICROCOSM_SCHEDULER__ = null
})

afterEach(function() {
global.__MICROCOSM_SCHEDULER__ = oldScheduler
})
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Microcosm from 'microcosm'
import { Microcosm, scheduler } from 'microcosm'
import { withUniqueScheduler } from '../../helpers'

describe('Promise middleware', function() {
it('completes when a promise resolves', async function() {
Expand Down Expand Up @@ -69,4 +70,50 @@ describe('Promise middleware', function() {
expect(error).toBe('error')
}
})

describe('promise error trapping', () => {
withUniqueScheduler()

it('does not trap errors in domain handlers', async function() {
const repo = new Microcosm()
const onError = jest.fn()

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

scheduler().onError(onError)

await repo.push('action')

expect(onError).toHaveBeenCalledWith('Oops')
})

it('does not trap errors in effect handlers', async function() {
const repo = new Microcosm()
const onError = jest.fn()

repo.addEffect({
register() {
return {
action: () => {
throw 'Oops'
}
}
}
})

scheduler().onError(onError)

await repo.push('action')

expect(onError).toHaveBeenCalledWith('Oops')
})
})
})

0 comments on commit f645efc

Please sign in to comment.