Remove second setImmediate wrapper around callback during action exec…

…ution, for less artifial yielding for most of the normal cases where callback function executes successfully with no exception. (#537)

packages/fluxible/docs/api/FluxibleContext.md

@@ -24,6 +24,7 @@ Creates a new context instance with the following parameters:
 
  * `options`: An object containing the context settings
  * `options.app`: Provides access to the application level functions and settings
+ * `options.optimizePromiseCallback`: Whether to optimize Promise callback. Defaults to `false`. `FluxibleContext` uses two setImmediate in [utils/callAction](https://github.com/yahoo/fluxible/blob/master/packages/fluxible/utils/callAction.js) when executing every action.  The second `setImmediate` wraps callback execution to make sure exceptions thrown during callback execution are not swallowed by Promise. This optimization eliminates the second `setImmediate` by catching errors caught by Promise and throwing it. This way, successful callback executions won't need this extra yielding because of the `setImmediate`.
 
 ### executeAction(action, payload, [done])
 

packages/fluxible/lib/Fluxible.js

@@ -8,6 +8,7 @@ var debug = require('debug')('Fluxible');
 var isPromise = require('is-promise');
 var FluxibleContext = require('./FluxibleContext');
 var dispatchr = require('dispatchr');
+var promiseCallback = require('../utils/promiseCallback');
 var __DEV__ = process.env.NODE_ENV !== 'production';
 
 /*
@@ -53,13 +54,14 @@ function Fluxible(options) {
 /**
  * Creates an isolated context for a request/session
  * @method createContext
- * @param {Object} [options]
+ * @param {Object} [options] The options object.  Please refer to FluxibleContext's constructor
+ *         doc for supported subfields and detailed description.
  * @returns {FluxibleContext}
  */
 Fluxible.prototype.createContext = function createContext(options) {
     var self = this;
     options = options || {};
-    var context = new FluxibleContext(self);
+    var context = new FluxibleContext(self, options);
 
     // Plug context with app plugins that implement plugContext method
     this._plugins.forEach(function eachPlugin(plugin) {
@@ -168,7 +170,8 @@ Fluxible.prototype.dehydrate = function dehydrate(context) {
  * @method rehydrate
  * @param {Object} obj Raw object of dehydrated state
  * @param {Object} obj.plugins Dehydrated app plugin state
- * @param {Object} obj.context Dehydrated context state
+ * @param {Object} obj.context Dehydrated context state. See FluxibleContext's
+ *                 rehydrate() for subfields in this object.
  * @param {Function} callback
  * @async Rehydration may require more asset loading or async IO calls
  */
@@ -203,22 +206,13 @@ Fluxible.prototype.rehydrate = function rehydrate(obj, callback) {
         });
     });
 
-    var context = self.createContext();
+    var context = self.createContext(obj.context && obj.context.options);
     var rehydratePromise = Promise.all(pluginTasks).then(function rehydratePluginTasks() {
         return context.rehydrate(obj.context || {});
     });
 
     if (callback) {
-        rehydratePromise
-            .then(function (contextValue) {
-                callback(null, contextValue);
-            }, function (err) {
-                callback(err);
-            })['catch'](function (err) {
-                setImmediate(function () {
-                    throw err;
-                });
-            });
+        promiseCallback(rehydratePromise, callback, {optimize: true});
     }
 
     return rehydratePromise;

packages/fluxible/lib/FluxibleContext.js

@@ -16,9 +16,17 @@ require('setimmediate');
  * A request or browser-session context
  * @class FluxibleContext
  * @param {Fluxible} app The Fluxible instance used to create the context
+ * @param {Object} options The options sharable by the context and context plugins
+ * @param {Boolean} options.optimizePromiseCallback Whether to optimize Promise callback.
+ *          Defaults to `false`. `FluxibleContext` uses two setImmediate in utils/callAction
+ *          when executing every action.  The second `setImmediate` wraps callback execution
+ *          to make sure exceptions thrown during callback execution are not swallowed by Promise.
+ *          This optimization eliminates the second `setImmediate` by catching errors caught by
+ *          Promise and throwing it. This way, successful callback executions won't need this extra
+ *          yielding because of the `setImmediate`.
  * @constructor
  */
-function FluxContext(app) {
+function FluxContext(app, options) {
     this._app = app;
 
     // To be created on demand
@@ -34,6 +42,8 @@ function FluxContext(app) {
     this._actionContext = null;
     this._componentContext = null;
     this._storeContext = null;
+
+    this._optimizePromiseCallback = !!(options && options.optimizePromiseCallback);
 }
 
 /**
@@ -185,8 +195,9 @@ FluxContext.prototype._createSubActionContext = function createSubActionContext(
     var displayName = action.displayName || action.name;
     var newActionContext = Object.assign({}, this.getActionContext(), {
         displayName: displayName,
-        stack: (parentActionContext.stack || []).concat([displayName]),
-        rootId: (parentActionContext.rootId) || generateUUID()
+        optimizePromiseCallback: this._optimizePromiseCallback,
+        rootId: (parentActionContext.rootId) || generateUUID(),
+        stack: (parentActionContext.stack || []).concat([displayName])
     });
     newActionContext.executeAction = newActionContext.executeAction.bind(newActionContext);
     return newActionContext;
@@ -311,6 +322,9 @@ FluxContext.prototype.dehydrate = function dehydrate() {
     var self = this;
     var state = {
         dispatcher: (this._dispatcher && this._dispatcher.dehydrate()) || {},
+        options: {
+            optimizePromiseCallback: this._optimizePromiseCallback
+        },
         plugins: {}
     };
 
@@ -328,6 +342,8 @@ FluxContext.prototype.dehydrate = function dehydrate() {
  * Rehydrates the context state
  * @method rehydrate
  * @param {Object} obj Configuration
+ * @param {Object} obj.options Dehydrated context options
+ * @param {Boolean} obj.options.optimizePromiseCallback Default to false.
  * @param {Object} obj.plugins Dehydrated context plugin state
  * @param {Object} obj.dispatcher Dehydrated dispatcher state
  */
@@ -341,6 +357,7 @@ FluxContext.prototype.rehydrate = function rehydrate(obj) {
         }
     }
     obj.plugins = obj.plugins || {};
+    self._optimizePromiseCallback = !!(obj.options && obj.options.optimizePromiseCallback);
     var pluginTasks = self._plugins.filter(function (plugin) {
         return 'function' === typeof plugin.rehydrate
             && obj.plugins[plugin.name];

packages/fluxible/tests/unit/utils/promiseCallback.js

@@ -0,0 +1,173 @@
+/* jshint newcap:false */
+/* global describe, it, beforeEach, before, after */
+
+'use strict';
+
+var expect = require('chai').expect;
+var promiseCallback = require('../../../utils/promiseCallback');
+
+var originalSetImmediate = global.setImmediate;
+var setImmediateFuncNames = [];
+
+function customSetImmediate() {
+    setImmediateFuncNames.push(arguments[0].name);
+    originalSetImmediate.apply(null, arguments);
+}
+
+describe('#promiseCallback', function () {
+    before(function() {
+        global.setImmediate = customSetImmediate;
+    });
+
+    beforeEach(function () {
+        setImmediateFuncNames = [];
+    });
+
+    after(function() {
+        global.setImmediate = originalSetImmediate;
+    });
+
+    describe('Regular execution', function () {
+        it('should call callback when promise resolves', function (done) {
+            var promise = new Promise(function (resolve, reject) {
+                resolve('resolved');
+            });
+            promiseCallback(promise, function callbackFn(err, result) {
+                expect(err).to.equal(null);
+                expect(result).to.equal('resolved');
+                expect(setImmediateFuncNames.length).to.equal(1);
+                expect(setImmediateFuncNames[0]).to.equal('callbackFn');
+                done();
+            });
+        });
+        it('should call callback when promise rejects', function (done) {
+            var promise = new Promise(function (resolve, reject) {
+                reject('rejected');
+            });
+            promiseCallback(promise, function callbackFn(err, result) {
+                expect(err).to.equal('rejected');
+                expect(result).to.equal(undefined);
+                expect(setImmediateFuncNames.length).to.equal(1);
+                expect(setImmediateFuncNames[0]).to.equal('callbackFn');
+                done();
+            });
+        });
+        it('should not throw error from success callback in same cycle', function (done) {
+            var promise = new Promise(function (resolve, reject) {
+                resolve('resolved');
+            });
+            var caughtError = null;
+            try {
+                promiseCallback(promise, function callbackFn(err, result) {
+                    throw new Error('callback error');
+                });
+            } catch (e) {
+                caughtError = e;
+            }
+            originalSetImmediate(function () {
+                expect(setImmediateFuncNames.length).to.equal(1);
+                expect(setImmediateFuncNames[0]).to.equal('callbackFn');
+                expect(caughtError).to.equal(null);
+                done();
+            });
+        });
+        it('should not throw error from failure callback in same cycle', function (done) {
+            var promise = new Promise(function callbackFn(resolve, reject) {
+                reject('rejected');
+            });
+            var caughtError = null;
+            try {
+                promiseCallback(promise, function callbackFn(err, result) {
+                    if (err) {
+                        throw new Error('callback error');
+                    }
+                });
+            } catch (e) {
+                caughtError = e;
+            }
+            originalSetImmediate(function () {
+                expect(setImmediateFuncNames.length).to.equal(1);
+                expect(setImmediateFuncNames[0]).to.equal('callbackFn');
+                expect(caughtError).to.equal(null);
+                done();
+            });
+        });
+    });
+
+    describe('Optimized execution', function () {
+        it('should call callback when promise resolves', function (done) {
+            var promise = new Promise(function (resolve, reject) {
+                resolve('resolved');
+            });
+            promiseCallback(promise, function (err, result) {
+                expect(err).to.equal(null);
+                expect(result).to.equal('resolved');
+                expect(setImmediateFuncNames.length).to.equal(0,
+                    'no setImmediate for successful callback');
+                done();
+            }, {
+                optimize: true
+            });
+        });
+        it('should call callback when promise rejects', function (done) {
+            var promise = new Promise(function (resolve, reject) {
+                reject('rejected');
+            });
+            promiseCallback(promise, function (err, result) {
+                expect(err).to.equal('rejected');
+                expect(result).to.equal(undefined);
+                expect(setImmediateFuncNames.length).to.equal(0,
+                    'no setImmediate for successful callback');
+                done();
+            }, {
+                optimize: true
+            });
+        });
+        it('should not throw error from success callback in same cycle', function (done) {
+            var promise = new Promise(function (resolve, reject) {
+                resolve('resolved');
+            });
+            var caughtError = null;
+            try {
+                promiseCallback(promise, function callbackFn(err, result) {
+                    throw new Error('callback error');
+                }, {
+                    optimize: true
+                });
+            } catch (e) {
+                caughtError = e;
+            }
+            originalSetImmediate(function () {
+                expect(setImmediateFuncNames.length).to.equal(1,
+                    '1 setImmediate for bad callback');
+                expect(setImmediateFuncNames[0]).to.equal('doNotSwallowError');
+                expect(caughtError).to.equal(null);
+                done();
+            });
+        });
+        it('should not throw error from failure callback in same cycle', function (done) {
+            var promise = new Promise(function (resolve, reject) {
+                reject('rejected');
+            });
+            var caughtError = null;
+            try {
+                promiseCallback(promise, function callbackFn(err, result) {
+                    if (err) {
+                        throw new Error('callback error');
+                    }
+                }, {
+                    optimize: true
+                });
+            } catch (e) {
+                caughtError = e;
+            }
+            originalSetImmediate(function () {
+                expect(setImmediateFuncNames.length).to.equal(1,
+                    '1 setImmediate for bad callback');
+                expect(setImmediateFuncNames[0]).to.equal('doNotSwallowError');
+                expect(caughtError).to.equal(null);
+                done();
+            });
+        });
+    });
+});

packages/fluxible/utils/callAction.js

@@ -5,6 +5,7 @@
 /* global Promise */
 'use strict';
 var isPromise = require('is-promise');
+var promiseCallback = require('./promiseCallback');
 require('setimmediate');
 
 /**
@@ -36,14 +37,7 @@ function callAction (actionContext, action, payload, done) {
     });
 
     if (done) {
-        executeActionPromise
-            .then(function(result) {
-                // Ensures that errors in callback are not swallowed by promise
-                setImmediate(done, null, result);
-            }, function (err) {
-                // Ensures that errors in callback are not swallowed by promise
-                setImmediate(done, err);
-            });
+        promiseCallback(executeActionPromise, done, {optimize: actionContext.optimizePromiseCallback});
     }
 
     return executeActionPromise;

packages/fluxible/utils/promiseCallback.js

@@ -0,0 +1,41 @@
+'use strict';
+
+require('setimmediate');
+
+/**
+ * Execute a callback function when promise resolves or rejects
+ * @method promiseCallback
+ * @param {Promise} promise The promise
+ * @param {Function} callbackFn The callback function
+ * @param {Object} options The options object
+ * @param {Boolean} options.optimize Whether to optimize
+ * @return {void}
+ */
+function promiseCallback (promise, callbackFn, options) {
+    if (!promise || typeof callbackFn !== 'function') {
+        return;
+    }
+
+    if (options && options.optimize) {
+        promise.then(function (result) {
+            callbackFn(null, result);
+        }, callbackFn)
+        ['catch'](function (err) {
+            // Ensures that thrown errors in the `callbackFn()` callback above are
+            // not swallowed by promise
+            setImmediate(function doNotSwallowError() {
+                throw err;
+            });
+        });
+    } else {
+        promise.then(function(result) {
+            // Ensures that errors in callback are not swallowed by promise
+            setImmediate(callbackFn, null, result);
+        }, function (err) {
+            // Ensures that errors in callback are not swallowed by promise
+            setImmediate(callbackFn, err);
+        });
+    }
+};
+
+module.exports = promiseCallback;