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

Avoid om.core.join using IEquiv to compare cursors in shouldComponentUpdate #38

Closed
rads opened this issue Jan 2, 2014 · 3 comments
Closed

Comments

@rads
Copy link

rads commented Jan 2, 2014

I was taking a look at the counters example and I noticed the usage of om.core.join. This function seems like trouble to me because it makes the component using it depend on the entire structure of the app state. A more composable way of putting components together would be to make sure that each component only knows about the data it needs to read and potentially update, without regard for the structure of anything else.

I created a fork with an example of one way to get around this problem: https://github.com/rads/om/compare/counters

Instead of using join, I simply attached the cursor for :shared to each counter. According to the current implementation, using assoc on an existing cursor gives you a new cursor. The counters continue to update when they need to, updating when either the :shared key or the individual counter changes.

The only problem with doing this naively is that since we're creating a new cursor every time we build the child component, the shouldComponentUpdate for the child always returns true. We can get rid of this inefficiency by using the IEquiv protocol to compare inside of shouldComponentUpdate, instead of the existing identical? check. This means that even if the map is not the same as the previous one, it will still return false if the values of the child cursors have not changed.

With this change to shouldComponentUpdate, it's also possible to create entirely new cursors on-the-fly to pass in to child components. Even if a new cursor is created every time, it is considered equivalent to the previous one as long as the cursors it contains -- the ones that come from the app state -- have not changed.

The benefit of this is that the structure of your app data does not have to match up with the structure of your components. For example, if your app data looks like {:states {:editing 1}, :todos [{:id 1, ...}, {:id 2, ...}]}, but you want to reuse a component that expects a data format of {:editing [{:id 1, ...}]}, you could pass in something like (om/to-cursor {:editing (filter #(= (:id %) (get-in app [:states :editing])) (:todos app)}) and it would only update when either :editing or :todos has changed on the existing app state. This would make it much easier to share components across projects.

Edit: Fixed typo and error in example at the end.

@swannodette
Copy link
Member

The point about IEquiv over identical? in shouldComponentUpdate is solid and one I've been pondering myself for a while. I'm not sure that something like om.core/join is problematic, in anycase it's orthogonal to benefits delivered by relying on IEquiv. I will think about it some more.

@swannodette
Copy link
Member

shouldComponentUpdate fixed to use not= in master, ed99bf2. If you think about the use case where the application state is laid out more like a database and not like a UI tree I think you'll see that om.core/join or something like it is really not such a bad idea. Thanks for the feedback.

@swannodette
Copy link
Member

Just to keep this up-to-date. I've reverted the IEquiv change in master. Relying on IEquiv is currently problematic since you can nest Om components that use the exact same piece of data. If you call om.core/set-state! further down then we can not determine the invalidated path.

I think I realized this at some point and that's why we weren't using IEquiv.

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

No branches or pull requests

2 participants