search

facebookarchive / redux-react-hook

React Hook for accessing state and dispatch from a Redux store
MIT License
2.16k stars 103 forks source link

Simple usemappedstate #40

Closed Turanchoks closed 5 years ago

Turanchoks commented 5 years ago

This PR is based on https://github.com/facebookincubator/redux-react-hook/pull/38 (let's merge it!).

This is more of a suggestion rather than a final pull request. The idea is to return not the state value from the hook but an actual mappedState from the global state so it's not stale even while the component is rebasing its state. Right now I don't see why calling mapState directly on store.getState would be a problem but I'm not 100% sure. We can also avoid extra renders to if the mapState function is changed but the actual mappedState remains the same and synchronous state updates are handled correctly. A problem I see is that calling a mapState on every render might be a problem if the mapState is expensive and not memoized so I decided to add a development-only warning that the function should be memoized.

I've also added a test for a state/props mismatch.

If the approach is fine a can try some benchmarking mentioned in https://github.com/facebookincubator/redux-react-hook/issues/39.

ianobermiller commented 5 years ago

Thanks for the PR and opening the discussion! I'm really hesitant on this one. I know in our app, the mapState function being called so much contributes significantly to the time taken to process an action (we can have > 2000 connected components). If you have a fast changing prop or state, for example, you'd then need to isolate it to a different component to avoid performance issues. This may not be the worst thing, of course. This is very simple, and really isn't that much different than the proposal in #49.

Turanchoks commented 5 years ago

Yes, this is basically what was discussed in #49 but extracted to the library level so we can try to optimise (or recommend to memoize).

I'm not sure I understand what you mean by "isolate it to a different component". You would do this if you want to utilise React.memo and skip children rendering. It might be helpful for the current implementation because a new mappedState causes a component to render at least twice even if the mappedState is unchanged. If the component is actually big it can become a problem and you might want to split it into multiple components and use React.memo. But returning fresh mappedState right away will not cause a second render (there is not setState in hook body) so I don't see what to extract. Moreover, unchanged state won't trigger a render.

To avoid calling mappedState too often I've also added a useMemo to calculate derivedState. I think It is even slightly better than it is now as mappedState will only be invoked if the state actually changes rather than on every action which does not necessary result in a new state.

ianobermiller commented 5 years ago

To avoid calling mappedState too often I've also added a useMemo to calculate derivedState

That is a pretty nice idea. I'm liking this overall. Need to dive in a bit more to the code first, as the forced update still feels a little dirty.

ianobermiller commented 5 years ago

I appreciate not calling mapState too often, but because there is a separate mapState call in checkForUpdates, every time the store changes mapState will be called twice. Can you take a shot at that one?

Turanchoks commented 5 years ago

Here is my suggestion. https://github.com/facebookincubator/redux-react-hook/pull/40/commits/e5518b278ae83d6edfd269239e59cf06f9a08ff6. We can memoize mapState (this can be omitted by passing false as a second arg) in the hook so mapState won't be invoked in useEffect if the state hasn't changed since render. Same goes for mapState and store changes that trigger useEffect. I've updated tests to reflected the behaviour and added a new one for the second arg in useMappedState

Turanchoks commented 5 years ago

@ianobermiller What's your opinion on this?

Turanchoks commented 5 years ago

I think I fixed the things you asked. I've been thinking of adding a comment on why we don't rely on React internal bailout mechanism but use a ref for it but I'm not sure it's worth it. What do you think?

ianobermiller commented 5 years ago

Yes, it is always worth adding more comments. Especially since we have a commit not to long ago using the bailout.

On Sat, Mar 30, 2019 at 4:08 AM Turanchoks notifications@github.com wrote:

I think I fixed the things you asked. I've been thinking of adding a comment on why we don't rely on React internal bailout mechanism but use a ref for it but I'm not sure it's worth it. What do you think?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/facebookincubator/redux-react-hook/pull/40#issuecomment-478235685, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2zi2vVzYmd1CumWYx2ramZCMZPA4ibks5vb0W6gaJpZM4bhmqx .

Turanchoks commented 5 years ago

Is there anything left before merging?