fix: check for connectedness before running scheduled rehydration#5480
fix: check for connectedness before running scheduled rehydration#5480
Conversation
| // We want to prevent rehydration from occurring when nodes are detached from the DOM as this can trigger | ||
| // unintended side effects, like lifecycle methods being called multiple times. | ||
| // For backwards compatibility, we use a flag to control the check. | ||
| // 1. When flag is disabled always rehydrate (legacy behavior) | ||
| // 2. When flag is enabled only rehydrate when the VM state is connected (fixed behavior) | ||
| if (!lwcRuntimeFlags.DISABLE_DETACHED_REHYDRATION || vm.state === VMState.connected) { | ||
| rehydrate(vm); | ||
| } |
There was a problem hiding this comment.
This is the entire change, we just need to check that the vm is connected before rehydrating.
Times when the vm is disconnected before hydration occur during async race conditions.
Example scenario:
VM2 is scheduled for rehydration due to update from wire adapter.
VM1, grand-owner of VM2, is scheduled for rehydration.
VM1 rehydration goes first due to idx-sorting.
VM1 rendering disconnects VM2 from the DOM.
VM2 disconnectedCallback fires.
VM2 rehydration triggers, calling connectedCallback and render for entire disconnected subtree.
This will become a more apparent issue with the introduction of state managers, as the state manger can trigger reactivity asynchronously from outside of the component.
This issue has occurred frequently for team using redux.
There was a problem hiding this comment.
@abdulsattar can you please verify if this will have an effect of HMR?
I see some usages of scheduleRehydration in the hot-swaps code.
| const expected = ['parent:connectedCallback']; | ||
|
|
||
| if (lwcRuntimeFlags.DISABLE_DETACHED_REHYDRATION) { | ||
| expected.push('parent:renderedCallback'); | ||
| } | ||
|
|
||
| if (!lwcRuntimeFlags.DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE) { | ||
| expected.push('child:connectedCallback'); | ||
| } |
There was a problem hiding this comment.
This changed because previously, both the parent and child were being rehydrated while they were both disconnected from the DOM.
When the elements are re-inserted into the DOM, the VMs are not longer marked as dirty so rehydration is skipped.
The renderedCallback did not fire previously because the parent element was not connected to the DOM.
There was a problem hiding this comment.
Can we record these concrete "this behavior changed" scenarios somewhere? It might really help both internal devs and support deal with potential breakages quicker if we have a list of "known scenarios" that might be caused by this.
There was a problem hiding this comment.
Also, looking at other examples below, shouldn't the connectedCallback of the child appear before the renderedCallback of the parent? Or am I misreading something?
There was a problem hiding this comment.
Can we record these concrete "this behavior changed" scenarios somewhere? It might really help both internal devs and support deal with potential breakages quicker if we have a list of "known scenarios" that might be caused by this.
@jye-sf I created a follow-up item to create some more tests and provide some documentation on what the breaking change is.
I think we should add it to the release notes as well.
Also, looking at other examples below, shouldn't the connectedCallback of the child appear before the renderedCallback of the parent? Or am I misreading something?
This is kinda funky behavior but it's what we're expecting. It should behavior like the connect/disconnect/reconnect test.
Essentially, the rehydration caused by the mutation should be discounted because the element is removed from the DOM.
That means the behavior should match the previous test.
| - run: DISABLE_DETACHED_REHYDRATION=1 yarn sauce:ci | ||
| - run: DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE=1 DISABLE_SYNTHETIC=1 yarn sauce:ci | ||
| - run: DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE=1 DISABLE_DETACHED_REHYDRATION=1 yarn sauce:ci | ||
|
|
There was a problem hiding this comment.
Adding test configs for when we can re-enable integration tests in CI.
jye-sf
left a comment
jye-sf
left a comment
There was a problem hiding this comment.
No major concerns with the code itself.
Can we include a link to the rollout plan in the PR? That's my biggest concern with this.
| const expected = ['parent:connectedCallback']; | ||
|
|
||
| if (lwcRuntimeFlags.DISABLE_DETACHED_REHYDRATION) { | ||
| expected.push('parent:renderedCallback'); | ||
| } | ||
|
|
||
| if (!lwcRuntimeFlags.DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE) { | ||
| expected.push('child:connectedCallback'); | ||
| } |
There was a problem hiding this comment.
Can we record these concrete "this behavior changed" scenarios somewhere? It might really help both internal devs and support deal with potential breakages quicker if we have a list of "known scenarios" that might be caused by this.
| const expected = ['parent:connectedCallback']; | ||
|
|
||
| if (lwcRuntimeFlags.DISABLE_DETACHED_REHYDRATION) { | ||
| expected.push('parent:renderedCallback'); | ||
| } | ||
|
|
||
| if (!lwcRuntimeFlags.DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE) { | ||
| expected.push('child:connectedCallback'); | ||
| } |
There was a problem hiding this comment.
Also, looking at other examples below, shouldn't the connectedCallback of the child appear before the renderedCallback of the parent? Or am I misreading something?
Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>
|
/nucleus ignore -m "unrelated downstream breakages" |
3 participants
Details
Does this pull request introduce a breaking change?
This PR fixes an issue where disconnected nodes could be scheduled for rehydration in the framework's reactivity system.
This means that disconnected nodes could have lifecycle methods, getters/setters, and function invocations happen on nodes that were disconnected from the DOM.
Any downstream users that had a dependency on this behavior will be impacted.
Does this pull request introduce an observable change?
See ☝️
GUS work item
W-19482166