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

store.previousAttributes is storing references to nested objects, preventing changes from being detected #262

Closed
OverZealous opened this issue Dec 3, 2014 · 3 comments

Comments

@OverZealous
Copy link

This took some research to track down, but I'm pretty sure the mout deepMixin functionality has a flaw when copying nested objects. It's in this function here:

    function copyProp(val, key) {
        var existing = this[key];
        if (isPlainObject(val) && isPlainObject(existing)) {
            deepMixIn(existing, val);
        } else {
            this[key] = val;
        }
    }

In this function, this is the target object, and val is coming from the source object. The issue is that it only recursively processes nested objects if the key already exists on the target object. Otherwise, it simply stores a reference.

So what happens is the previousAttributes object is actually made of references to nested children. So, if you update a value on that nested child, it won't see it as a change (because the nested object is a literal reference).

I don't know if the right fix is to fix it on mout, which could have adverse affects for anyone using the library, or replace deepMixin with the existing angular.copy function, which already performs the required action without the extra dependency.

Edit: There is one difference between the functions, angular.copy only copies one item onto one destination, while deepMixin accepts multiple objects, if that matters.

@OverZealous
Copy link
Author

While I'm at it, mixIn is the same as angular.extend, with the same arguments.

jmdobry added a commit that referenced this issue Dec 3, 2014
Stable Version 1.5.2.
@jmdobry jmdobry self-assigned this Dec 3, 2014
@jmdobry jmdobry closed this as completed Dec 3, 2014
@jmdobry
Copy link
Member

jmdobry commented Dec 3, 2014

Fixed in 1.5.2

@OverZealous
Copy link
Author

Fantastic, works great, thanks again for the rapid response!

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

No branches or pull requests

2 participants