Repository: vigetlabs/microcosm
Pull request #368 (2017-07-13 21:11:19)

#367 Coroutine can distinguish between thunks and pass-through params
See #367 

**Question**: Should we do this? Does this make anything harder to do? Theoretically this could be a breaking change if someone is returning an argument and expecting it to be executed as a thunk (passing a thunk action body as an argument from the `repo.push` callsite) which seems like a potentially legitimate use case.

**What kind of change does this PR introduce?**

- [x] Bugfix
- [x] Feature
- Code style update
- Refactor
- Build tools
- Documentation
- Other, please describe:

**Does this PR introduce a breaking change?**

- [x] Yes (maybe?)
- No

If yes, please describe the impact and migration path for existing applications:

If you were passing a function as an action parameter when calling `repo.push` or `send` and then returning it as the value from within the action body _and_ expecting that function-argument to be evaluated as a thunk as part of Microcosm's action resolution process, then you would need to wrap it in an anonymous function before returning it in the action body. e.g.

```js
// before
function maybeThunk(thunkFn, shouldThunk) {
  if (shouldThunk) {
    return thunkFn // I expect this to be called with (action, repo)
  }
}

// after
function maybeThunk(thunkFn, shouldThunk) {
  if (shouldThunk) {
    return (action, repo) => thunkFn(action, repo)
  }
}
```

When the action body's value is a function it is evaluated as a thunk _unless_ it's identity matches one of the passed-in action params. Wrapping the action in a lambda and passing the arguments through will circumvent this equality check and cause the thunk to be evaluated when microcosm rolls forward processing actions.

**Does this PR fulfill these requirements:**

- [x] All tests are passing
- [x] `yarn pretty` has been run
- [x] Commits reference open issues

**Additional Information**

See #367 


