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

Why listen to change event on collection? #96

Open
mir3z opened this issue May 12, 2016 · 6 comments
Open

Why listen to change event on collection? #96

mir3z opened this issue May 12, 2016 · 6 comments
Labels

Comments

@mir3z
Copy link
Contributor

mir3z commented May 12, 2016

What's the reason behind listening to change event on collections? This causes a lot of events which leads to re-rendering components. Wouldn't be better to listen to add and remove events?

@kevincolten
Copy link

kevincolten commented Jun 10, 2016

Or even the update event, which only fires once for any group of add/remove events

@magalhas
Copy link
Owner

@mir3z I've been off Backbone development for quite a while, but I think the change event is triggered when a model attribute change, it's not related to the addition and removal of models from a collection. Or am I wrong?

Actually we're already listening to the update event (which listens to add and remove event) here

@mir3z
Copy link
Contributor Author

mir3z commented Jun 13, 2016

I understand your point but let's consider the following use case.

Let's have a collection of of TODOs. Then we create a React component which represents a list of TODOs. This component uses a collection as a data source. The list consists of specific TODO items. Each item is another React component and has TODO model as a data source.

Now let's assume a model changes. It triggers change event. React item component re-renders to reflect model state. Then change event is propagates to collection. But list component listens to change event of collection as well so React tries to re-render whole list (at least at the virtual dom level).

My point is that backbone-react-component should inform React only about adding/removing items in collection since in most cases collection is rendered as a some kind of list component. And any change in model should not trigger re-rendering of whole list as it is now. Now you can achieve this only by implementing shouldComponentUpdate.

Your approach is fine as long you work with small collection collections and models. But it does not scale well if collection has 1000 or more heavy models.

Of course implementing #79 may be a cure for my concerns.

@magalhas
Copy link
Owner

I totally agree with you and that's also the best way of binding your data sources. Feel free to PR this as well, I'll release a new minor afterwards. I think tests will need some changes to accommodate this.

#79 might take some time or may never be coded, at least by me :)

@mir3z
Copy link
Contributor Author

mir3z commented Jun 13, 2016

Unfortunately I don't have enough time to implement this change. It seems to be rather a massive change.

@magalhas
Copy link
Owner

I don't think this is that massive change but I'll try to tackle this when I find sometime. Though I'm not sure if I'll have anytime soon

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

No branches or pull requests

3 participants