Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Editor: Align Hook: Pass empty deps to useSelect #19008

Merged
merged 1 commit into from Dec 9, 2019

Conversation

@aduth
Copy link
Member

aduth commented Dec 9, 2019

Follow-up of #18963

This pull request seeks to revise the use of useSelect in the newly-refactored align hook to pass an (empty) dependencies array as the second argument, used as an indicator that the select callback does not use any scoped dependencies. Without this argument, the mapSelect callback would be unnecessarily called on every render.

This is intended as a minor optimization.

Testing Instructions:

Repeat testing instructions from #18963

@gziolo
gziolo approved these changes Dec 9, 2019
Copy link
Member

gziolo left a comment

Good catch, I think there are more places in the code where 2nd param in useSelect calls isn't provided. Should we update them as well? Do you know if there is an easy way to teach ESLint rule for the React core hooks to work with custom hooks as well? It should help to catch issues like that.

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Dec 9, 2019

I have also forgotten it 🙈

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 9, 2019

Definitely, worth an ESlint rule, I was wrongly assuming it's the default behavior but I do understand why it's not. (values not updated properly as prop changes...)

@youknowriad youknowriad merged commit 8647dbf into master Dec 9, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the add/align-use-select-deps branch Dec 9, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 9, 2019

Glad we're all thinking the same thing 😄

I opened #19007 as another exploration of whether this should be something we do universally:

This follows some discussion at #18960 (comment) and for me is something of a personal study of if and how dependencies should apply to hooks, or if dependencies should always be provided when supported by a hook.

My comment at #14985 (comment) was also related to this, based on the discussion from #18960 (comment), whether react-hooks/exhaustive-deps is meant to do anything on this front (forcing the second parameter).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.