-
Notifications
You must be signed in to change notification settings - Fork 419
fix: check for connectedness before running scheduled rehydration #5480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e360cc5
f42948b
fc014b5
ea515d6
8990ad4
0bd84c7
e8fc09a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -673,7 +673,14 @@ function flushRehydrationQueue() { | |
for (let i = 0, len = vms.length; i < len; i += 1) { | ||
const vm = vms[i]; | ||
try { | ||
rehydrate(vm); | ||
// 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); | ||
} | ||
Comment on lines
+676
to
+683
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the entire change, we just need to check that the Times when the Example scenario: VM2 is scheduled for rehydration due to update from wire adapter. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @abdulsattar can you please verify if this will have an effect of HMR? I see some usages of |
||
} catch (error) { | ||
if (i + 1 < len) { | ||
// pieces of the queue are still pending to be rehydrated, those should have priority | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,10 @@ import TimingParentLight from 'timing/parentLight'; | |
import ReorderingList from 'reordering/list'; | ||
import ReorderingListLight from 'reordering/listLight'; | ||
import Details from 'x/details'; | ||
import MutationsParent from 'mutations/parent'; | ||
import MutationsParentLight from 'mutations/parentLight'; | ||
|
||
import { extractDataIds } from 'test-utils'; | ||
|
||
function resetTimingBuffer() { | ||
window.timingBuffer = []; | ||
|
@@ -319,11 +323,18 @@ describe('connectedCallback/renderedCallback timing when reconnected', () => { | |
resetTimingBuffer(); | ||
|
||
document.body.appendChild(elm); | ||
expect(window.timingBuffer).toEqual( | ||
!lwcRuntimeFlags.DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE | ||
? ['parent:connectedCallback', 'child:connectedCallback'] | ||
: ['parent:connectedCallback'] | ||
); | ||
|
||
const expected = ['parent:connectedCallback']; | ||
|
||
if (lwcRuntimeFlags.DISABLE_DETACHED_REHYDRATION) { | ||
expected.push('parent:renderedCallback'); | ||
} | ||
|
||
if (!lwcRuntimeFlags.DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE) { | ||
expected.push('child:connectedCallback'); | ||
} | ||
Comment on lines
+327
to
+335
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
expect(window.timingBuffer).toEqual(expected); | ||
}); | ||
}); | ||
}); | ||
|
@@ -436,3 +447,118 @@ describe('attributeChangedCallback', () => { | |
expect(details.getAttribute('open')).toBeNull(); | ||
}); | ||
}); | ||
|
||
describe('child mutations - scheduled rehydration', () => { | ||
const scenarios = [ | ||
{ | ||
testName: 'shadow', | ||
tagName: 'mutations-parent', | ||
Ctor: MutationsParent, | ||
}, | ||
{ | ||
testName: 'light', | ||
tagName: 'mutations-parent-light', | ||
Ctor: MutationsParentLight, | ||
}, | ||
]; | ||
|
||
scenarios.forEach(({ testName, tagName, Ctor }) => { | ||
describe(testName, () => { | ||
it('connect', async () => { | ||
const elm = createElement(tagName, { is: Ctor }); | ||
document.body.appendChild(elm); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'parent:connectedCallback', | ||
'child1:connectedCallback', | ||
'grand:child1:connectedCallback', | ||
'grand:child1:renderedCallback', | ||
'child1:renderedCallback', | ||
'parent:renderedCallback', | ||
]); | ||
}); | ||
|
||
it('connect/mutate-child', async () => { | ||
const elm = createElement(tagName, { is: Ctor }); | ||
document.body.appendChild(elm); | ||
resetTimingBuffer(); | ||
|
||
const ids = extractDataIds(elm); | ||
ids.child1.addChild(); | ||
|
||
await Promise.resolve(); | ||
|
||
expect(window.timingBuffer).toEqual([ | ||
'grand:child2:connectedCallback', | ||
'grand:child2:renderedCallback', | ||
'child1:renderedCallback', | ||
]); | ||
}); | ||
|
||
it('connect/mutate-child/disconnect-child', async () => { | ||
const elm = createElement(tagName, { is: Ctor }); | ||
document.body.appendChild(elm); | ||
resetTimingBuffer(); | ||
|
||
const ids = extractDataIds(elm); | ||
// Mutate child - grand child 2 | ||
ids.child1.addChild(); | ||
// Disconnect the child that was just mutated | ||
elm.disconnectLastChild(); | ||
|
||
await Promise.resolve(); | ||
|
||
const expected = [ | ||
'child1:disconnectedCallback', | ||
'grand:child1:disconnectedCallback', | ||
'parent:renderedCallback', | ||
]; | ||
|
||
if ( | ||
lwcRuntimeFlags.DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE && | ||
!lwcRuntimeFlags.DISABLE_DETACHED_REHYDRATION | ||
) { | ||
// These are fired in the children of the disconnected child | ||
expected.push( | ||
'grand:child2:connectedCallback', | ||
'grand:child2:renderedCallback' | ||
); | ||
} | ||
|
||
expect(window.timingBuffer).toEqual(expected); | ||
}); | ||
|
||
it('connect/disconnect-child/mutate-child', async () => { | ||
const elm = createElement(tagName, { is: Ctor }); | ||
document.body.appendChild(elm); | ||
resetTimingBuffer(); | ||
|
||
const ids = extractDataIds(elm); | ||
elm.disconnectLastChild(); | ||
// Mutate child that was just disconnected | ||
ids.child1.addChild(); | ||
|
||
await Promise.resolve(); | ||
|
||
const expected = [ | ||
'child1:disconnectedCallback', | ||
'grand:child1:disconnectedCallback', | ||
'parent:renderedCallback', | ||
]; | ||
|
||
if ( | ||
lwcRuntimeFlags.DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE && | ||
!lwcRuntimeFlags.DISABLE_DETACHED_REHYDRATION | ||
) { | ||
// These are fired in the children of the disconnected child | ||
expected.push( | ||
'grand:child2:connectedCallback', | ||
'grand:child2:renderedCallback' | ||
); | ||
} | ||
|
||
expect(window.timingBuffer).toEqual(expected); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<template> | ||
<template for:each={children} for:item="child"> | ||
<mutations-grand-child key={child.uid} uid={child.uid}></mutations-grand-child> | ||
</template> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
@api uid; | ||
@api children = [{ uid: '1' }]; | ||
connectedCallback() { | ||
window.timingBuffer.push(`child${this.uid}:connectedCallback`); | ||
} | ||
renderedCallback() { | ||
window.timingBuffer.push(`child${this.uid}:renderedCallback`); | ||
} | ||
disconnectedCallback() { | ||
// This component could get disconnected by our Karma `test-setup.js` after `window.timingBuffer` has | ||
// already been cleared; we don't care about the `disconnectedCallback`s in that case. | ||
if (window.timingBuffer) { | ||
window.timingBuffer.push(`child${this.uid}:disconnectedCallback`); | ||
} | ||
} | ||
@api | ||
addChild() { | ||
this.children = [...this.children, { uid: `${this.children.length + 1}` }]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<template lwc:render-mode="light"> | ||
<template for:each={children} for:item="child"> | ||
<mutations-grand-child key={child.uid} uid={child.uid}></mutations-grand-child> | ||
</template> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
@api uid; | ||
@api children = [{ uid: '1' }]; | ||
connectedCallback() { | ||
window.timingBuffer.push(`child${this.uid}:connectedCallback`); | ||
} | ||
renderedCallback() { | ||
window.timingBuffer.push(`child${this.uid}:renderedCallback`); | ||
} | ||
disconnectedCallback() { | ||
// This component could get disconnected by our Karma `test-setup.js` after `window.timingBuffer` has | ||
// already been cleared; we don't care about the `disconnectedCallback`s in that case. | ||
if (window.timingBuffer) { | ||
window.timingBuffer.push(`child${this.uid}:disconnectedCallback`); | ||
} | ||
} | ||
@api | ||
addChild() { | ||
this.children = [...this.children, { uid: `${this.children.length + 1}` }]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
<template></template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
@api uid; | ||
connectedCallback() { | ||
window.timingBuffer.push(`grand:child${this.uid}:connectedCallback`); | ||
} | ||
renderedCallback() { | ||
window.timingBuffer.push(`grand:child${this.uid}:renderedCallback`); | ||
} | ||
disconnectedCallback() { | ||
// This component could get disconnected by our Karma `test-setup.js` after `window.timingBuffer` has | ||
// already been cleared; we don't care about the `disconnectedCallback`s in that case. | ||
if (window.timingBuffer) { | ||
window.timingBuffer.push(`grand:child${this.uid}:disconnectedCallback`); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
<template lwc:render-mode="light"></template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
static renderMode = 'light'; | ||
@api uid; | ||
connectedCallback() { | ||
window.timingBuffer.push(`grand:child${this.uid}:connectedCallback`); | ||
} | ||
renderedCallback() { | ||
window.timingBuffer.push(`grand:child${this.uid}:renderedCallback`); | ||
} | ||
disconnectedCallback() { | ||
// This component could get disconnected by our Karma `test-setup.js` after `window.timingBuffer` has | ||
// already been cleared; we don't care about the `disconnectedCallback`s in that case. | ||
if (window.timingBuffer) { | ||
window.timingBuffer.push(`grand:child${this.uid}:disconnectedCallback`); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<template> | ||
<template for:each={children} for:item="child"> | ||
<mutations-child key={child.uid} uid={child.uid} data-id={child.name}></mutations-child> | ||
</template> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import { api, LightningElement } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
@api children = [{ uid: '1', name: 'child1' }]; | ||
connectedCallback() { | ||
window.timingBuffer.push('parent:connectedCallback'); | ||
} | ||
renderedCallback() { | ||
window.timingBuffer.push('parent:renderedCallback'); | ||
} | ||
disconnectedCallback() { | ||
// This component could get disconnected by our Karma `test-setup.js` after `window.timingBuffer` has | ||
// already been cleared; we don't care about the `disconnectedCallback`s in that case. | ||
if (window.timingBuffer) { | ||
window.timingBuffer.push('parent:disconnectedCallback'); | ||
} | ||
} | ||
@api | ||
addChild() { | ||
const uid = this.children.length + 1; | ||
this.children = [...this.children, { uid, name: `child${uid}` }]; | ||
} | ||
@api | ||
disconnectLastChild() { | ||
this.children = this.children.slice(0, this.children.length - 1); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<template lwc:render-mode="light"> | ||
<template for:each={children} for:item="child"> | ||
<mutations-child key={child.uid} uid={child.uid} data-id={child.name}></mutations-child> | ||
</template> | ||
</template> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding test configs for when we can re-enable integration tests in CI.