Allow value equality checker to be overridden #21
Conversation
HeroicEric
changed the title
addon/mixin.js
Outdated
| } | ||
| }, | ||
|
|
||
| _compareValues(a, b) { |
There was a problem hiding this comment.
Since you're intending the consumer to override this in some situations, why did you go with the _ naming convention here?
|
This seems like a totally sane use case @HeroicEric , especially because this implementation strategy won't change the behavior for folks if they don't want / care to know about this. If you'd like to rebase this old branch and write some tests, I think it would be a great addition. |
HeroicEric
force-pushed
the
override-value-comparison
branch
3 times, most recently
from
c1e71b8 to
d47b17f
Compare
|
Sorry for letting this linger for so long but I need it again 😄 I updated this with some docs and renamed the function. |
HeroicEric
force-pushed
the
override-value-comparison
branch
from
d47b17f to
77ca797
Compare
|
No problem - this looks great to me and it backwards compatible. cc @lukemelia do you want to have a look and sign off on this? @HeroicEric if I don't hear from Luke in a few days, I can merge. |
|
@blimmer I just remembered why the function was initially underscored. I thought it might be better in terms of avoiding potentially causing issues with the underlying content having a key with the same name. I think it's probably uncommon to have an |
|
That's a good point, though underscored methods usually indicate don't use this to me. You could use something less likely to be overridden like 'bufferedProxyIsEqual' or the like. |
It would be nice if there was a way to override how values are compared inside of
setUknownPropertywithout having to override the whole function.For example, we're working on an app where we're buffering changes made to an array of POJOs and
===doesn't detect equality there like we want it to so we're using_.isEqualfrom lodash.This also makes it easy to override how values for a specific key are compared.
I'm not a fan of the function name I used here but I wanted to see if you're open to this possibility before spending any more time on it.
Another option could be using
Ember.isEqual, since you can defineisEqualfor whatever you want, but this would require converting to anEmber.Object, making it a little less flexible.