Components: refactor SlotFill to pass exhaustive-deps#44403
Merged
Conversation
|
Size Change: -3 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
7ae82de to
0d7c7f6
Compare
44 tasks
ciampo
reviewed
Oct 13, 2022
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Given the complexity of this component, it would be great if @youknowriad could take a look and advise on the best way to fix the exhaustive deps linter warning 🙏
|
|
||
| useLayoutEffect( () => { | ||
| registry.registerSlot( name, ref, fillProps ); | ||
| registerSlot( name, ref, fillPropsRef.current ); |
Contributor
There was a problem hiding this comment.
If the original intention was to "only consider the initial value of fillProps", then using a ref may cause that behaviour to change
Contributor
Author
There was a problem hiding this comment.
Can you explain that a bit more for me? My hope was that if we store the initial value in the ref, and then don't update that ref at all, then the initial value should be preserved, regardless of what happens to fillProps itself later on. Am I not thinking about the ref behavior correctly?
Contributor
There was a problem hiding this comment.
Excuses for not explaining myself correctly before. What you say is true, but your code changes may introduce a difference in behavior.
Imagine this scenario:
- component renders for the first time,
useLayoutEffectruns and callsregisterSlotwith the initial value offillProps - later, the component re-renders and one of the dependencies of this
useLayoutEffecthas changed — this will cause theuseLayoutEffectto run again and callregisterSlotagain.- with the code written as it is on
trunk,registerSlotwill use the latest value offillProps, which may be different from the initial value - with the changes from this PR,
registerSlotwill keep using the initial value offillProps. This means that if in the meantime the value offillPropschanged, it would be ignored.
- with the code written as it is on
I'm not exactly sure which one was the intended behaviour here, but I wanted to flag this potential change. @youknowriad , could you advise on which of these two scenarios sounds the most correct to you, please? Thank you!
Contributor
There was a problem hiding this comment.
Indeed that's problematic behavior, I missed it. The fillProps used should always be the last ones, not the initial ones but there's no need to re-register if they change, because there's a hook right after that update them when they change.
Contributor
There was a problem hiding this comment.
Thank you, Riad!
@chad1008 , probably the best thing to do here is also to revert the changes, and simply add an eslint-disable comment.
Contributor
Author
There was a problem hiding this comment.
Ah, thank you for clarifying @ciampo, and for the second look @youknowriad!
I think it was the and we are only considering the initial value of fillProps part of the existing comment that through me off, but looking more closely after the above explanation I see exactly what you mean. I'm going to remove that 'initial value' bit from the comment, add an ignore, and link back here.
For any future refactors that are led here by that code comment: unless a more comprehensive refactor is needed, the Latest Ref pattern might be of use here. We could store fillProps in a ref for use in the useLayoutEffect, and keep it up to date as it changes... but the ref wouldn't be a dependency so we wouldn't need to add it to the array. This way when the effect does fire, the ref will contain the latest value, but changes to that value won't trigger anything themselves.
youknowriad
approved these changes
Oct 14, 2022
Contributor
youknowriad
left a comment
There was a problem hiding this comment.
The changes here look good to me.
cbf0f70 to
e378d2e
Compare
ciampo
reviewed
Oct 18, 2022
ciampo
reviewed
Oct 18, 2022
… fully refactored
c4aed99 to
fbb48f2
Compare
ciampo
approved these changes
Oct 20, 2022
ciampo
reviewed
Oct 20, 2022
| - `withFocusReturn`: Refactor tests to `@testing-library/react` ([#45012](https://github.com/WordPress/gutenberg/pull/45012)). | ||
| - `ToolsPanel`: updated to satisfy `react/exhaustive-deps` eslint rule ([#45028](https://github.com/WordPress/gutenberg/pull/45028)) | ||
| - `Tooltip`: updated to ignore `react/exhaustive-deps` eslint rule ([#45043](https://github.com/WordPress/gutenberg/pull/45043)) | ||
| - `SlotFill`: updated to satisfy `react/exhaustive-deps` eslint rule ([#44403](https://github.com/WordPress/gutenberg/pull/44403)) |
Contributor
There was a problem hiding this comment.
This CHANGELOG entry got added under the wrong version, maybe we can move it as part of another exhaustive-deps related PR ?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Updates the
SlotFillcomponent to satisfy theexhaustive-depseslint ruleWhy?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
.../slot-fill/bubbles-virtually/fill.js:
Due to issues with how
thisapplies to reading methods from an object, the lint rule wants to see the fullslotobject as a dependency and not the methods themselves. To resolve this, we can destructure the methods beforehand, so our dependencies reflect the methods themselves..../slot-fill/bubbles-virtually/slot.js:
registerSlotandunregisterSlothave been destructured for the same reasons as the dependencies above.It was noted in the comments that
fillPropswere intentionally left out of the array to avoid un-and-reregistering the fill on every single prop change. We really only want the initial value. That makes sense, but causes our favorite lint rule a lot of anxiety, so I've placed the initialfillPropsvalue into a ref that doesn't get updated, and used that in the layout effect..../src/slot-fill/fill.js & .../slot-fill/use-slot.js
The warnings here have been treated with
eslint ignorecomments for now, as they may require a more in-depth refactor. theuseLayoutEffects in slot-fill/fill.js in particular look like they were written to fire at very specific points.cc'ing @youknowriad in case you're able to assist or offer any advice on updating this component to pass the
exhaustive-depses-lintrule.Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/slot-fill