Fix useSelector is returned a different result when called with the s… #6450
Conversation
…ame parameters in IdWidget.
✅ Deploy Preview for plone-components canceled.
|
|
@Tishasoumya-02 Keep an eye, during converting from class to function. We should never return the new array or object from useSelector. |
This is not true, in general. It's perfectly ok to return an array or object from useSelector. There are cases however when if the same input provides a different return value, and this value is used as a guard value (in the guard array of a useXxx function) then this might cause an extra re-render, while using useMemo actually does prevent this. |
There was a problem hiding this comment.
To add context, this is needed because we want to shut up this warning:
Selector unknown returned a different result when called with the same parameters. This can lead to unnecessary rerenders.
Selectors that return a new reference (such as an object or an array) should be memoized: https://redux.js.org/usage/deriving-data-selectors#optimizing-selectors-with-memoization {
So it's true that Object.key returns a different array each time it's called with the same value, hence the warning.
However. To be clear. This cannot cause any re-render (even without the useMemo) in this situation. It would only cause re-render if we used this value in a guard expression, which we don't, so this is perfectly safe without the useMemo. So we are trying to fix a no-problem just because React is too dumb and does not know that we are not using this value in a guard expression. So it gives a warning anyway to be on the safe side. To be clear, the suggested fix won't cause any change in the rendering, nor cause nor prevent any additional re-render. (other than a small performance drawback because of the useMemo).
With that the useMemo does indeed shut up the warning, but In fact, an even better solution would be to not use keys on the result, but just return the object itself from useSelector, and check if an index is a key in it. See my comment.
Please change it this way, re-test if it fixes the warning in your use case, and re-submit the PR.
|
|
||
| const indexes = useSelector((state) => keys(state.querystring.indexes)); | ||
| const indexesObj = useSelector((state) => state.querystring.indexes); | ||
| const indexes = useMemo(() => keys(indexesObj), [indexesObj]); |
There was a problem hiding this comment.
remove this line
change line 69 from:
if (indexes.indexOf(values) !== -1) {
to:
if (values in indexesObj) {
This should get rid of the problem.
There was a problem hiding this comment.
... also another remark... it would be better to rename values to value, as it's clearly a single string and not an array of strings, so the plural is not correct name imo.
There was a problem hiding this comment.
done as you said.
Maybe I am wrong. But useSelector may re-render the component if you provide the new object every time.
|
@iFlameing Don't forget to link the PR to the related issue |
* main: (111 commits) Moved `applyBlockDefaults` one component up so the style is computed correctly (#6451) Fix useSelector is returned a different result when called with the s… (#6450) Fix page changes not being announced by assistive technology when navigating using the client-side router (#5288) BlockStyleWrapper aware of block themes (#6445) A11y: Wrap ToC Block within nav tag (#6084) Fix backend error when there is no query supplied (#6423) Added upgrade guide fix for HMR (#6446) Fix CSS lint in Volto Slate (#6444) Release 18.0.0-alpha.47 Release generate-volto 9.0.0-alpha.20 Release @plone/scripts 3.7.0 Release @plone/registry 2.0.0-alpha.0 Improve @plone/registry release-it config `@plone/registry` as ESM module, move to TS, complete documentation (#6399) [Next.js] Better Vercel deployment (#6441) Replace all `yarn` appearences with `pnpm` (#6433) Add deprecation notices to the Upgrade guide (#6426) Clean up #6422 (#6443) Rename page title from Frontend to Volto UI (#6438) URL-Management control panel: Add missing translations (#6436) ...
Fixes #6449