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

Master #17

Closed
wants to merge 6 commits into from
Closed

Master #17

wants to merge 6 commits into from

Conversation

miyconst
Copy link

Hello!

I have fixed the issue #16.

Please review and accept my changes.

@MaxArt2501
Copy link
Owner

This can't be merged: runGlobalLoop is meant to be called once (then it calls itself), when there are objects to be observed. In order to minimize overhead, the "global loop" that performs the checks on the properties must be only one.
Moreover, starting a new loop every time an handler is attached deliver the current changes synchronously.

I already have the solution for #16, I'll push it later. If you need it ASAP, it's basically two lines later:

doObserve = function(object, handler, acceptList) {
    var data = observed.get(object);

    if (data) {
        performPropertyChecks(data, object); // <- fixes #16
        setHandler(object, data, handler, acceptList);
    } else {

Thank you for your interest, anyway. I'll let you add some thoughts, but if there's nothing else I'll close this PR.

@miyconst
Copy link
Author

Sure, if you have the solution, then close this PR. Just add the unit test for the issue, you can copy it from my branch.

@MaxArt2501
Copy link
Owner

Closing as per 5f2909a.

@MaxArt2501 MaxArt2501 closed this Oct 15, 2015
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