Repository: vigetlabs/microcosm
Pull request #279 (2017-03-30 11:31:42)

General action refactor
Changes related to actions:

- Action methods are assigned manually, instead of looping over a map
  of statuses
- Renamed `action.disposable` to `action.complete`
- History listens to action changes, instead of actions telling
  history to reconcile

Other changes:

- The uglify step now mangles propertes that start with
  underscores, like: `_callOrSubscribeOnce`

Interestingly, there is 100 more lines of code, but the gzipped payload is actually smaller.

codecov-io: # [Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/279?src=pr&el=h1) Report
> Merging [#279](https://codecov.io/gh/vigetlabs/microcosm/pull/279?src=pr&el=desc) into [master](https://codecov.io/gh/vigetlabs/microcosm/commit/5e09e3f6bad206ab5f2fcf79180b491202f3be83?src=pr&el=desc) will **decrease** coverage by `0.12%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
- Coverage     100%   99.87%   -0.13%     
==========================================
  Files          23       23              
  Lines         762      774      +12     
==========================================
+ Hits          762      773      +11     
- Misses          0        1       +1
```


| [Impacted Files](https://codecov.io/gh/vigetlabs/microcosm/pull/279?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/constants.js](https://codecov.io/gh/vigetlabs/microcosm/pull/279?src=pr&el=tree#diff-c3JjL2NvbnN0YW50cy5qcw==) | `100% <ø> (ø)` | :arrow_up: |
| [src/history.js](https://codecov.io/gh/vigetlabs/microcosm/pull/279?src=pr&el=tree#diff-c3JjL2hpc3RvcnkuanM=) | `99% <100%> (-1%)` | :arrow_down: |
| [src/action.js](https://codecov.io/gh/vigetlabs/microcosm/pull/279?src=pr&el=tree#diff-c3JjL2FjdGlvbi5qcw==) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/279?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/279?src=pr&el=footer). Last update [5e09e3f...13b19b1](https://codecov.io/gh/vigetlabs/microcosm/pull/279?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
mackermedia: Hard to understand the context around this change. Code looks 👍  except for one question on `STATUSES`.
nhunzaker: @mackermedia I want to eventually move over to using es6 classes. Assigning directly to the prototype is pretty wonky looking for that case. Also, I didn't like how a bunch of methods were getting added to Action from a configuration file. I thought it would make it easier, but it just made it harder to understand.
mackermedia: I wouldn't say it made it harder to understand... I don't know enough about the general codebase to say that. Generally I'd prefer to work with es6 classes over prototypes, so you have my 👍 

Patch:
diff --git a/rollup.config.js b/rollup.config.js
index 346e1f27..11465fb7 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -25,7 +25,12 @@ if (process.env.NODE_ENV === 'production') {
       compress: {
         passes: 5
       },
-      mangle: { toplevel: true }
+      mangleProperties: {
+        regex: /^_/
+      },
+      mangle: {
+        toplevel: true
+      }
     })
   )
 }
diff --git a/src/action.js b/src/action.js
index 1a77cdd6..e6410ebf 100644
--- a/src/action.js
+++ b/src/action.js
@@ -1,56 +1,173 @@
 import Emitter from './emitter'
-import History from './history'
 import tag from './tag'
 
 import {
   inherit
 } from './utils'
 
-import {
-  STATES
-} from './constants'
+let uid = 0
 
 /**
  * Actions encapsulate the process of resolving an action creator. Create an
  * action using `Microcosm::push`:
+ * @constructor
+ * @param {Function|string} command
+ * @param {string} [status] Starting status
  */
-export default function Action (command, status, history) {
+export default function Action (command, status) {
   Emitter.call(this)
 
-  this.history = history || new History()
-
-  this.id = this.history.getId()
+  this.id = uid++
   this.command = tag(command)
   this.timestamp = Date.now()
   this.children = []
 
   if (status) {
-    this.setStatus(status)
+    console.assert(this[status], 'Unexpected task status ' + status)
+    this[status]()
   }
 }
 
 inherit(Action, Emitter, {
-  status     : 'inactive',
-  payload    : undefined,
-  disabled   : false,
-  disposable : false,
-  parent     : null,
-  next       : null,
+  status   : 'inactive',
+  payload  : undefined,
+  disabled : false,
+  complete : false,
+  parent   : null,
+  next     : null,
+
+  /**
+   * Open state. The action has started, but has received no response.
+   * @param {*} [nextPayload]
+   */
+  open (nextPayload) {
+    this._setPayload.apply(this, arguments)
+    this._setStatus('open', false)
+
+    return this
+  },
+
+  /**
+   * Update state. The action has received an update, such as loading progress.
+   * @param {*} [nextPayload]
+   */
+  update (nextPayload) {
+    this._setPayload.apply(this, arguments)
+    this._setStatus('update', false)
+
+    return this
+  },
+
+  /**
+   * Resolved state. The action has completed successfully.
+   * @param {*} [nextPayload]
+   */
+  resolve (nextPayload) {
+    this._setPayload.apply(this, arguments)
+    this._setStatus('resolve', true)
+
+    return this
+  },
+
+  /**
+   * Failure state. The action did not complete successfully.
+   * @param {*} [nextPayload]
+   */
+  reject (nextPayload) {
+    this._setPayload.apply(this, arguments)
+    this._setStatus('reject', true)
+
+    return this
+  },
+
+  /**
+   * Cancelled state. The action was halted, like aborting an HTTP
+   * request.
+   * @param {*} [nextPayload]
+   */
+  cancel (nextPayload) {
+    this._setPayload.apply(this, arguments)
+    this._setStatus('cancel', true)
 
+    return this
+  },
+
+  /**
+   * Subscribe to when an action opens.
+   * @param {Function} callback
+   * @param {*} [scope]
+   */
+  onOpen (callback, scope) {
+    this._callOrSubscribeOnce('open', callback, scope)
+    return this
+  },
+
+  /**
+   * Subscribe to when a action updates
+   * @param {Function} callback
+   * @param {*} [scope]
+   */
+  onUpdate (callback, scope) {
+    return this.on('update', callback, scope)
+  },
+
+  /**
+   * Subscribe to when a action resolves
+   * @param {Function} callback
+   * @param {*} [scope]
+   */
+  onDone (callback, scope) {
+    this._callOrSubscribeOnce('resolve', callback, scope)
+    return this
+  },
+
+  /**
+   * Subscribe to when a action rejects
+   * @param {Function} callback
+   * @param {*} [scope]
+   */
+  onError (callback, scope) {
+    this._callOrSubscribeOnce('reject', callback, scope)
+    return this
+  },
+
+  /**
+   * Subscribe to when a action is cancelled
+   * @param {Function} callback
+   * @param {*} [scope]
+   */
+  onCancel (callback, scope) {
+    this._callOrSubscribeOnce('cancel', callback, scope)
+    return this
+  },
+
+  /**
+   * @param {string} type
+   * @returns {boolean}
+   */
   is (type) {
     return this.command[this.status] === this.command[type]
   },
 
+  /**
+   * @param {boolean} [silent]
+   * @return {this}
+   */
   toggle (silent) {
     this.disabled = !this.disabled
 
     if (!silent) {
-      this.history.reconcile(this)
+      this._emit('change', this)
     }
 
     return this
   },
 
+  /**
+   * @param {?Function} pass
+   * @param {Function} [fail]
+   * @returns {Promise}
+   */
   then (pass, fail) {
     return new Promise((resolve, reject) => {
       this.onDone(resolve)
@@ -58,11 +175,12 @@ inherit(Action, Emitter, {
     }).then(pass, fail)
   },
 
-  setStatus (status) {
-    console.assert(STATES[status], 'Invalid action status "' + status + '"')
-
-    this.status = status
-    this.disposable = STATES[status].disposable
+  /**
+   * Is this action connected?
+   * @return {boolean}
+   */
+  isDisconnected () {
+    return !this.parent
   },
 
   /**
@@ -87,7 +205,7 @@ inherit(Action, Emitter, {
   },
 
   /**
-   * Add an action to the list of children
+   * Add action to the list of children
    * @param {Action} child Action to include in child list
    */
   adopt (child) {
@@ -100,9 +218,22 @@ inherit(Action, Emitter, {
     child.parent = this
   },
 
+  /**
+   * Connect the parent node to the next node. Removing this action
+   * from history.
+   */
+  remove () {
+    console.assert(!this.isDisconnected(), 'Action has already been removed.')
+
+    this.parent.abandon(this)
+
+    this.removeAllListeners()
+  },
+
   /**
    * Remove a child action
    * @param {Action} child Action to remove
+   * @private
    */
   abandon (child) {
     let index = this.children.indexOf(child)
@@ -111,85 +242,66 @@ inherit(Action, Emitter, {
       this.children.splice(index, 1)
       child.parent = null
     }
-  },
 
-  /**
-   * Connect the parent node to the next node. Removing this action
-   * from history.
-   */
-  remove () {
-    /**
-     * If an action the oldest child of a parent, pass on the lead
-     * role to the next child.
-     */
-    if (this.parent.next === this) {
-      this.parent.lead(this.next)
+    // If the action is the oldest child of a parent, pass
+    // on the lead role to the next child.
+    if (this.next === child) {
+      this.lead(child.next)
     }
-
-    this.parent.abandon(this)
-
-    // Remove all relations
-    this.next = null
   },
 
   /**
-   * Indicates if an action is currently connected within
-   * a history.
-   * @returns {Boolean} Is the action connected to a parent?
+   * Reset the payload
+   * @private
    */
-  isDisconnected () {
-    return this.parent == null
-  }
-
-})
-
-Object.defineProperty(Action.prototype, 'type', {
-  get () {
-    return this.command[this.status]
-  }
-})
-
-/**
- * Generate action methods for each action state
- */
-Object.keys(STATES).forEach(function (status) {
-  const { once, disposable, listener } = STATES[status]
+  _setPayload (payload) {
+    if (arguments.length) {
+      this.payload = payload
+    }
+  },
 
   /**
-   * Create a method to update the action status. For example:
-   * action.done({ id: 'earth' })
+   * Change the status of the action
+   * @private
+   * @param {string} status The next action status
+   * @param {boolean} complete Should the action be complete?
    */
-  Action.prototype[status] = function (payload) {
-    if (!this.disposable) {
+  _setStatus (status, complete) {
+    if (!this.complete) {
       this.status = status
-      this.disposable = disposable
-
-      if (arguments.length) {
-        this.payload = payload
-      }
-
-      this.history.reconcile(this)
+      this.complete = complete
 
+      this._emit('change', this)
       this._emit(status, this.payload)
     }
-
-    return this
-  }
+  },
 
   /**
-   * Create a method to subscribe to a status. For example:
-   * action.onDone({ id: 'earth' })
+   * If an action is already a given status, invoke the
+   * provided callback. Otherwise setup a one-time listener
+   * @private
+   * @param {string} status
+   * @param {Function} callback
+   * @param {*} scope
    */
-  Action.prototype[listener] = function (callback, scope) {
-    if (callback) {
-      if (once && this.status === status) {
+  _callOrSubscribeOnce (status, callback, scope) {
+    if (!!callback) {
+      console.assert(typeof callback === 'function',
+                     `Expected a function when subscribing to ${status}` +
+                     `instead got ${typeof callback}`)
+
+      if (this.is(status)) {
         callback.call(scope, this.payload)
       } else {
         this.once(status, callback, scope)
       }
     }
-
-    return this
   }
 
 })
+
+Object.defineProperty(Action.prototype, 'type', {
+  get () {
+    return this.command[this.status]
+  }
+})
diff --git a/src/constants.js b/src/constants.js
deleted file mode 100644
index 5d62171d..00000000
--- a/src/constants.js
+++ /dev/null
@@ -1,51 +0,0 @@
-/**
- * Actions move through a specific set of states. This manifest
- * controls how they should behave.
- */
-
-export const STATES = {}
-
-/**
- * Inactive state. This is the starting state of an action
- */
-STATES['inactive'] = { disposable: false, once: true,  listener: 'onStart' }
-
-/**
- * Open state. The action has started, but has received no response
- */
-STATES['open'] = { disposable: false, once: true,  listener: 'onOpen' }
-
-/**
- * Update state. The action has received an update, such as loading progress.
- */
-STATES['update'] = { disposable: false, once: false, listener: 'onUpdate' }
-
-/**
- * Resolved state. The action has completed successfully.
- */
-STATES['resolve'] = { disposable: true,  once: true,  listener: 'onDone' }
-
-/**
- * Failure state. The action did not complete successfully.
- */
-STATES['reject'] = { disposable: true,  once: true,  listener: 'onError' }
-
-/**
- * Cancelled state. The action was halted, like aborting an HTTP request.
- */
-STATES['cancel'] = { disposable: true,  once: true,  listener: 'onCancel' }
-
-
-// For nested registrations, track our aliases
-export const ALIASES = {
-  inactive  : 'inactive',
-  open      : 'open',
-  update    : 'loading',
-  loading   : 'update',
-  done      : 'resolve',
-  resolve   : 'done',
-  reject    : 'error',
-  error     : 'reject',
-  cancel    : 'cancelled',
-  cancelled : 'cancel'
-}
diff --git a/src/get-registration.js b/src/get-registration.js
index d40b6f56..079a85d2 100644
--- a/src/get-registration.js
+++ b/src/get-registration.js
@@ -1,5 +1,17 @@
 import { isObject } from './utils'
-import { ALIASES } from './constants'
+
+export const ALIASES = {
+  inactive  : 'inactive',
+  open      : 'open',
+  update    : 'loading',
+  loading   : 'update',
+  done      : 'resolve',
+  resolve   : 'done',
+  reject    : 'error',
+  error     : 'reject',
+  cancel    : 'cancelled',
+  cancelled : 'cancel'
+}
 
 /**
  * Gets any registrations that match a given command and status.
diff --git a/src/history.js b/src/history.js
index 9aa15dc9..7b867234 100644
--- a/src/history.js
+++ b/src/history.js
@@ -19,7 +19,6 @@ import {
 export default function History (limit) {
   Emitter.call(this)
 
-  this.ids = 0
   this.size = 0
   this.limit = Math.max(1, limit || 1)
   this.repos = []
@@ -123,10 +122,6 @@ inherit(History, Emitter, {
     this.append(START, 'resolve')
   },
 
-  getId () {
-    return ++this.ids
-  },
-
   addRepo (repo) {
     this.repos.push(repo)
   },
@@ -149,13 +144,13 @@ inherit(History, Emitter, {
   },
 
   append (command, status) {
-    let action = new Action(command, status, this)
+    let action = new Action(command, status)
 
     if (this.size > 0) {
       this.head.lead(action)
     } else {
       // Always have a parent node, no matter what
-      let birth = new Action(BIRTH, 'resolve', this)
+      let birth = new Action(BIRTH, 'resolve')
       birth.adopt(action)
 
       this.root = action
@@ -166,6 +161,8 @@ inherit(History, Emitter, {
 
     this.invoke('createInitialSnapshot', action)
 
+    action.on('change', this.reconcile, this)
+
     this._emit('append', action)
 
     return this.head
@@ -237,7 +234,7 @@ inherit(History, Emitter, {
     let size = this.size
     let root = this.root
 
-    while (size > this.limit && root.disposable) {
+    while (size > this.limit && root.complete) {
       size -= 1
       this.invoke('removeSnapshot', root.parent)
       root = root.next


Test patch:
diff --git a/test/unit/action/cancelled.test.js b/test/unit/action/cancelled.test.js
index 5ed0bcb6..6701bc71 100644
--- a/test/unit/action/cancelled.test.js
+++ b/test/unit/action/cancelled.test.js
@@ -15,12 +15,12 @@ describe('Action cancelled state', function () {
     expect(callback).toHaveBeenCalled()
   })
 
-  it('becomes disposable when cancelled', function () {
+  it('becomes complete when cancelled', function () {
     const action = new Action(identity)
 
     action.cancel()
 
-    expect(action.disposable).toBe(true)
+    expect(action.complete).toBe(true)
   })
 
   it('exposes a cancelled type when cancelled', function () {
diff --git a/test/unit/action/disposable.test.js b/test/unit/action/complete.test.js
similarity index 61%
rename from test/unit/action/disposable.test.js
rename to test/unit/action/complete.test.js
index 6715e6c0..bf98c124 100644
--- a/test/unit/action/disposable.test.js
+++ b/test/unit/action/complete.test.js
@@ -3,30 +3,30 @@ import Microcosm from '../../../src/microcosm'
 
 const identity = n => n
 
-describe('Action disposable state', function () {
+describe('Action complete state', function () {
 
-  it('actions are disposable when they resolve', function () {
+  it('actions are complete when they resolve', function () {
     const action = new Action(identity)
 
     action.resolve(true)
 
-    expect(action.disposable).toBe(true)
+    expect(action.complete).toBe(true)
   })
 
-  it('actions are disposable when they cancel', function () {
+  it('actions are complete when they cancel', function () {
     const action = new Action(identity)
 
     action.cancel(true)
 
-    expect(action.disposable).toBe(true)
+    expect(action.complete).toBe(true)
   })
 
-  it('actions are disposable when they fail', function () {
+  it('actions are complete when they fail', function () {
     const action = new Action(identity)
 
     action.reject(true)
 
-    expect(action.disposable).toBe(true)
+    expect(action.complete).toBe(true)
   })
 
   it('will not change states if already disposed', function () {
diff --git a/test/unit/action/disabled.test.js b/test/unit/action/disabled.test.js
index f8336c42..35290660 100644
--- a/test/unit/action/disabled.test.js
+++ b/test/unit/action/disabled.test.js
@@ -26,12 +26,13 @@ describe('Action disabled state', function () {
 
   it('can toggle history without triggering a reconciliation', function () {
     const action = new Action(identity)
+    const handler = jest.fn()
 
-    jest.spyOn(action.history, 'reconcile')
+    action.on('change', handler)
 
     action.toggle('silently')
 
-    expect(action.history.reconcile).not.toHaveBeenCalled()
+    expect(handler).not.toHaveBeenCalled()
   })
 
 })
diff --git a/test/unit/action/error.test.js b/test/unit/action/error.test.js
index c9145402..648c2a72 100644
--- a/test/unit/action/error.test.js
+++ b/test/unit/action/error.test.js
@@ -43,7 +43,7 @@ describe('Action error state', function () {
     expect(fn).toHaveBeenCalledWith(true)
   })
 
-  it('does not trigger an error event if it is disposable', function () {
+  it('does not trigger an error event if it is complete', function () {
     const action = new Action(identity)
     const spy = jest.fn()
 
diff --git a/test/unit/action/open.test.js b/test/unit/action/open.test.js
index 0b4ec050..4c5aa1e5 100644
--- a/test/unit/action/open.test.js
+++ b/test/unit/action/open.test.js
@@ -32,7 +32,7 @@ describe('Action open state', function () {
     expect(action).toHaveStatus('open')
   })
 
-  it('does not trigger an open event if it is disposable', function () {
+  it('does not trigger an open event if it is complete', function () {
     const action = new Action(identity)
     const spy = jest.fn()
 
diff --git a/test/unit/action/update.test.js b/test/unit/action/update.test.js
index 0a586178..f0382be5 100644
--- a/test/unit/action/update.test.js
+++ b/test/unit/action/update.test.js
@@ -53,7 +53,7 @@ describe('Action update state', function () {
     expect(callback).toHaveBeenCalledWith(3)
   })
 
-  it('does not trigger an update event if it is disposable', function () {
+  it('does not trigger an update event if it is complete', function () {
     const action = new Action(identity)
     const spy = jest.fn()
 
diff --git a/test/unit/get-registration.test.js b/test/unit/get-registration.test.js
index b3cb51d8..c7c3d491 100644
--- a/test/unit/get-registration.test.js
+++ b/test/unit/get-registration.test.js
@@ -1,9 +1,5 @@
 import tag from '../../src/tag'
-import getRegistration from '../../src/get-registration'
-
-import {
-  ACTION_ALIASES
-} from '../../src/constants'
+import getRegistration, {ALIASES} from '../../src/get-registration'
 
 const action = tag(n => n)
 
@@ -29,8 +25,8 @@ describe('getRegistration', function () {
   })
 
   describe('Action aliasing', function () {
-    for (let status in ACTION_ALIASES) {
-      let alias = ACTION_ALIASES[status]
+    for (let status in ALIASES) {
+      let alias = ALIASES[status]
 
       it(`can inspect the ${status} status`, function () {
         let success = n => n
@@ -38,7 +34,7 @@ describe('getRegistration', function () {
           [action]: {
             [status]: success
           }
-        }, action, ACTION_ALIASES[status])
+        }, action, ALIASES[status])
 
         expect(answer).toBe(success)
       })