Avoid allocating a redundant function in the Provider
#1857
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b1d058c:
|
|
✔️ Deploy Preview for react-redux-docs ready! 🔨 Explore the source changes: b1d058c 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-redux-docs/deploys/61c453616c5415000a6f3df4 😎 Browse the preview: https://deploy-preview-1857--react-redux-docs.netlify.app |
| @@ -39,7 +39,7 @@ function Provider<A extends Action = AnyAction>({ | |||
| } | |||
| }, [store, serverState]) | |||
|
|
|||
| const previousState = useMemo(() => store.getState(), [store]) | |||
| const previousState = useMemo(store.getState, [store]) | |||
There was a problem hiding this comment.
It seems that it breaks custom stores that rely on this. A test covering this use case was originally introduced here and fixed this issue
Given how rare of a use case this is and the allocation cost that this requires for all users I would say that it would be better to drop support for this use case and require users who need this to bind their stores appropriately. It's an easy thing to do and feels to me like something that can be easily handled in the userland.
OTOH Provider itself is, at least currently, usually not rendered often as it lives "on the top" of the app. So kinda 🤷
There was a problem hiding this comment.
Typically Provider is only ever going to render once in an app anyway. Given that, I'm not sure this change really provides any benefit :)
|
I'm gonna pass on this one. I can see the argument for dropping a meaningless use case, but also I don't think there's enough actual benefit here to justify the change. |
|
Fine by me 👍 |
No description provided.