Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

Commit

Permalink
PathObserver.defineProperty(target, name, model, path) -> Observer.de…
Browse files Browse the repository at this point in the history 
…fineProperty(target, name, observable)

R=arv
BUG=

Review URL: https://codereview.appspot.com/41370044
  • Loading branch information
@rafaelw
rafaelw committed Dec 12, 2013
1 parent f4eaba8 commit 1515c4a
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 54 deletions.
57 changes: 30 additions & 27 deletions src/observe.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,10 @@
return this.value_;
},

getValue: function() {
return this.value_;
},

internalCallback_: function(records) {
if (this.state_ != OPENED)
return;
Expand Down Expand Up @@ -435,11 +439,12 @@
this.callback_.apply(this.target_, args);
} catch (ex) {
Observer._errorThrownDuringCallback = true;
console.error('Exception caught during observer callback: ' + (ex.stack || ex));
console.error('Exception caught during observer callback: ' +
(ex.stack || ex));
}
},

reset: function() {
discardChanges: function() {
if (this.state_ != OPENED)
throw Error('Observer is not open');

Expand Down Expand Up @@ -696,6 +701,10 @@
PathObserver.prototype = createObject({
__proto__: Observer.prototype,

getValue: function() {
return this.path_.getValueFrom(this.object_);

This comment has been minimized.

Sign in to view

Copy link
@wereHamster

wereHamster Dec 14, 2013

Shouldn't this check for this.path_ being undefined, like setValue does?

I'm seeing exceptions because somebody is calling getValue on a CompoundObserver (which extends PathObserver). But, a CompoundObserver never has path_ nor object_ properties defined. Those are only initialized in the PathObserver constructor, but CompoundObserver doesn't call it.

This comment has been minimized.

Sign in to view

Copy link
@rafaelw

rafaelw Dec 17, 2013

Author Contributor

Thanks for the report. This is now fixed.
},

connect_: function() {
if (hasObserve)
this.observedSet_ = new ObservedSet(this.boundInternalCallback_);
Expand Down Expand Up @@ -791,7 +800,8 @@
for (var i = 0; i < this.observed_.length; i = i + 2) {
var pathOrObserver = this.observed_[i+1];
var object = this.observed_[i];
var value = object === observerSentinel ? pathOrObserver.reset() :
var value = object === observerSentinel ?
pathOrObserver.discardChanges() :
pathOrObserver.getValueFrom(object, this.observedSet_)

if (sync) {
Expand Down Expand Up @@ -874,9 +884,12 @@
this.callback_.call(this.target_, this.value_, oldValue);
},

reset: function() {
this.value_ = this.getValueFn_(this.observable_.reset());
return this.value_;
getValue: function() {
return this.observable_.getValue();
},

discardChanges: function() {
return this.getValueFn_(this.observable_.discardChanges());
},

deliver: function() {
Expand Down Expand Up @@ -925,40 +938,30 @@
}
}

// TODO(rafaelw): It should be possible for the Object.observe case to have
// every PathObserver used by defineProperty share a single Object.observe
// callback, and thus get() can simply call observer.deliver() and any changes
// to any dependent value will be observed.
PathObserver.defineProperty = function(target, name, object, path) {
// TODO(rafaelw): Validate errors
path = getPath(path);
Observer.defineProperty = function(target, name, observable) {
var notify = notifyFunction(target, name);

var observer = new PathObserver(object, path,
function(newValue, oldValue) {
if (notify)
notify(PROP_UPDATE_TYPE, oldValue);
}
);
observable.open(function(newValue, oldValue) {
if (notify)
notify(PROP_UPDATE_TYPE, oldValue);
});

Object.defineProperty(target, name, {
get: function() {
return path.getValueFrom(object);
return observable.getValue();
},
set: function(newValue) {
path.setValueFrom(object, newValue);
observable.setValue(newValue);
return newValue;
},
configurable: true
});

return {
close: function() {
var oldValue = path.getValueFrom(object);
if (notify)
observer.deliver();
observer.close();
var lastValue = observable.getValue();
observable.close();
Object.defineProperty(target, name, {
value: oldValue,
value: lastValue,
writable: true,
configurable: true
});
Expand Down
51 changes: 24 additions & 27 deletions tests/test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,12 +328,12 @@ suite('ObserverTransform', function() {

obj.foo = 2;

assert.strictEqual(4, observer.reset());
assert.strictEqual(4, observer.discardChanges());
assertNoChanges();

observer.setValue(2);
assert.strictEqual(obj.foo, 1);
assert.strictEqual(2, observer.reset());
assert.strictEqual(2, observer.discardChanges());
assertNoChanges();

observer.close();
Expand Down Expand Up @@ -451,7 +451,7 @@ suite('PathObserver Tests', function() {
assertNoChanges();
});

test('Path reset', function() {
test('Path discardChanges', function() {
var arr = {};

arr.foo = 'bar';
Expand All @@ -462,7 +462,7 @@ suite('PathObserver Tests', function() {
assertPathChanges('baz', 'bar');

arr.foo = 'bat';
observer.reset();
observer.discardChanges();
assertNoChanges();

arr.foo = 'bag';
Expand All @@ -483,7 +483,7 @@ suite('PathObserver Tests', function() {
assertPathChanges('bat', 'bar');

observer.setValue('bot');
observer.reset();
observer.discardChanges();
assertNoChanges();

observer.close();
Expand Down Expand Up @@ -540,10 +540,10 @@ suite('PathObserver Tests', function() {
assert.equal(3, observer.open(callback));

path.setValueFrom(obj, 2);
assert.equal(2, observer.reset());
assert.equal(2, observer.discardChanges());

path.setValueFrom(obj, 3);
assert.equal(3, observer.reset());
assert.equal(3, observer.discardChanges());

assertNoChanges();

Expand Down Expand Up @@ -821,14 +821,14 @@ suite('PathObserver Tests', function() {
var b = {};
var c = {};

root.a.observer = PathObserver.defineProperty(root.a, 'value',
root, 'value');
root.a.observer = Observer.defineProperty(root.a, 'value',
new PathObserver(root, 'value'));

root.a.b.observer = PathObserver.defineProperty(root.a.b, 'value',
root.a, 'value');
root.a.b.observer = Observer.defineProperty(root.a.b, 'value',
new PathObserver(root.a, 'value'));

root.c.observer = PathObserver.defineProperty(root.c, 'value',
root, 'value');
root.c.observer = Observer.defineProperty(root.c, 'value',
new PathObserver(root, 'value'));

root.c.value = 2;
assert.strictEqual(2, root.a.b.value);
Expand All @@ -852,8 +852,9 @@ suite('PathObserver Tests', function() {
Object.observe(target, callback);
}

var observer = PathObserver.defineProperty(target, 'computed',
source, 'foo.bar');
var observer = Observer.defineProperty(target, 'computed',
new PathObserver(source, 'foo.bar'));

assert.isTrue(target.hasOwnProperty('computed'));
assert.strictEqual(1, target.computed);

Expand Down Expand Up @@ -892,12 +893,6 @@ suite('PathObserver Tests', function() {
name: 'computed',
type: Observer.changeRecordTypes.add
},
{
object: target,
name: 'computed',
type: Observer.changeRecordTypes.update,
oldValue: 1
},
{
object: target,
name: 'computed',
Expand All @@ -910,12 +905,14 @@ suite('PathObserver Tests', function() {

test('DefineProperty - empty path', function() {
var target = {}
var observer = PathObserver.defineProperty(target, 'foo', 1, '');
var observer = Observer.defineProperty(target, 'foo',
new PathObserver(1));
assert.isTrue(target.hasOwnProperty('foo'));
assert.strictEqual(1, target.foo);

var obj = {};
var observer2 = PathObserver.defineProperty(target, 'bar', obj, '');
var observer2 = Observer.defineProperty(target, 'bar',
new PathObserver(obj));
assert.isTrue(target.hasOwnProperty('bar'));
assert.strictEqual(obj, target.bar);
});
Expand Down Expand Up @@ -1175,7 +1172,7 @@ suite('ArrayObserver Tests', function() {
assertNoChanges();
});

test('Array reset', function() {
test('Array discardChanges', function() {
var arr = [];

arr.push(1);
Expand All @@ -1190,7 +1187,7 @@ suite('ArrayObserver Tests', function() {
}]);

arr.push(3);
observer.reset();
observer.discardChanges();
assertNoChanges();

arr.pop();
Expand Down Expand Up @@ -1784,7 +1781,7 @@ suite('ObjectObserver Tests', function() {
assertNoChanges();
});

test('Object reset', function() {
test('Object discardChanges', function() {
var obj = {};

obj.foo = 'bar';
Expand All @@ -1804,7 +1801,7 @@ suite('ObjectObserver Tests', function() {
});

obj.blaz = 'bat';
observer.reset();
observer.discardChanges();
assertNoChanges();

obj.bat = 'bag';
Expand Down

0 comments on commit 1515c4a

Please sign in to comment.