Remove value syncing in Link Control #51387
Conversation
getdave
added
[Type] Code Quality
|
@Mamaduka if you look at the scenario in: ...it should outline why this code exists. Let's see if we can work out a way to remove it. For example in rich text we use a gutenberg/packages/format-library/src/link/inline.js Lines 261 to 264 in 8db07c1 |
|
Size Change: +47 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
The key is one of the recommended ways to reset the component state in React. So I don't see anything wrong with using it.
Maybe we just need to better document this method for LinkControl.
cc @kevin940726
getdave
force-pushed
the
remove/value-syncing-in-link-control
branch
from
d6f38be
to
d735be3
Compare
|
@Mamaduka One approach we could consider here is exporting a wrapped version of const LinkControlWithKey = ( props ) => {
return (
<LinkControl { ...props } key={ useLinkInstanceKey( props?.value ) } />
);
};
export default LinkControlWithKey;I tried this and it does seem to work. The hook comes from I'm not 100% sure about the approach so I'd appreciate your 👀 (and also @kevin940726 if he is around?). |
|
My suggestion would suggest not to use an implicit approach. Let the consumer components handle state resetting via We can document the case in the readme file and link the React docs. What do you think? |
That's probably best. I just realised there's a problem with the automatic approach anyway. Ok so we remove it and update docs. I'll write the docs tomorrow UTC. |
|
@Mamaduka I updated the docs. Let's refine them and see if it's good enough to merge now I removed the test coverage. |
|
Thank you, @getdave! The update looks good (for me, personally) but also feels too technical. People who'll understand all the details there probably know about resetting the state via Maybe we should just document how to solve this particular case and skip why it might happen. P.S. I'm terrible at writing docs. Going to ping @ndiego and @ryanwelcher if they have any suggestions. |
There was a problem hiding this comment.
I'm a bit confused as it's been a while since I looked at <LinkControl> so excuse me if I'm horribly wrong 😅 . Don't we already have key defined in
Either way I think this is still a better change. Nice job on updating the doc too! Perhaps we can also link to the official React doc about this for more info?
|
@kevin940726 Thanks for your review and feedback. Much appreciated. That However certain implementations such as the rich text implementation of Link UI does: gutenberg/packages/format-library/src/link/inline.js Lines 262 to 263 in fa4f5a5 This PR is suggesting making such an implementation the "best practice" and not trying to be clever about syncing values. |
|
Potential bug highlighted by the failing e2e tests here. |
scruffian
force-pushed
the
remove/value-syncing-in-link-control
branch
from
eb6c27a
to
80f8342
Compare
|
Flaky tests detected in 1e9288f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6417847914
|
Personally, I like the extra details. While I can comfortably build "stuff" with React, my knowledge of React fundamentals is not comprehensive, which I imagine is the case for many other developers. The added insight was helpful. Unless @ryanwelcher feels differently, I would move forward. |
|
The test failure is genuine. It happens when we update a link value in the button component and then that needs to get synced back to link control. I'm not sure if that means we need this code after all, or if there's another way to achieve that. |
Ok let's look into that...thanks for confirming the issue @scruffian 🙇 |
getdave
dismissed
kevin940726’s stale review
The feature causes bugs so let's re-review when it's ready.
|
I've taken a look at the test failure and it does indeed seem legit. The issue is that as @scruffian described. The buttons block uses gutenberg/packages/block-library/src/button/edit.js Lines 239 to 248 in e57bf0d This causes the value passed as The component is a controlled component so you should be able to modify the provided Another reason for the internal state is to handle things like checkboxes for The only way I can see this working is if we remove the internal state and make the component fully controlled. This would implement IoC but have the consequence of placing the burden of predictable behaviour onto the consumer. For example, they would need to implement all the logic to determine if/when a value is valid and can be submitted. One way around this might be to expose some standardised methods on the component ( The benefits of such a change would be we remove this hybrid modal and get a predictable controlled component. The downsides are breaking backwards compatibility and also placing additional burden on the consumer. Does anyone have any better suggestions? Which route should we take? |
|
Thank you, @getdave! Unfortunately, I don't have any bright ideas at the moment. The explorations here should show us the pain points of the component better, which would be helpful in the future. Let's keep this PR or issue open for now, and I'll try to experiment with "value syncing" removal more when I get a chance. P.S. I think prepending the HTTP logic could be absorbed by the |
Yes it seems like this is key functionality which could be absorbed. As long as you and I are careful that it's not seen as a solution to allowing this PR to merge though 😓 |
Mamaduka
added
the
[Status] Blocked
|
@Mamaduka I've been thinking about this. I think one option could be to get rid of this internal value code and instead make this a true controlled input. In practice this would mean the following API:
So consumers would wire things up as follows: const value = {
// existing link value from persistence layer somewhere.
// for example block attributes
};
const [newLink, setNewLink] = useState(value);
function persistLink(val) {
// function which updates the persistence layer with
// the new link.
}
return (
<LinkControl
value={newLink}
onChange={setNewLink}
onSubmit={persistLink}
/>
)The benefit is that whilst the consumer accepts responsibility for wiring up the temporary state of the link value (via The problem I see with the approach is backwards compatibility. Up until now We have two options:
Questions:
|
|
Thank you, @getdave! As you mentioned in your previous comment, that burdens consumers more. While I don't think it's an issue if we have used this method from the start, now it might be perceived as a significant breaking change. We have to remember that consumers have to support multiple versions of WP. Let's make this a low priority for now; the gain probably isn't worth it, and the annoying issues could be solved by memoizing the What do you think? |
Exactly. This is the major problem. I think the advantages aren't worth it, especially with the backwards compatibility concerns I outlined before.
👍 I think we should update the documentation for the component to recommend this approach. |
|
Sorry for coming back to this so late! It seems like changing the API dramatically is not worth it at this stage for backward-compatibility reasons. Using I propose deriving states by storing states from the previous renders instead: - useEffect( () => {
- setInternalValue( ( prevValue ) => {
- if ( value && value !== prevValue ) {
- return value;
- }
-
- return prevValue;
- } );
- }, [ value ] );
+ const [ previousValue, setPreviousValue ] = useState( value );
+
+ if ( value !== previousValue ) {
+ setPreviousValue( value );
+ if ( value !== internalValue ) {
+ setInternalValue( value );
+ }
+ }It seems to work in my testing, WDYT? |
I've tested this and it appears to work as described 🥳 The unit test below will pass as well: |
|
|
||
| Why would this happen? | ||
|
|
||
| React's reconciliation algorithm means that if you return the same element from a component, it keeps the nodes around as an optimization, even if the props change. This means that if you render two (or more) `<LinkControl>`s, it is possible that the `value` from one will appear in the other as you move between them. |
There was a problem hiding this comment.
This could be slightly misleading. React only does that when the component node is at the same place in the tree. This happens in gutenberg because we use SlotFill to portal the component to different places, which in the meantime tricks the React reconciler into thinking that they are the same node.
There was a problem hiding this comment.
Questions:
- How would you recommend we improve this section of the README?
- Shall we just remove this stuff about
key? Or do you think it remains relevant (I think it is)?
There was a problem hiding this comment.
I don't have a preference TBH. I think it's okay to keep this, but maybe we can also make it less misleading. We could also add a link to the official React doc about reconciling for reference.
|
I'm not sure about the new solution. It doesn't do much if the component doesn't receive a stable value reference; when the value changes, both states will be updated. Basically, we're just replacing effect with state update during render. Yes, it fixes double re-rendering, but not the original issue. |
|
Pinging @Mamaduka for a final 👀 before we merge this one. I suggest that if merged we do not include this in WP 6.4. That will allow sufficient time for any unintended consequences of the changes to filter through outside of a major release BETA period. It can then safely land in 6.5. |
Yep! It's simply a replacement for the
Is the original issue about |
|
I guess we can also do a deep equality checks and only sync state then. WDYT? But I'm also okay with current solution. Example: if ( ! fastDeepEqual( value, internalValue ) ) {
setInternalValue( value );
} |
Sounds good to me too! 💯 |
|
Okay, I've adjusted a snippet a little bit. We want to keep the previous value snapshot in a separate state and compare it against it as the internal state changes based on user input. if ( ! fastDeepEqual( value, previousValue ) ) {
setPreviousValue( value );
setInternalValue( value );
} |
getdave
force-pushed
the
remove/value-syncing-in-link-control
branch
from
cdc8734
to
1e9288f
Compare
I've applied this change and tests are ✅ Specifically this test |
|
Thanks, @getdave! Let's see if all tests are passing, including e2e, and then I think we're good to merge. |
getdave
removed
the
[Status] Blocked
What?
Removes the (potentially) unwanted value syncing in Link Control component.
Closes #51256
Why?
It might be unnecessary as per #51256
How?
Remove the effectful code.
Testing Instructions
Run the tests with
Testing Instructions for Keyboard
Screenshots or screencast