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.
| - 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.
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