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

Make add/remove refs based on ref parent states and paths instead of contains? check, keep the valid refs after the to-cursor -> adapt cycle #364

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

Conversation

verma
Copy link

@verma verma commented Jul 10, 2015

This is a proposed change for #360

When a value pointed to by ref changes, update-refs makes the ref go through a (adapt (to-cursor ...)) process. First we keep the "transformed" ref instead of ignoring it.

Second, we make the "whether we have a ref" check based on parent state and path into this state instead of a contains? check (which for collections comes down to identical? check).

Since a newly added ref is never identical to a "transformed" ref, we end up with duplicate refs pointing to the same parent state and path in the component's __om_refs array. Since there are always new refs in the __om_refs array, refs-changed? always tends to return true even when the parent state didn't change, causing shouldComponentUpdate to always return true when observe pattern is used.

After applying this patch, no duplicate refs are added to the __om_refs arary. The refs stop updating when no parent state changes occur, causing the ref-changed? to eventually return false and making shouldComponentUpdate behave in an expected manner when observe is used.

…contains? check, keep the valid refs after the to-cursor -> adapt cycle
@verma
Copy link
Author

verma commented Jul 10, 2015

Here is the sample program which helps demonstrate the problem:

https://github.com/verma/omfix

The timer on top ticks on app-state mutation, the lower timer ticks on local state mutation of a child component. After a while the state mutations to child component start causing the top timer to re-render even though there are no updates to app-state.

HTH

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

Successfully merging this pull request may close these issues.

1 participant