Skip to content

use deep quality comparison for derived updates - #141

Open
cscheye wants to merge 3 commits into
AmpersandJS:masterfrom
cscheye:CristiScheye-derived-updates
Open

use deep quality comparison for derived updates#141
cscheye wants to merge 3 commits into
AmpersandJS:masterfrom
cscheye:CristiScheye-derived-updates

Conversation

@cscheye

@cscheye cscheye commented Feb 11, 2015

Copy link
Copy Markdown

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
Copy Markdown
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

cscheye commented Mar 24, 2015

Copy link
Copy Markdown
Author

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

@kamilogorek

Copy link
Copy Markdown
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

cscheye commented Mar 29, 2015

Copy link
Copy Markdown
Author

👍

@wraithgar

Copy link
Copy Markdown
Contributor

Looks good, thanks!

@latentflip

Copy link
Copy Markdown
Contributor

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

@HenrikJoreteg

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@flipside

Copy link
Copy Markdown

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