Repository: vigetlabs/microcosm
Pull request #524 (2018-06-29 12:30:21)

Handle promise errors using second argument of then()
**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:

```javascript
const repo = new Microcosm()

repo.addDomain('test', {
  register() {
    return {
      action: () => {
        throw 'Oops'
      }
    }
  }
})

await repo.push('action') // => where did the error go?
```

codecov-io: # [Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/524?src=pr&el=h1) Report
> Merging [#524](https://codecov.io/gh/vigetlabs/microcosm/pull/524?src=pr&el=desc) into [master](https://codecov.io/gh/vigetlabs/microcosm/commit/ed603ac2a08ae72957def21fd171270a64d000ee?src=pr&el=desc) will **increase** coverage by `0.16%`.
> The diff coverage is `86.36%`.

[![Impacted file tree graph](https://codecov.io/gh/vigetlabs/microcosm/pull/524/graphs/tree.svg?token=AaE0FLz9sU&width=650&height=150&src=pr)](https://codecov.io/gh/vigetlabs/microcosm/pull/524?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #524      +/-   ##
==========================================
+ Coverage   93.88%   94.04%   +0.16%     
==========================================
  Files          32       32              
  Lines         801      806       +5     
  Branches      162      162              
==========================================
+ Hits          752      758       +6     
+ Misses         42       41       -1     
  Partials        7        7
```


| [Impacted Files](https://codecov.io/gh/vigetlabs/microcosm/pull/524?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [packages/microcosm/src/scheduler.js](https://codecov.io/gh/vigetlabs/microcosm/pull/524/diff?src=pr&el=tree#diff-cGFja2FnZXMvbWljcm9jb3NtL3NyYy9zY2hlZHVsZXIuanM=) | `95.34% <0%> (+4.43%)` | :arrow_up: |
| [packages/microcosm/src/coroutine.js](https://codecov.io/gh/vigetlabs/microcosm/pull/524/diff?src=pr&el=tree#diff-cGFja2FnZXMvbWljcm9jb3NtL3NyYy9jb3JvdXRpbmUuanM=) | `100% <100%> (ø)` | :arrow_up: |
| [packages/microcosm/src/subject.js](https://codecov.io/gh/vigetlabs/microcosm/pull/524/diff?src=pr&el=tree#diff-cGFja2FnZXMvbWljcm9jb3NtL3NyYy9zdWJqZWN0Lmpz) | `98.57% <66.66%> (-1.43%)` | :arrow_down: |
| [packages/microcosm/src/observable.js](https://codecov.io/gh/vigetlabs/microcosm/pull/524/diff?src=pr&el=tree#diff-cGFja2FnZXMvbWljcm9jb3NtL3NyYy9vYnNlcnZhYmxlLmpz) | `89.16% <93.33%> (+0.18%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/524?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/524?src=pr&el=footer). Last update [ed603ac...b11069f](https://codecov.io/gh/vigetlabs/microcosm/pull/524?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

nhunzaker: Thanks, @dce !

Patch:
diff --git a/packages/microcosm/src/coroutine.js b/packages/microcosm/src/coroutine.js
index 6fb960f5..1e218436 100644
--- a/packages/microcosm/src/coroutine.js
+++ b/packages/microcosm/src/coroutine.js
@@ -35,7 +35,8 @@ export function coroutine(
   }
 
   if (isPromise(body)) {
-    body.then(action.complete).catch(action.error)
+    body.then(action.complete, action.error)
+
     return
   }
 
diff --git a/packages/microcosm/src/observable.js b/packages/microcosm/src/observable.js
index 88ce4e7b..5842325c 100644
--- a/packages/microcosm/src/observable.js
+++ b/packages/microcosm/src/observable.js
@@ -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) {
diff --git a/packages/microcosm/src/scheduler.js b/packages/microcosm/src/scheduler.js
index 90b3d63c..18b4e431 100644
--- a/packages/microcosm/src/scheduler.js
+++ b/packages/microcosm/src/scheduler.js
@@ -106,9 +106,7 @@ class Scheduler {
       this._errorCallbacks.forEach(callback => callback(error))
       this._errorCallbacks.length = 0
     } else {
-      setTimeout(() => {
-        throw error
-      }, 0)
+      throw error
     }
   }
 
diff --git a/packages/microcosm/src/subject.js b/packages/microcosm/src/subject.js
index ba8b87d3..ccd92892 100644
--- a/packages/microcosm/src/subject.js
+++ b/packages/microcosm/src/subject.js
@@ -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
 }


Test patch:
diff --git a/packages/microcosm/test/helpers.js b/packages/microcosm/test/helpers.js
index 0395c309..68eb0a72 100644
--- a/packages/microcosm/test/helpers.js
+++ b/packages/microcosm/test/helpers.js
@@ -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
+  })
+}
diff --git a/packages/microcosm/test/unit/middleware/promise-middleware.test.js b/packages/microcosm/test/unit/middleware/promise-middleware.test.js
index e0073435..b2ede40c 100644
--- a/packages/microcosm/test/unit/middleware/promise-middleware.test.js
+++ b/packages/microcosm/test/unit/middleware/promise-middleware.test.js
@@ -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() {
@@ -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')
+    })
+  })
 })