Simple usemappedstate

[Turanchoks]

This PR is based on #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 #39.

[ianobermiller]

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]

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]

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]

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]

Here is my suggestion. e5518b2. 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

[ianobermiller]

this is going to have almost no overhead. Any reason not to memoize it unconditionally? I'd like to avoid adding options to useMappedState

[Turanchoks]

Not really. Removed it.

[Turanchoks]

@ianobermiller What's your opinion on this?

[ianobermiller]

Add a comment that this causes the component to re-render using the new derived state (and that memozing mapState means this doesn't run mapState again).

[ianobermiller]

Just a bit of feedback and I think we should totally merge this.

[Turanchoks]

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]

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 ***@***.***> 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 <#40 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2zi2vVzYmd1CumWYx2ramZCMZPA4ibks5vb0W6gaJpZM4bhmqx> .

[Turanchoks]

Is there anything left before merging?