From b939a058c6b3824abe5b4c999957d70b74f288e6 Mon Sep 17 00:00:00 2001 From: Clark Wang Date: Sat, 30 Aug 2014 15:57:43 +0800 Subject: [PATCH 1/3] Fix dynamic shared methods can't hide Signed-off-by: Clark Wang --- lib/shared-class.js | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/lib/shared-class.js b/lib/shared-class.js index 5dfefeac..dc705916 100644 --- a/lib/shared-class.js +++ b/lib/shared-class.js @@ -64,16 +64,13 @@ function SharedClass(name, ctor) { SharedClass.prototype.methods = function () { var ctor = this.ctor; - var methods = []; + var methods = this._methods; var sc = this; - var functionIndex = []; // static methods eachRemoteFunctionInObject(ctor, function (fn, name) { - if(functionIndex.indexOf(fn) === -1) { - functionIndex.push(fn); - } else { - sharedMethod = find(methods, fn); + var sharedMethod = find(methods, fn, true); + if(sharedMethod) { sharedMethod.addAlias(name); return; } @@ -82,14 +79,12 @@ SharedClass.prototype.methods = function () { // instance methods eachRemoteFunctionInObject(ctor.prototype, function (fn, name) { - if(functionIndex.indexOf(fn) === -1) { - functionIndex.push(fn); - } else { - sharedMethod = find(methods, fn); + var sharedMethod = find(methods, fn, false); + if(sharedMethod) { sharedMethod.addAlias(name); return; } - methods.push(SharedMethod.fromFunction(fn, name, sc)); + methods.push(SharedMethod.fromFunction(fn, name, sc, false)); }); // resolvers @@ -97,12 +92,10 @@ SharedClass.prototype.methods = function () { resolver.call(this, define.bind(sc, methods)); }); - methods = methods.concat(this._methods); - return methods.filter(function(sharedMethod) { return sharedMethod.shared === true; }); -} +}; /** * Define a shared method with the given name. @@ -118,8 +111,11 @@ SharedClass.prototype.defineMethod = function(name, options, fn) { function define(methods, name, options, fn) { options = options || {}; - var sharedMethod = new SharedMethod(fn, name, this, options) - methods.push(sharedMethod); + var sharedMethod = find(methods, name, options.isStatic); + if (!sharedMethod) { + sharedMethod = new SharedMethod(fn, name, this, options); + methods.push(sharedMethod); + } return sharedMethod; } From 54a4e09f77bf7a84030e8afb40286d1ef534dec9 Mon Sep 17 00:00:00 2001 From: Clark Wang Date: Sun, 31 Aug 2014 09:14:15 +0800 Subject: [PATCH 2/3] add tests for SharedClass 1. fix a doc typo in shared-method.js 2. fix tests to use isStatic instead of prototype 3. add tests for SharedClass.methods() 4. add tests for SharedClass.defineMethod() 5. add tests for SharedClass.find() Signed-off-by: Clark Wang --- lib/shared-method.js | 2 +- test/shared-class.test.js | 109 ++++++++++++++++++++++++++++++++------ 2 files changed, 94 insertions(+), 17 deletions(-) diff --git a/lib/shared-method.js b/lib/shared-method.js index 45922b07..2bc79772 100644 --- a/lib/shared-method.js +++ b/lib/shared-method.js @@ -54,7 +54,7 @@ var EventEmitter = require('events').EventEmitter * @property {String} description * @property {String} http * @property {String} rest - * @property {String} shared + * @property {Boolean} shared */ function SharedMethod(fn, name, sc, options) { diff --git a/test/shared-class.test.js b/test/shared-class.test.js index e613504a..6e3a8fe6 100644 --- a/test/shared-class.test.js +++ b/test/shared-class.test.js @@ -34,6 +34,11 @@ describe('SharedClass', function() { }); describe('sharedClass.methods()', function() { + function createSharedFn() { + var fn = function() {}; + fn.shared = true; + return fn; + } it('discovers remote methods', function() { var sc = new SharedClass('some', SomeClass); SomeClass.staticMethod = function() {}; @@ -71,11 +76,6 @@ describe('SharedClass', function() { expect(fns).to.contain(MyClass.b); expect(fns).to.contain(MyClass.prototype.a); expect(fns).to.contain(MyClass.prototype.b); - function createSharedFn() { - var fn = function() {}; - fn.shared = true; - return fn; - } }); it('should skip properties that are model classes', function() { var sc = new SharedClass('some', SomeClass); @@ -92,6 +92,60 @@ describe('SharedClass', function() { expect(fns).to.not.contain(SomeClass.staticMethod); expect(fns).to.not.contain(SomeClass.prototype.instanceMethod); }); + it('should share the state of sharedMethod for static method cross calls', function() { + function MyClass() {} + var sc = new SharedClass('SomeClass', MyClass); + MyClass.myMethod = createSharedFn(); + var methods1 = sc.methods(); + expect(methods1).to.have.length(1); + methods1[0].shared = false; + var methods2 = sc.methods(); + expect(methods2).to.have.length(0); + expect(sc._methods).to.have.length(1); + expect(sc._methods[0].shared).to.equal(false); + }); + it('should share the state of sharedMethod for instance method cross calls', function() { + function MyClass() {} + var sc = new SharedClass('SomeClass', MyClass); + MyClass.prototype.myMethod = createSharedFn(); + var methods1 = sc.methods(); + expect(methods1).to.have.length(1); + methods1[0].shared = false; + var methods2 = sc.methods(); + expect(methods2).to.have.length(0); + expect(sc._methods).to.have.length(1); + expect(sc._methods[0].shared).to.equal(false); + }); + it('should share the state of sharedMethod for dynamic resolve instance method cross calls', function() { + function MyClass() {} + var myMethod = function() {}; + var sc = new SharedClass('SomeClass', MyClass); + sc.resolve(function(define) { + define('myMethod', {/* isStatic: false */}, myMethod); + }); + var methods1 = sc.methods(); + expect(methods1).to.have.length(1); + methods1[0].shared = false; + var methods2 = sc.methods(); + expect(methods2).to.have.length(0); + expect(sc._methods).to.have.length(1); + expect(sc._methods[0].shared).to.equal(false); + }); + it('should share the state of sharedMethod for dynamic resolve static method cross calls', function() { + function MyClass() {} + var myMethod = function() {}; + var sc = new SharedClass('SomeClass', MyClass); + sc.resolve(function(define) { + define('myMethod', { isStatic: true }, myMethod); + }); + var methods1 = sc.methods(); + expect(methods1).to.have.length(1); + methods1[0].shared = false; + var methods2 = sc.methods(); + expect(methods2).to.have.length(0); + expect(sc._methods).to.have.length(1); + expect(sc._methods[0].shared).to.equal(false); + }); }); describe('sharedClass.defineMethod(name, options)', function() { @@ -99,9 +153,7 @@ describe('SharedClass', function() { var sc = new SharedClass('SomeClass', SomeClass); SomeClass.prototype.myMethod = function() {}; var METHOD_NAME = 'myMethod'; - sc.defineMethod(METHOD_NAME, { - prototype: true - }); + sc.defineMethod(METHOD_NAME, { isStatic: false }); var methods = sc.methods().map(function(m) {return m.name}); expect(methods).to.contain(METHOD_NAME); }); @@ -112,7 +164,7 @@ describe('SharedClass', function() { process.nextTick(function() { MyClass[METHOD_NAME] = function(str, cb) { cb(null, str); - } + }; done(); }); @@ -123,6 +175,24 @@ describe('SharedClass', function() { expect(methods).to.contain(METHOD_NAME); } ); + it('should ONLY define same remote instance method once', function() { + var sc = new SharedClass('SomeClass', SomeClass); + SomeClass.prototype.myMethod = function() {}; + var METHOD_NAME = 'myMethod'; + var method1 = sc.defineMethod(METHOD_NAME, { isStatic: false }); + var method2 = sc.defineMethod(METHOD_NAME, { isStatic: false }); + assert(method1 === method2); + expect(sc._methods).to.have.length(1); + }); + it('should ONLY define same remote static method once', function() { + var sc = new SharedClass('SomeClass', SomeClass); + SomeClass.myMethod = function() {}; + var METHOD_NAME = 'myMethod'; + var method1 = sc.defineMethod(METHOD_NAME, { isStatic: true }); + var method2 = sc.defineMethod(METHOD_NAME, { isStatic: true }); + assert(method1 === method2); + expect(sc._methods).to.have.length(1); + }); }); describe('sharedClass.resolve(resolver)', function () { @@ -144,20 +214,27 @@ describe('SharedClass', function() { describe('sharedClass.find()', function () { var sc; - var sm; + var sm1; + var sm2; beforeEach(function() { sc = new SharedClass('SomeClass', SomeClass); SomeClass.prototype.myMethod = function() {}; + SomeClass.myMethod = function () {}; var METHOD_NAME = 'myMethod'; - sm = sc.defineMethod(METHOD_NAME, { - prototype: true - }); + sm1 = sc.defineMethod(METHOD_NAME, { isStatic: false }); + sm2 = sc.defineMethod(METHOD_NAME, { isStatic: true }); + }); + it('finds sharedMethod for the given instance function', function () { + assert(sc.find(SomeClass.prototype.myMethod) === sm1); + }); + it('find sharedMethod by name', function () { + assert(sc.find('myMethod') === sm1); }); - it('finds sharedMethod for the given function', function () { - assert(sc.find(SomeClass.prototype.myMethod) === sm); + it('finds sharedMethod for the given static function', function () { + assert(sc.find(SomeClass.myMethod, true) === sm2); }); it('find sharedMethod by name', function () { - assert(sc.find('myMethod') === sm); + assert(sc.find('myMethod', true) === sm2); }); }); From c586e8bda9bc2b7c31507e0de231d213e2586d92 Mon Sep 17 00:00:00 2001 From: Clark Wang Date: Sun, 31 Aug 2014 09:23:19 +0800 Subject: [PATCH 3/3] differentiate two tests Signed-off-by: Clark Wang --- test/shared-class.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/shared-class.test.js b/test/shared-class.test.js index 6e3a8fe6..c615546f 100644 --- a/test/shared-class.test.js +++ b/test/shared-class.test.js @@ -227,13 +227,13 @@ describe('SharedClass', function() { it('finds sharedMethod for the given instance function', function () { assert(sc.find(SomeClass.prototype.myMethod) === sm1); }); - it('find sharedMethod by name', function () { + it('find instance sharedMethod by name', function () { assert(sc.find('myMethod') === sm1); }); it('finds sharedMethod for the given static function', function () { assert(sc.find(SomeClass.myMethod, true) === sm2); }); - it('find sharedMethod by name', function () { + it('find static sharedMethod by name', function () { assert(sc.find('myMethod', true) === sm2); }); });