Skip to content
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

Fix shared instance fields #79

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix shared instance fields #79

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f315734
Failing test cases for #76
betalb Nov 16, 2018
6514297
Fix of #76 for boundMethod
betalb Nov 16, 2018
46d7028
Disabled binding of super methods
betalb Nov 21, 2018
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
34 changes: 32 additions & 2 deletions src/__tests__/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import autobind, {boundMethod, boundClass} from '..';

describe('autobind method decorator: @boundMethod', () => {
class A {
constructor() {
this.value = 42;
constructor(value = 42) {
this.value = value;
}

@boundMethod
Expand Down Expand Up @@ -68,6 +68,36 @@ describe('autobind method decorator: @boundMethod', () => {
assert(value === 50);
});

test('should not share instance methods between instances', () => {
const a = new A('foo');
const b = new A('bar');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test seems too much. Why can't we just change a method on a and see if that method is the same on b?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we also need a test:
Once instance method is changed, it shouldn't be bound.

Copy link
Author

@betalb betalb Nov 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

For the first point: don't think that this will work, methods won't be equal, they will be bound instances of the same underlying function. So theoretically we need to check underlying method. Though I think that test case may be made cleaner. And just realised that

const aOriginalGetValue = a.getValue;
a.getValue = function () {
  return 'foo' + '-' + aOriginalGetValue();
};

And

a.getValue = function () {
  return 'fake-foo';
};

Will hit different code paths, will need to cover this too

For the second point: this one is interesting. Are you saing that if we run setter with new function it shouldn't be auto-bound? If yes, this is not how existing code works (before my change), so I've mimicked this behaviour

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the second point: this one is interesting. Are you saing that if we run setter with new function it shouldn't be auto-bound?

What do you think? I feel like autobind is bound on the method not the property. Let's also take performance into consideration.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, we should not continue auto-binding if somebody changes method, or more interesting - plays with prototype chain of parents

But this will be breaking change and should be done as separate PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

But this will be breaking change and should be done as separate PR

Since this change is quite big I'd release a major for this anyways...

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should not continue auto-binding if somebody changes method, or more interesting - plays with prototype chain of parents

Will this improve the performance?

Copy link
Author

Choose a reason for hiding this comment

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

In certain use-cases, most likely, but this needs to be proved


const aOriginalGetValue = a.getValue;
a.getValue = function () {
return 'foo' + '-' + aOriginalGetValue();
};

const bOriginalGetValue = b.getValue;
b.getValue = function () {
return 'bar' + '-' + bOriginalGetValue();
};

const aGetValue = a.getValue;
const bGetValue = b.getValue;

assert.equal(aGetValue(), 'foo-foo');
assert.equal(bGetValue(), 'bar-bar');
});

test('should not share instance fields between instances', () => {
const a = new A();
const b = new A();
a.getValue = {
foo: 'bar11'
};
assert.notStrictEqual(a.getValue, b.getValue);
});

describe('set new value', () => {
class A {
constructor() {
Expand Down
79 changes: 60 additions & 19 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const OBJECT_INSTANCE = {};

/**
* Return a descriptor removing the value and returning a getter
* The getter will return a .bind version of the function
Expand All @@ -15,32 +17,71 @@ export function boundMethod(target, key, descriptor) {
// recursion and an "Out of stack space" error.
let definingProperty = false;

function reDefineProperty(instance, propertyValue, callGetter) {
let boundFn = null;
definingProperty = true;
let underlyingFn = propertyValue;
// Dummy object which is always strictly not equal to any other value
let currentlyBoundFn = OBJECT_INSTANCE;
const getter = function () {
if (currentlyBoundFn !== underlyingFn) {
if (typeof underlyingFn === 'function') {
boundFn = underlyingFn.bind(this);
} else {
boundFn = underlyingFn;
}
currentlyBoundFn = underlyingFn;
}
return boundFn;
};
Object.defineProperty(instance, key, {
configurable: true,
get: getter,
set(value) {
underlyingFn = value;
}
});
definingProperty = false;
if (callGetter) {
return getter.call(instance);
}
}

return {
configurable: true,
get() {
// eslint-disable-next-line no-prototype-builtins
if (definingProperty || this === target.prototype || this.hasOwnProperty(key) ||
typeof fn !== 'function') {
// If getter is called
// * definingProperty: in IE while defining property
// * this === target: on prototype itself Clazz.prototype[key]
if (definingProperty || this === target || typeof fn !== 'function') {
return fn;
}

const boundFn = fn.bind(this);
definingProperty = true;
Object.defineProperty(this, key, {
configurable: true,
get() {
return boundFn;
},
set(value) {
fn = value;
delete this[key];
}
});
definingProperty = false;
return boundFn;
// Object.getPrototypeOf(this) !== target: getter is called on super
// Not sure that this part of code worth it
// it will cover this use case:
// method() {
// var superMethod = super.method;
// return superMethod()
// }
// We can't redefin property here, because it will be re-defined
// on this, shadowing existing property
if (Object.getPrototypeOf(this) !== target) {
return fn.bind(this);
}

return reDefineProperty(this, fn, true);
},
set(value) {
fn = value;
// accessing prototype
if (this === target) {
fn = value;
} else {
// If setter is called on instance we should create instance bound
// property here. We won't get into getter anymore
// setter on super behaves like on instance
reDefineProperty(this, value, false);
}
}
};
}
Expand Down Expand Up @@ -72,7 +113,7 @@ export function boundClass(target) {

// Only methods need binding
if (typeof descriptor.value === 'function') {
Object.defineProperty(target.prototype, key, boundMethod(target, key, descriptor));
Object.defineProperty(target.prototype, key, boundMethod(target.prototype, key, descriptor));
}
});
return target;
Expand Down