codecov-io: # [Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/368?src=pr&el=h1) Report
> Merging [#368](https://codecov.io/gh/vigetlabs/microcosm/pull/368?src=pr&el=desc) into [master](https://codecov.io/gh/vigetlabs/microcosm/commit/1c6bf7ace56fd8e806c370b38e4ad4ad34f8da9f?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

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

```diff
@@          Coverage Diff          @@
##           master   #368   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          26     26           
  Lines         865    865           
=====================================
  Hits          865    865
```


| [Impacted Files](https://codecov.io/gh/vigetlabs/microcosm/pull/368?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/coroutine.js](https://codecov.io/gh/vigetlabs/microcosm/pull/368?src=pr&el=tree#diff-c3JjL2Nvcm91dGluZS5qcw==) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/368?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/368?src=pr&el=footer). Last update [1c6bf7a...c8e8f1e](https://codecov.io/gh/vigetlabs/microcosm/pull/368?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

nhunzaker: _Subtle prod_. Anything I can do here to help out?
solomonhawk: Just need to make time - tomorrow morning I'll update this and the other PR and then get your approval!
solomonhawk: @nhunzaker Ready for another review. Updated the changelog, docs, and added a test.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c12f8953..3e915756 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,10 @@
   object) before the action gets dispatched.
 - Domain and effect registration handlers can be an array of functions that are processed sequentially.
 
+- Tweaked internal processing of actions to allow function arguments
+  to be returned from actions without being treated as a thunk. See
+  the docs for Actions for more information.
+
 # 12.11.0
 
 - Removed string `ref` in ActionForm, avoiding some edge cases and
diff --git a/docs/api/actions.md b/docs/api/actions.md
index a3f84e4d..c36d775e 100644
--- a/docs/api/actions.md
+++ b/docs/api/actions.md
@@ -117,6 +117,38 @@ function readPlanets () {
 repo.push(readPlanets)
 ```
 
+**Heads up:** Sometimes you want an action to return a function that isn't 
+executed as a thunk. If you pass a function as an action argument and return 
+that function in the action body, Microcosm _will not_ treat this as a thunk.
+
+```javascript
+function noThunk(fn) {
+  return fn // will not be called as a thunk by Microcosm
+}
+```
+
+If you need to return a function from an action body consider wrapping it in a 
+thunk:
+
+```javascript
+function actionReturnsFunction () {
+  return function (action) {
+    action.resolve(x => x) // the action's payload will be this inner function
+  }
+}
+```
+
+Alternatively, if you want Microcosm to treat your function action argument as 
+a thunk, you can wrap it in an anonymous function first.
+
+```javascript
+function maybeThunk(thunkFn, shouldThunk) {
+  if (shouldThunk) {
+    return (action, repo) => thunkFn(action, repo)
+  }
+}
+```
+
 ### Return a generator
 
 **Heads up:** Generators are a new feature included in the JS2015
diff --git a/docs/api/domains.md b/docs/api/domains.md
index 6b4f547b..9b6b88d8 100644
--- a/docs/api/domains.md
+++ b/docs/api/domains.md
@@ -219,6 +219,34 @@ const Planets = {
 repo.push(Actions.add, { name: 'earth' }) // this will add Earth
 ```
 
+#### Multiple handlers for the same action
+
+You can assign multiple handlers to an action by passing an array:
+
+```javascript
+import { addPlanet } from '../actions/planets'
+import { sortBy } from 'lodash'
+
+const Planets = {
+  //...
+  register () {
+    return {
+      [addPlanet]: [this.append, this.sort]
+    }
+  },
+  append (planets, params) {
+    return planets.concat(params)
+  },
+  sort (planets) {
+    return sortBy(planets, 'name')
+  }
+}
+
+repo.push(Actions.add, { name: 'earth' }) // this will add Earth
+```
+
+These handlers are processed from left to right, receiving the result of the prior handler as the first argument.
+
 ### `Domain.defaults`
 
 Specifies default options a Domain is instantiated with. This
diff --git a/docs/api/effects.md b/docs/api/effects.md
index accf19bf..9612afeb 100644
--- a/docs/api/effects.md
+++ b/docs/api/effects.md
@@ -151,6 +151,36 @@ repo.addEffect(Planets)
 repo.push(addPlanet, { name: 'earth' }) // this will add Earth
 ```
 
+#### Multiple handlers for the same action
+
+You can assign multiple handlers to an action by passing an array:
+
+```javascript
+// /src/effects/planets.js
+
+import { addPlanet } from '../actions/planets'
+
+class Planets {
+  //...
+  register () {
+    return {
+      [addPlanet.error]: [this.alert, this.trackError]
+    }
+  }
+
+  alert (repo, error) {
+    alert('There was an issue!)
+  }
+  
+  trackError (repo, error) {
+    myLogService.trackError(error)
+  }
+}
+
+repo.addEffect(Planets)
+repo.push(addPlanet, { name: 'earth' })
+```
+
 ### `Effect.defaults`
 
 Specifies default options a Effect is instantiated with. This
diff --git a/src/coroutine.js b/src/coroutine.js
index 2517f2e5..174f46e0 100644
--- a/src/coroutine.js
+++ b/src/coroutine.js
@@ -94,12 +94,14 @@ export default function coroutine(
   }
 
   /**
-   * Check for thunks. An escape hatch to direction work with an
+   * Check for thunks. An escape hatch to directly work with an
    * action. It is triggered by returning a function from a
    * command. This middleware will execute that function with the
-   * action as the first argument.
+   * action as the first argument. If the returned function is equal
+   * to one of the action's params, assume it's a value to be returned
+   * instead of a thunk.
    */
-  if (isFunction(body)) {
+  if (isFunction(body) && !params.some(param => param === body)) {
     body(action, repo)
 
     return action


Test patch:
diff --git a/test/unit/middleware/thunk-middleware.test.js b/test/unit/middleware/thunk-middleware.test.js
index e8f2ee1c..ee688cee 100644
--- a/test/unit/middleware/thunk-middleware.test.js
+++ b/test/unit/middleware/thunk-middleware.test.js
@@ -8,4 +8,18 @@ describe('Thunk middleware', function() {
 
     expect(spy).toHaveBeenCalledWith(action, repo)
   })
+
+
+  it('does not treat function action arguments as thunks when they are directly returned', function() {
+    expect.assertions(2)
+
+    let action = fn => fn
+    let spy = jest.fn()
+    let repo = new Microcosm()
+
+    repo.push(action, spy).onDone(result => {
+      expect(result).toEqual(spy)
+      expect(spy).not.toHaveBeenCalled()
+    })
+  })
 })