From a7d33f62e03df75b3b25e5e16692dfca5448aae1 Mon Sep 17 00:00:00 2001 From: scott-wyatt Date: Sun, 1 Jul 2018 14:06:17 -0400 Subject: [PATCH] [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. --- lib/Core.ts | 122 +++++++++++++++++++++++------ lib/Fabrix.ts | 4 +- package-lock.json | 2 +- package.json | 2 +- test/integration/fabrixapp.test.js | 89 ++++++++++++++++++++- test/integration/testspool3.js | 16 +++- test/integration/testspool4.js | 43 ++++++++++ 7 files changed, 247 insertions(+), 31 deletions(-) create mode 100644 test/integration/testspool4.js diff --git a/lib/Core.ts b/lib/Core.ts index feda5d4..ae777c5 100755 --- 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 index ead9d35..4eea0be 100644 --- a/lib/Fabrix.ts +++ b/lib/Fabrix.ts @@ -115,7 +115,7 @@ export class FabrixApp extends EventEmitter { // Merge extensions into app. 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 index 35c0032..414dc19 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "@fabrix/fabrix", - "version": "1.0.0", + "version": "1.0.1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 10bd10e..aca8fd9 100755 --- 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 index 7a43d90..da54de7 100755 --- 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 index 13d3464..4543c7a 100644 --- 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 new file mode 100644 index 0000000..1ce8e96 --- /dev/null +++ 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' + } + } + } + } + }) + } +}