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

Data: Remove unused forceRender argument #19206

Merged
merged 1 commit into from Dec 17, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Dec 17, 2019

This pull request seeks to remove an unused argument in the implementation of useSelect. When a change is detected in the result of a mapSelect callback, the hook will force a render using its internal forceRender reducer. Since forceRender is implemented using a useReducer which will always yield a new value and never considers the incoming argument, the empty object we currently pass is unused.

const [ , forceRender ] = useReducer( ( s ) => s + 1, 0 );

In theory, the previous implementation could cause memory accumulation and/or extra load on garbage collection due to the initialization and subsequent destruction of the unused object references. The expected impact of this is not significant, but since this is a hot code path, it is worth the extra attention.

I expect this could have been lingering code from an earlier iteration of the hook, considering that this pattern is often used when forcing a render using the setState callback of a useState hook (where setState( {} ) is equally as effective at forcing a render).

Testing Instructions:

Ensure unit tests pass:

npm run test-unit

@aduth aduth added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality [Package] Data /packages/data labels Dec 17, 2019
Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch!

@aduth aduth merged commit b45e53e into master Dec 17, 2019
@aduth aduth deleted the remove/use-select-force-render-arg branch December 17, 2019 22:46
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants