[fix] Fixes API Merging

- Fixes an Issue where the API defined in the app.api wouldn't overwrite a spool.api namespace conflict. - Fixes an Issue where spool.api was not being properly merged within a namespace conflict.
fabrix-app · Jul 1, 2018 · a7d33f6 · a7d33f6
1 parent 524a256
commit a7d33f6
Show file tree

Hide file tree

Showing 7 changed files with 247 additions and 31 deletions.
diff --git a/lib/Core.ts b/lib/Core.ts
@@ -1,4 +1,4 @@
-import { union } from 'lodash'
+import { union, defaultsDeep } from 'lodash'
 import { FabrixApp } from './'
 import * as mkdirp from 'mkdirp'
 import { Templates } from './'
@@ -115,8 +115,8 @@ export const Core = {
   /**
    * Instantiate resource classes and bind resource methods
    */
-  bindResourceMethods(app: FabrixApp, defaults: string[]): void {
-    defaults.forEach(resource => {
+  bindResourceMethods(app: FabrixApp, resources: string[]): void {
+    resources.forEach(resource => {
       try {
         app[resource] = Core.bindMethods(app, resource)
       }
@@ -148,42 +148,112 @@ export const Core = {
     return props
   },
 
+  /**
+   * Get the property names of an Object
+   */
+  getPropertyNames(obj) {
+    return Object.getOwnPropertyNames(obj.prototype)
+  },
+
+  /**
+   * If the object has a prototype property
+   */
+  hasPrototypeProperty(obj, proto) {
+    return obj.prototype.hasOwnProperty(proto)
+  },
+
+  /**
+   * Merge the Prototype of uninitiated classes
+   */
+  mergePrototype(obj, next, proto) {
+    obj.prototype[proto] = next.prototype[proto]
+  },
+
   /**
    * Merge the app api resources with the ones provided by the spools
    * Given that they are allowed by app.config.main.resources
    */
-  mergeApi (app: FabrixApp, spool: Spool) {
-    // Use the setter to see if any new api resources from the spool can be applied
-    app.resources = union(app.resources, Object.keys(app.api), Object.keys(spool.api))
-    // Foreach resource, bind the spool.api into the app.api
-    app.resources.forEach( resource => {
-      // If there is a conflict, resolve it at the resource level
-      if (app.api.hasOwnProperty(resource) && spool.api.hasOwnProperty(resource)) {
-        Core.mergeApiResource(app, spool, resource)
-      }
-      // Else define the resource in the app.api and merge the spool.api resource into it
-      else if (!app.api.hasOwnProperty(resource) && spool.api.hasOwnProperty(resource)) {
-        app.api[resource] = {}
-        Object.assign(
-          (app.api[resource] || {}),
-          (spool.api[resource] || {})
-        )
-      }
+  mergeApi (app: FabrixApp) {
+    const spools = Object.keys(app.spools).reverse()
+    app.resources.forEach(resource => {
+      spools.forEach(s => {
+        // Add the defaults from the spools Apis
+        defaultsDeep(app.api, app.spools[s].api)
+        // Deep merge the Api
+        Core.mergeApiResource(app, app.spools[s], resource)
+      })
     })
   },
 
   /**
-   * Merge the Spool Api Resource if not already defined by App Api
+   * Adds Api resources that were not merged by default
    */
   mergeApiResource (app: FabrixApp, spool: Spool, resource: string) {
-    const spoolApiResources = Object.keys(spool.api[resource])
-    spoolApiResources.forEach(k => {
-      if (!app.api[resource].hasOwnProperty(k)) {
-        app.api[resource][k] = spool.api[resource][k]
-      }
+    if (app.api.hasOwnProperty(resource) && spool.api.hasOwnProperty(resource)) {
+      Object.keys(spool.api[resource]).forEach(method => {
+        Core.mergeApiResourceMethod(app, spool, resource, method)
+      })
+    }
+  },
+
+  /**
+   * Adds Api resource methods that were not merged by default
+   */
+  mergeApiResourceMethod (app: FabrixApp, spool: Spool, resource: string, method: string) {
+    if (spool.api[resource].hasOwnProperty(method) && app.api[resource].hasOwnProperty(method)) {
+      const spoolProto = Core.getPropertyNames(spool.api[resource][method])
+      spoolProto.forEach(proto => {
+        if (!Core.hasPrototypeProperty(app.api[resource][method], proto)) {
+          Core.mergePrototype(app.api[resource][method], spool.api[resource][method], proto)
+          app.log.silly(`${spool.name}.api.${resource}.${method}.${proto} extending app.api.${resource}.${method}.${proto}`)
+        }
+      })
+    }
+  },
+
+  /**
+   * Merge the spool api resources with the ones provided by other spools
+   * Given that they are allowed by app.config.main.resources
+   */
+  mergeSpoolApi (app: FabrixApp, spool: Spool) {
+    app.resources = union(app.resources, Object.keys(app.api), Object.keys(spool.api))
+
+    const spools = Object.keys(app.spools)
+      .filter(s => s !== spool.name)
+
+    app.resources.forEach(resource => {
+      spools.forEach(s => {
+        Core.mergeSpoolApiResource(app, app.spools[s], spool, resource)
+      })
     })
   },
 
+  /**
+   * Merge two Spools Api Resources's in order of their load
+   */
+  mergeSpoolApiResource (app: FabrixApp, spool: Spool, next: Spool, resource: string) {
+    if (spool.api.hasOwnProperty(resource) && next.api.hasOwnProperty(resource)) {
+      Object.keys(next.api[resource]).forEach(method => {
+        Core.mergeSpoolApiResourceMethod(app, spool, next, resource, method)
+      })
+    }
+  },
+
+  /**
+   * Merge two Spools Api Resources Method's in order of their load
+   */
+  mergeSpoolApiResourceMethod (app: FabrixApp, spool: Spool, next: Spool, resource: string, method: string) {
+    if (spool.api[resource].hasOwnProperty(method) && next.api[resource].hasOwnProperty(method)) {
+      const spoolProto = Core.getPropertyNames(spool.api[resource][method])
+      spoolProto.forEach(proto => {
+        if (!Core.hasPrototypeProperty(next.api[resource][method], proto)) {
+          Core.mergePrototype(next.api[resource][method], spool.api[resource][method], proto)
+          app.log.silly(`${spool.name}.api.${resource}.${method}.${proto} extending ${next.name}.api.${resource}.${method}.${proto}`)
+        }
+      })
+    }
+  },
+
   /**
    * Merge extensions provided by spools into the app
    */

diff --git a/lib/Fabrix.ts b/lib/Fabrix.ts
@@ -115,7 +115,7 @@ export class FabrixApp extends EventEmitter {
         // Merge extensions into app.<ext>
         Core.mergeExtensions(this, spool)
         // Merge the spool.api with app.api
-        Core.mergeApi(this, spool)
+        Core.mergeSpoolApi(this, spool)
         // Bind the Spool Listeners to app.emit
         Core.bindSpoolMethodListeners(this, spool)
       }
@@ -125,6 +125,8 @@ export class FabrixApp extends EventEmitter {
       }
     })
 
+    // Merge the API from the spools
+    Core.mergeApi(this)
     // Instantiate resource classes and bind resource methods
     Core.bindResourceMethods(this, this.resources)
     // Bind Application Listeners

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@fabrix/fabrix",
-  "version": "1.0.0",
+  "version": "1.0.1",
   "description": "Strongly Typed Modern Web Application Framework for Node.js",
   "keywords": [
     "framework",

diff --git a/test/integration/fabrixapp.test.js b/test/integration/fabrixapp.test.js
@@ -7,6 +7,7 @@ const Controller = require('../../dist/common').FabrixController
 const Testspool = require('./testspool')
 const Testspool2 = require('./testspool2')
 const Testspool3 = require('./testspool3')
+const Testspool4 = require('./testspool4')
 const testAppDefinition = require('./testapp')
 const lib = require('../../dist/index')
 
@@ -271,7 +272,93 @@ describe('Fabrix', () => {
             }
           }
           const app = new FabrixApp(def)
-          assert.equal(app.controllers.Test2Controller.foo(), 'bar')
+          assert.equal(app.controllers.TestController.foo(), 'bar')
+          assert.equal(app.controllers.Test2Controller.foo(), 'baz')
+        })
+
+        it('(sanity) should combine api resources in the same name space from different spools in order', () => {
+          const def = {
+            pkg: { },
+            api: { },
+            config: {
+              main: {
+                spools: [
+                  Testspool3,
+                  Testspool4
+                ]
+              }
+            }
+          }
+          const app = new FabrixApp(def)
+
+          assert.equal(app.controllers.TestController.foo(), 'barf')
+          assert.equal(app.controllers.Test2Controller.foo(), 'barf')
+          assert.equal(app.controllers.Test3Controller.foo(), 'baz')
+          assert.equal(app.controllers.Test4Controller.foo(), 'barf')
+
+          assert.equal(app.controllers.TestController.kaz(), 'ba')
+          assert.equal(app.controllers.Test2Controller.kaz(), 'baf')
+          assert.equal(app.controllers.Test3Controller.kaz(), 'ba')
+          assert.equal(app.controllers.Test4Controller.kaz(), 'baf')
+        })
+
+        it('(sanity) should combine root api resources in the same name space from different spools in order', () => {
+          const def = {
+            pkg: { },
+            api: {
+              controllers: {
+                TestController: class TestController extends Controller {
+                  foo() {
+                    return 'bar'
+                  }
+                }
+              }
+            },
+            config: {
+              main: {
+                spools: [
+                  Testspool3,
+                  Testspool4
+                ]
+              }
+            }
+          }
+          const app = new FabrixApp(def)
+
+          assert.equal(app.controllers.TestController.foo(), 'bar')
+          assert.equal(app.controllers.Test2Controller.foo(), 'barf')
+          assert.equal(app.controllers.Test3Controller.foo(), 'baz')
+          assert.equal(app.controllers.Test4Controller.foo(), 'barf')
+
+          assert.equal(app.controllers.TestController.kaz(), 'ba')
+          assert.equal(app.controllers.Test2Controller.kaz(), 'baf')
+          assert.equal(app.controllers.Test3Controller.kaz(), 'ba')
+          assert.equal(app.controllers.Test4Controller.kaz(), 'baf')
+        })
+
+        it('(sanity) should ignore spool resource method becacuse it\'s defined in the app', () => {
+          const def = {
+            pkg: { },
+            api: {
+              controllers: {
+                TestController: class TestController extends Controller {
+                  foo() {
+                    return 'bar'
+                  }
+                }
+              }
+            },
+            config: {
+              main: {
+                spools: [
+                  Testspool3,
+                  Testspool4,
+                ]
+              }
+            }
+          }
+          const app = new FabrixApp(def)
+          assert.equal(app.controllers.TestController.foo(), 'bar')
         })
 
         it('should have default resources', () => {

diff --git a/test/integration/testspool3.js b/test/integration/testspool3.js
@@ -19,10 +19,24 @@ module.exports = class Testspool3 extends Spool {
             foo() {
               return 'baz'
             }
+            kaz() {
+              return 'ba'
+            }
           },
           Test2Controller: class Test2Controller extends Controller {
             foo() {
-              return 'bar'
+              return 'baz'
+            }
+            kaz() {
+              return 'ba'
+            }
+          },
+          Test3Controller: class Test3Controller extends Controller {
+            foo() {
+              return 'baz'
+            }
+            kaz() {
+              return 'ba'
             }
           }
         }

diff --git a/test/integration/testspool4.js b/test/integration/testspool4.js
@@ -0,0 +1,43 @@
+const Spool = require('../../dist/common').Spool
+const Controller = require('../../dist/common').FabrixController
+
+module.exports = class Testspool4 extends Spool {
+  constructor (app) {
+    super(app, {
+      pkg: {
+        name: 'spool-test4'
+      },
+      config: {
+        test: {
+          val: 0,
+          otherval: 1
+        }
+      },
+      api: {
+        controllers: {
+          TestController: class TestController extends Controller {
+            foo() {
+              return 'barf'
+            }
+          },
+          Test2Controller: class Test2Controller extends Controller {
+            foo() {
+              return 'barf'
+            }
+            kaz() {
+              return 'baf'
+            }
+          },
+          Test4Controller: class Test4Controller extends Controller {
+            foo() {
+              return 'barf'
+            }
+            kaz() {
+              return 'baf'
+            }
+          }
+        }
+      }
+    })
+  }
+}