From ae7d99682b533401da2647baf0845be23bec13b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= <miroslav@strongloop.com>
Date: Wed, 12 Nov 2014 08:59:56 +0100
Subject: [PATCH] Simplify `app.defineMiddlewarePhases`

Refactor the implementation to use the new method `phaseList.zipMerge`.

This is commit is changing the behaviour in the case when
the first new phase does not exist in the current list.

Before the change, all new phases were added just before the "routes"
phase.

After this change, new phases are added to the head of the list,
until an existing phase is encountered, at which point the regular
merge algorithm kicks in.

Example:

    app.defineMiddlewarePhases(['first', 'routes', 'subapps']);

Before the change: code throws an error - 'routes' already exists.

After the change: phases are merged with the following result:

    'first', 'initial', ..., 'routes', 'subapps', ...
---
 lib/server-app.js | 41 ++++-------------------------------------
 test/app.test.js  | 11 ++++++++---
 2 files changed, 12 insertions(+), 40 deletions(-)

diff --git a/lib/server-app.js b/lib/server-app.js
index 71ea7b0c..84148aa2 100644
--- a/lib/server-app.js
+++ b/lib/server-app.js
@@ -84,44 +84,11 @@ proto.middlewareFromConfig = function(factory, config) {
  */
 proto.defineMiddlewarePhases = function(nameOrArray) {
   this.lazyrouter();
-  if (!Array.isArray(nameOrArray)) {
+
+  if (Array.isArray(nameOrArray)) {
+    this._requestHandlingPhases.zipMerge(nameOrArray);
+  } else {
     this._requestHandlingPhases.addBefore('routes', nameOrArray);
-    return;
-  }
-
-  var sourcePhases = nameOrArray;
-  if (!sourcePhases.length) return;
-
-  var targetPhases = this._requestHandlingPhases.getPhaseNames();
-  var targetIx = targetPhases.indexOf(nameOrArray[0]);
-
-  if (targetIx === -1) {
-    // the first new phase does not match any existing one
-    // add all phases before "routes"
-    sourcePhases.forEach(function(it) {
-      this._requestHandlingPhases.addBefore('routes', it);
-    }, this);
-    return;
-  }
-
-  // merge (zip) two arrays of phases
-  for (var sourceIx = 1; sourceIx < sourcePhases.length; sourceIx++) {
-    var nameToAdd = sourcePhases[sourceIx];
-    var previousPhase = sourcePhases[sourceIx - 1];
-    var existingIx = targetPhases.indexOf(nameToAdd, targetIx);
-    if (existingIx === -1) {
-      // A new phase - try to add it after the last one,
-      // unless it was already registered
-      if (targetPhases.indexOf(nameToAdd) !== -1) {
-        throw new Error('Phase ordering conflict: cannot add "' + nameToAdd +
-        '" after "' + previousPhase + '", because the opposite order was ' +
-        ' already specified');
-      }
-      this._requestHandlingPhases.addAfter(previousPhase, nameToAdd);
-    } else {
-      // An existing phase - move the pointer
-      targetIx = existingIx;
-    }
   }
 };
 
diff --git a/test/app.test.js b/test/app.test.js
index 18ab3cbc..20240595 100644
--- a/test/app.test.js
+++ b/test/app.test.js
@@ -157,9 +157,14 @@ describe('app', function() {
       verifyMiddlewarePhases(['custom', 'routes'], done);
     });
 
-    it('adds an array of phases just before "routes"', function(done) {
-      app.defineMiddlewarePhases(['custom1', 'custom2']);
-      verifyMiddlewarePhases(['custom1', 'custom2', 'routes'], done);
+    it('merges phases adding to the start of the list', function(done) {
+      app.defineMiddlewarePhases(['first', 'routes', 'subapps']);
+      verifyMiddlewarePhases([
+        'first',
+        'initial', // this was the original first phase
+        'routes',
+        'subapps'
+      ], done);
     });
 
     it('merges phases preserving the order', function(done) {