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

Re-renders everywhere Context api limitation? #11

Closed
alexandrius opened this issue Aug 5, 2019 · 19 comments
Closed

Re-renders everywhere Context api limitation? #11

alexandrius opened this issue Aug 5, 2019 · 19 comments
Labels
question Further information is requested

Comments

@alexandrius
Copy link
Contributor

Hello.
Although I'm big fan of this library. I found one issue which causes lots of unnecessary re-renders. Basically if any store changes i triggers re-renders to components which rely on different store.
Another problem is that I can't use React.memo if I'm using store inside that component or parent.

Could you help me?

@yamalight yamalight added the question Further information is requested label Aug 5, 2019
@yamalight
Copy link
Owner

I'm not sure I understand the "different store" bit - could you please provide a simple test case? (e.g. on codesandbox)
Another question is - do re-renders actually cause (performance) issues for you?

@alexandrius
Copy link
Contributor Author

alexandrius commented Aug 6, 2019

First issue:
Let's say we have 2 stores:
1 - ItemsStore = Items fetched from the server
2 - FiltersStore = Dynamic Filters list fetched from the server

Filters generate layout with checkboxes, radios, etc. Any change to filters (ex. make checkbox selected) re-renders whole view tree which is correct behavior but there's no way to Memoize layout my current workaround to use useMemo instead of React.memo.

Here's the second issue (Basically the same as previous)
https://snack.expo.io/@alexandrius/4b53ea
Please open console on the bottom.
Click Increment
Both PositiveComponent and NegativeComponent will be re-rendered. Only Positive should have re-rendered.

To address performance Basically it will cause issue when application I'm working on is bigger. Every change in any store will make every component depending on outstated store re-render.
I would not have a need to Memoize my checkboxes but since change in a different store makes checkboxes re-render it caused performance issue.

@yamalight
Copy link
Owner

Ah, got it. This is indeed expected behavior and beyond using React.memo and useMemo to limit re-renders - there's nothing you could really do (or maybe I just couldn't think of anything? if you have any ideas - would be more than happy to discuss).
I've built outstated specifically with small apps in mind where you don't need to care that much about re-renders.
If you see that re-renders do indeed impact your performance, I'd suggest looking into more complex alternatives (e.g. unstated-next)

@alexandrius
Copy link
Contributor Author

What comes from top of my mind is instead of using single Provider what can be done is dynamic creation of Providers for each store. ATM I don't have much time but I will look into this on the weekend. This will solve unnecessary re-renders for component which depend on different store

@yamalight
Copy link
Owner

That's exactly what unstated-next does :)

@alexandrius
Copy link
Contributor Author

alexandrius commented Aug 6, 2019

Unstated doesn't do it automatically. You have to manually wrap Root with Providers you need. I much prefer outstated api so I will try to implement this weekend. I can also make a pull request if you might be interested in something like that.

@yamalight
Copy link
Owner

That does sound interesting and if you can make it work - I'd be more than happy to accept a PR!

@yamalight
Copy link
Owner

Related answer from Dan: facebook/react#15156 (comment)
Option 3 works quite nicely.

@alexandrius
Copy link
Contributor Author

@yamalight I use option 3 ATM for dynamically generated checkboxes but I don't want to wrap everything inside useMemo.

@yamalight
Copy link
Owner

Well, as I've mentioned before - if you can figure out how to auto-split stores into contexts, I'd be more than happy to accept a PR with it :)

@alexandrius
Copy link
Contributor Author

Thanks @yamalight !

@marrkeri
Copy link

marrkeri commented Aug 7, 2019

Related answer from Dan: facebook/react#15156 (comment)
Option 3 works quite nicely.

I have tried Option 2 and Option 3 but they are not working correctly - check my test here facebook/react#15156 (comment)

@alexandrius
Copy link
Contributor Author

@marrkeri I looked at your test but you aren't using option 3. Try using useMemo instead of React.memo the component will be recreated but UI will not.

@yamalight
Copy link
Owner

@marrkeri here's a little demo: https://codesandbox.io/s/outstated-rerenders-test-34p82
Seems to be working as expected to me

@marrkeri
Copy link

marrkeri commented Aug 7, 2019

@marrkeri here's a little demo: https://codesandbox.io/s/outstated-rerenders-test-34p82
Seems to be working as expected to me

@yamalight if you take a look on picture from my post you will see in console that it is correct, but dev tools draws re-render for both inputs. I have posted there also other solution without context and that example draws correct re-render in dev tools. facebook/react#15156 (comment)
Same as your count it looks like memo does his job but when you inspect it, you will see with dev tools different behavior. I need to render 1000 textboxes in table without using virtual list and now it is lagging so much with using context.

@yamalight
Copy link
Owner

@marrkeri fair enough. seems like there is something weird going on indeed 🤔

@alexandrius
Copy link
Contributor Author

alexandrius commented Aug 7, 2019

@yamalight OK I drafted first implementation. Can you look into the code if I'm going in the direction you may allow on this repo?

The new state code is in state.js

https://snack.expo.io/@alexandrius/outstated-issue

  • This will be fully backwards compatible. The only issue I see is too many nested Providers but OTOH cpu overhead will be less

@yamalight
Copy link
Owner

@alexandrius yep, that looks pretty good! 👍

@yamalight
Copy link
Owner

@alexandrius v3.0 is released with your PR included. thanks for your work! ❤️

I'll close this issue, feel free to re-open or create a new one if there's something else.

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

No branches or pull requests

3 participants