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

use deep quality comparison for derived updates #141

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cscheye
Copy link

@cscheye cscheye commented Feb 11, 2015

Instead of using strict comparison on derived property changes, uses the _.isEqual method. Helps prevent triggering unnecessary change events when a derived property is not a primitive data type.

@kamilogorek
Copy link
Contributor

@CristiScheye thanks for the feedback! I believe this should utilize isObjectEqual amp module – http://amp.ampersandjs.com/#amp-is-object-equal

This will reduce whole code to:

var isObjectEqual = require('amp-is-object-equal');
if (!isObjectEqual(self._cache[name], newVal) || !def.cache) { ... }

@cscheye
Copy link
Author

cscheye commented Mar 24, 2015

@kamilogorek thanks for the pointer, and sorry for the delayed response!

@kamilogorek
Copy link
Contributor

Looks good to me @CristiScheye. Could you please rebase, so it can get merged?

@cscheye cscheye force-pushed the CristiScheye-derived-updates branch from e44a79b to eb47ccb Compare March 29, 2015 19:04
@cscheye
Copy link
Author

cscheye commented Mar 29, 2015

👍

@wraithgar
Copy link
Contributor

Looks good, thanks!

@latentflip
Copy link
Contributor

My only concern about this is a performance hit on derived property changes. Thoughts @HenrikJoreteg ?

@HenrikJoreteg
Copy link
Member

Hard to say what type of performance hit it would actually have, but shouldn't be significant for simple values, it should only be a hit if you're comparing more complex objects, which is probably still better than falsely triggering changes that turn into DOM updates.

This seems like a reasonable change to me. +1

@latentflip
Copy link
Contributor

Oh, and just to be annoying, now that we're not using amp anymore, amp-is-object-equal will need to be lodash.isequal :/

@HenrikJoreteg
Copy link
Member

Easy fix, though. But yeah, we should swap that out.

@flipside
Copy link

Saw this so I decided to update #116. it allows a type to be specified for derived properties and if it exists to use the dataType compare method which defaults to lodash.isequal via _getCompareForType.

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.

7 participants