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.

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

Remove need for once() #20

Merged
merged 1 commit into from
Feb 4, 2014
Merged

Conversation

asolove
Copy link
Contributor

@asolove asolove commented Sep 25, 2013

tl;dr; autoCancelPrevious should not allow its observer to be replaced after it is cancelled. There is a problem upstream with the propertyObserver that allows this to happen, which I can't diagnose. We may want to hold off this merge and fix there.

I spent some time tracing down sources of multiple calls of functions passed to once(). I discovered that all of the ones caused by the current test suite boil down to replacing parent objects that have property observers on them.

var test = Bindings.defineBindings({
  a: []
}, {
  "b": {"<-": "a.0"}
}
test.a[0] = 2; // no problem
test.a = [] // kaboom!

The property change observer on ".0" has its cancel callback called, and then later in the same call-stack its replace callback is called. Disallowing this in autoCancelPrevious removed all cases of functions passed to once being called more than once.

@@ -1404,21 +1404,8 @@ function autoCancelPrevious(emit) {
cancelPrevious = emit.apply(this, arguments) || Function.noop;
return function cancelObserver() {
cancelPrevious();
cancelPrevious = Function.noop;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only substantive bit.

Copy link
Member

Choose a reason for hiding this comment

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

Ah hah!

@kriskowal
Copy link
Member

The test.a[0] = 2 line isn’t going to trigger a property change dispatch unless it’s changed to test.a.set(0, 2). I’ll check whether this is germane before merging.

kriskowal added a commit to kriskowal/frb that referenced this pull request Jan 31, 2014
kriskowal added a commit that referenced this pull request Feb 4, 2014
After running the specs with memwatch probing for leaks,
I am content that this change is solid.
@kriskowal kriskowal merged commit 2254d9c into montagejs:master Feb 4, 2014
@kriskowal
Copy link
Member

Consider this a send-off, @asolove. I followed up and also removed all use of autoCancelPrevious. I used memwatch and wrote out a scenario to convince myself that the previous observers were canceled without the extra decorator. One side effect is that observe functions do not necessarily return anything, but vigilance in all cases where a cancel function might or might not exist solves the problem. Another side effect is that the equality binder specs had to change, but near as I can tell, the new behavior is even more logical, though for my life I have no idea why it changed.

If the Montage tree controller breaks, I would look there.

@asolove
Copy link
Contributor Author

asolove commented Feb 4, 2014

🍷 What a sendoff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants