Skip to content

Commit

Permalink
Bug fix for issue where multiple observers on same object can trigger
Browse files Browse the repository at this point in the history
before mutation has been applied.
  • Loading branch information
ElliotNB committed Aug 22, 2018
1 parent b6bcf6a commit eb008c3
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 12 deletions.
18 changes: 8 additions & 10 deletions observable-slim.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,9 @@ var ObservableSlim = (function() {

if (originalChange === true) {

// perform the delete that we've trapped if changes are not paused for this observable
if (!observable.changesPaused) delete target[property];

for (var a = 0, l = targets.length; a < l; a++) if (target === targets[a]) break;

// loop over each proxy and see if the target for this change has any other proxies
Expand All @@ -238,10 +241,6 @@ var ObservableSlim = (function() {
}
}

// perform the delete that we've trapped if changes are not paused for this observable
if (!observable.changesPaused)
delete target[property];

}

_notifyObservers(changes.length);
Expand Down Expand Up @@ -307,6 +306,11 @@ var ObservableSlim = (function() {
// observers can be modified of the change that has occurred.
if (originalChange === true) {

// because the value actually differs than the previous value
// we need to store the new value on the original target object,
// but only as long as changes have not been paused
if (!observable.changesPaused) target[property] = value;

for (var a = 0, l = targets.length; a < l; a++) if (target === targets[a]) break;

foundObservable = (a < l);
Expand Down Expand Up @@ -397,12 +401,6 @@ var ObservableSlim = (function() {
},10000);
}

// because the value actually differs than the previous value
// we need to store the new value on the original target object,
// but only as long as changes have not been paused
if (!observable.changesPaused)
target[property] = value;

// TO DO: the next block of code resolves test case #24, but it results in poor IE11 performance. Find a solution.
//
// if the value we've just set is an object, then we'll need to iterate over it in order to initialize the
Expand Down
2 changes: 1 addition & 1 deletion observable-slim.min.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 15 additions & 1 deletion test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -620,10 +620,24 @@ function suite(proxy) {

});

it('30. Verify that a mutation on an object observed by two handlers returns the correct new value in both handlers.', () => {

var data = {"foo":"bar"};
var p = ObservableSlim.create(data, false, function(changes) {
expect(p.foo).to.equal("test");
});

var pp = ObservableSlim.create(p, false, function(changes) {
expect(p.foo).to.equal("test");
});

p.foo = "test";
});


// This test currently does not have an expectation or assertion. For now it simply
// ensures that the garbage clean-up code runs and that the garbage clean-up doesn't throw an error.
it('30. Clean-up observers of overwritten (orphaned) objects.', (done) => {
it('31. Clean-up observers of overwritten (orphaned) objects.', (done) => {

var data = {"testing":{"test":{"testb":"hello world"},"testc":"hello again"},"blah":{"tree":"world"}};
var p = ObservableSlim.create(data, false, function(changes) {
Expand Down

0 comments on commit eb008c3

Please sign in to comment.