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

Added in deep compare option for bind and documented bind feature #144

Closed
wants to merge 6 commits into from
Closed

Conversation

kjlubick
Copy link

@kjlubick kjlubick commented Oct 9, 2014

First off, thanks for making a great localstorage module. I especially liked that it basically worked with everything, arrays and all, out of the box.

I had a complicated array structure that I wanted to store and as per http://stackoverflow.com/questions/14712089/how-to-deep-watch-an-array-in-angularjs
the advice was to use $watch instead of $watchCollection to deal with the deep comparision that I needed.

It's a relatively small change and I hope my documentation on bind is helpful to others as well.

It will be slower, but allows for binding an array objects.
http://stackoverflow.com/a/14713978/1447621
Documented the newly added feature and the other arguments as well.
@a8m
Copy link
Collaborator

a8m commented Oct 9, 2014

@kjlubick , Thanks.
please add test(s).

@kjlubick
Copy link
Author

kjlubick commented Oct 9, 2014

Added a test. Is that sufficient?

@a8m
Copy link
Collaborator

a8m commented Oct 10, 2014

@kjlubick would you please just merge these 4 commits into only one? thanks.

@kjlubick
Copy link
Author

Couldn't do it in one because of the two merge conflicts.

Conflicts:
	README.md
	src/angular-local-storage.js
@kjlubick
Copy link
Author

Ready to merge

@a8m
Copy link
Collaborator

a8m commented Oct 11, 2014

I think we should always use $watch(..., ..., true) instead of $watchCollection.
Because when we bind a non-primitive value we actually want tס do deep comparison.
@grevory What do you think ?

@kjlubick I think it's kinda messy to do something like that:
ls.bind($scope, 'key', null, 'key', true) instead of ls.bind($scope, 'key')
I dunno, 5 arguments it's too much for me.

@kjlubick
Copy link
Author

5 args is a bit much, I agree. I didn't make it default because the angularjs docs warned of a performance hit - but if it's not doing what we want anyway than we don't have too much of a choice.

@a8m
Copy link
Collaborator

a8m commented Oct 11, 2014

@kjlubick , we can go this way:

$scope.$watch(key, function(newVal) {
  //...
}, isObject($scope[key]));

so, if it's primitive value, we comparing for reference equality. so actually it really cheaper.

@grevory
Copy link
Owner

grevory commented Oct 11, 2014

I like it
On Oct 11, 2014 5:00 PM, "Ariel Mashraki" notifications@github.com wrote:

@kjlubick https://github.com/kjlubick , we can go this way:

$scope.$watch(key, function(newVal) {
//...}, isObject($scope[key]));

so, if it's primitive value, we comparing for reference equality. so
actually it really cheaper.


Reply to this email directly or view it on GitHub
#144 (comment)
.

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

Successfully merging this pull request may close these issues.

3 participants