Skip to content

Fix dynamic shared methods can't hide #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
clark0x wants to merge 3 commits into from
Closed

Fix dynamic shared methods can't hide #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b939a05
Fix dynamic shared methods can't hide
clark0x Aug 30, 2014
54a4e09
add tests for SharedClass
clark0x Aug 31, 2014
c586e8b
differentiate two tests
clark0x Aug 31, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 12 additions & 16 deletions lib/shared-class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -82,27 +79,23 @@ 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
this._resolvers.forEach(function(resolver) {
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.
Expand All @@ -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);

@clark0x clark0x Aug 30, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have to use name instead of fn. fn might different from sharedMethod.fn even they have the same name.

e.g., in Model.belongsToRemoting(), this.prototype[relationName] becomes sharedMethod.fn, and '__get__' + relationName becomes sharedMethod.name. This will cause that SharedMethod.prototype.getFunction() will return ctor.prototype['__get__' + relationName], and SharedMethod.prototype.isDelegateFor() will always return false.

@clark0x clark0x Aug 30, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g., in Model.scopeRemoting(), the sharedMethods will be defined WITHOUT fn. So, SharedMethod.prototype.isDelegateFor(fn) will always return false. Unless call with name.

@raymondfeng raymondfeng Aug 30, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the inconsistency is caused by two different versions of remoting definition.

  1. The 1st version is to define all remoting metadata directly on the function itself. It creates a constraint that a remote function has to be referenced by a property on the model class/prototype. In the case of relations, we had to add a dummy method to the model class prototype. That's why you see ctor.prototype['__get__' + relationName]. The other limitation is that the remote operation name has to be same as the method name.
  2. The 2nd version is to define all remoting metadata on the SharedClass/SharedMethod so that the target function can be decoupled from the model class/prototype. The new approach allows us to connect any functions to a model class/prototype and make them remote. The target function can be invoked with the model class or instance as the receiver (this) without requiring the function to be physically defined on the model class/prototype. For example, ctor.prototype['__get__' + relationName] should be removed.
  3. To keep the backward compatibility, both styles are supported. That creates some confusions too.
  4. I think SharedMethod.prototype.getFunction() should return this.fn if it is present before looking up from the model class/prototype by name.
  5. loopback.Model now uses 2nd version of remoting definition. For belongsToRemoting, it connects the ctor.prototype[relationName] to '__get__' + relationName. For scopeRemoting, it only provides the name and expects the function will be resolved from the model class prototype by name. I think we can fix that to make the resolved scope functions explicit from the prototype.

I think your patch mostly looks good. Can you add some tests?

if (!sharedMethod) {
sharedMethod = new SharedMethod(fn, name, this, options);
methods.push(sharedMethod);
}
return sharedMethod;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/shared-method.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
111 changes: 94 additions & 17 deletions test/shared-class.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {};
Expand Down Expand Up @@ -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);
Expand All @@ -92,16 +92,68 @@ 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() {
it('defines a remote method', 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);
});
Expand All @@ -112,7 +164,7 @@ describe('SharedClass', function() {
process.nextTick(function() {
MyClass[METHOD_NAME] = function(str, cb) {
cb(null, str);
}
};
done();
});

Expand All @@ -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 () {
Expand All @@ -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 instance 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);
it('find static sharedMethod by name', function () {
assert(sc.find('myMethod', true) === sm2);
});
});

Expand Down