yamalight / outstated

Simple hooks-based state management for React
107 stars 7 forks source link

Re-renders everywhere Context api limitation? #11

Closed alexandrius closed 5 years ago

alexandrius commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

That's exactly what unstated-next does :)

alexandrius commented 5 years ago

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 commented 5 years ago

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

yamalight commented 5 years ago

Related answer from Dan: https://github.com/facebook/react/issues/15156#issuecomment-474590693 Option 3 works quite nicely.

alexandrius commented 5 years ago

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

yamalight commented 5 years ago

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 commented 5 years ago

Thanks @yamalight !

marrkeri commented 5 years ago

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 https://github.com/facebook/react/issues/15156#issuecomment-518970311

alexandrius commented 5 years ago

@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 commented 5 years ago

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

marrkeri commented 5 years ago

@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. https://github.com/facebook/react/issues/15156#issuecomment-518970311 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 commented 5 years ago

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

alexandrius commented 5 years ago

@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

yamalight commented 5 years ago

@alexandrius yep, that looks pretty good! 👍

yamalight commented 5 years ago

@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.