Refactor around missing dependency in Link Control internal value sync effect #49509
Conversation
This file contains 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
getdave
added
[Type] Code Quality
|
Size Change: +6 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
One other suggestion to explore (hat tip @kevin940726) is |
Could we try this solution here and see if the test passes? I think we can even simplify it to just: setInternalInputValue( value );Since that the setter of |
scruffian
force-pushed
the
fix/derived-state-in-link-control
branch
from
029caff to
0c5f357
Compare
scruffian
reviewed
Apr 21, 2023
packages/block-editor/src/components/link-control/test/index.js
Outdated
Show resolved
Hide resolved
scruffian
approved these changes
Apr 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
In #49506 we found that the effect at
gutenberg/packages/block-editor/src/components/link-control/use-internal-input-value.js
Lines 11 to 19 in a1f2679
internalInputValueas a dependency.This fixes that by using the lazy update of setState to avoid having to pass the dependency.
Fixes #49506
Why?
Omitting dependencies from an effect is a code smell. However, adding the dependency in this case makes it impossible to update the values of in the component as they will constantly be synced back up with the
value. That's not what we want.Instead we want to make sure that if the LinkControl's
value(only!) changes (or rather the particular sub property that this hook is controlling) then we sync the internal state back up with it.Why do we even need this syncing anyway
The LinkControl is a controlled component in that it's
valuechanges based ononChangehandler. However, the UX is such that you should be able to update the.valueof the<input>'s in a sort of "draft" state and only have them "committed" as the newvaluewhen hitting the "Apply" button (this will triggeronChange).Unfortunately however this means that if:
<input>valueto the component...the internal state needs to reflect the new value passed to the component and not the internal "draft" value.
Basically the
valueshould be the source of truth. If it changes then the component must reset.How?
By using the lazy form of
setStateupdater, we can access the existing internal value and perform the necessary comparison without having to add it as a dependency of the effect. This restores the existing functionality without breaking the rules of hooks.Testing Instructions
keyprop heregutenberg/packages/format-library/src/link/inline.js
Line 262 in 78c4a2a
Testing Instructions for Keyboard
Screenshots or screencast