-
Notifications
You must be signed in to change notification settings - Fork 394
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
fix(hydration): only ignore mutated host attributes #4385
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1,4 @@ | ||
<x-cmp aria-label="haha" class="yolo woot" data-foo="bar" data-lwc-host-mutated="aria-label class data-foo"> | ||
<template shadowrootmode="open"> | ||
</template> | ||
</x-cmp> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export const tagName = 'x-cmp'; | ||
export { default } from 'x/cmp'; | ||
export * from 'x/cmp'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
<template> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
connectedCallback() { | ||
// Modify this component's class and attributes at runtime | ||
// We expect a data-lwc-host-mutated attr to be added with the mutated attribute names in unique sorted order | ||
this.setAttribute('data-foo', 'bar') | ||
this.classList.add('yolo') | ||
this.classList.add('woot') | ||
this.setAttribute('aria-label', 'haha') | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
<x-cmp aria-activedescendant="foo" aria-busy="true" data-lwc-host-mutated> | ||
<x-cmp aria-activedescendant="foo" aria-busy="true" data-lwc-host-mutated="aria-activedescendant aria-busy"> | ||
<template shadowrootmode="open"> | ||
</template> | ||
</x-cmp> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
<x-cmp ARIA-LABEL="haha" DATA-FOO="bar" data-lwc-host-mutated="aria-label data-bar data-foo"> | ||
<template shadowrootmode="open"> | ||
</template> | ||
</x-cmp> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export const tagName = 'x-cmp'; | ||
export { default } from 'x/cmp'; | ||
export * from 'x/cmp'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
<template> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class extends LightningElement { | ||
connectedCallback() { | ||
// Modify this component's attributes at runtime, using uppercase | ||
// We expect a data-lwc-host-mutated attr to be added with the mutated attribute names in unique sorted order, | ||
// all lowercase | ||
this.setAttribute('DATA-FOO', 'bar') | ||
this.setAttribute('ARIA-LABEL', 'haha') | ||
this.removeAttribute('dAtA-BaR') | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
<x-getter-class-list class="a c d-e" data-lwc-host-mutated> | ||
<x-getter-class-list class="a c d-e" data-lwc-host-mutated="class"> | ||
<template shadowrootmode="open"> | ||
</template> | ||
</x-getter-class-list> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
<x-method-remove-attribute data-a data-c data-lwc-host-mutated> | ||
<x-method-remove-attribute data-a data-c data-lwc-host-mutated="data-a data-b data-c data-unknown"> | ||
<template shadowrootmode="open"> | ||
</template> | ||
</x-method-remove-attribute> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
<x-method-set-attribute data-boolean="true" data-empty-string data-lwc-host-mutated data-null="null" data-number="1" data-override="override" data-string="test"> | ||
<x-method-set-attribute data-boolean="true" data-empty-string data-lwc-host-mutated="data-boolean data-empty-string data-null data-number data-override data-string" data-null="null" data-number="1" data-override="override" data-string="test"> | ||
<template shadowrootmode="open"> | ||
</template> | ||
</x-method-set-attribute> |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,16 +10,25 @@ const elementsToTrackForMutations: WeakSet<HostElement> = new WeakSet(); | |||||
|
||||||
const MUTATION_TRACKING_ATTRIBUTE = 'data-lwc-host-mutated'; | ||||||
|
||||||
export function reportMutation(element: HostElement) { | ||||||
export function reportMutation(element: HostElement, attributeName: string) { | ||||||
if (elementsToTrackForMutations.has(element)) { | ||||||
const hasMutationAttribute = element[HostAttributesKey].find( | ||||||
const existingMutationAttribute = element[HostAttributesKey].find( | ||||||
(attr) => attr.name === MUTATION_TRACKING_ATTRIBUTE && attr[HostNamespaceKey] === null | ||||||
); | ||||||
if (!hasMutationAttribute) { | ||||||
const attrNameValues = new Set( | ||||||
existingMutationAttribute ? existingMutationAttribute.value.split(' ') : [] | ||||||
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.
Suggested change
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. We're less concerned about compiler perf than runtime perf, but yeah it would have been nice to use |
||||||
); | ||||||
attrNameValues.add(attributeName.toLowerCase()); | ||||||
|
||||||
const newMutationAttributeValue = [...attrNameValues].sort().join(' '); | ||||||
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. Do we need to sort here or is it just for nice? 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. Consistent output formatting and to look nice, yeah. |
||||||
|
||||||
if (existingMutationAttribute) { | ||||||
existingMutationAttribute.value = newMutationAttributeValue; | ||||||
} else { | ||||||
element[HostAttributesKey].push({ | ||||||
name: MUTATION_TRACKING_ATTRIBUTE, | ||||||
[HostNamespaceKey]: null, | ||||||
value: '', | ||||||
value: newMutationAttributeValue, | ||||||
}); | ||||||
} | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
export default { | ||
props: { | ||
ssr: true, | ||
}, | ||
clientProps: { | ||
ssr: false, | ||
}, | ||
snapshot(target) { | ||
const child = target.shadowRoot.querySelector('x-child'); | ||
const div = child.shadowRoot.querySelector('div'); | ||
|
||
return { | ||
child, | ||
div, | ||
}; | ||
}, | ||
test(target, snapshots, consoleCalls) { | ||
const snapshotAfterHydration = this.snapshot(target); | ||
expect(snapshotAfterHydration.child).not.toBe(snapshots.child); | ||
expect(snapshotAfterHydration.div).not.toBe(snapshots.div); | ||
|
||
const { child } = snapshotAfterHydration; | ||
expect(child.getAttribute('data-foo')).toBe('bar'); | ||
expect(child.getAttribute('data-mutatis')).toBe('mutandis'); | ||
expect(child.getAttribute('class')).toBe('is-client'); | ||
|
||
TestUtils.expectConsoleCallsDev(consoleCalls, { | ||
warn: [], | ||
error: [ | ||
'Mismatch hydrating element <x-child>: attribute "class" has different values, expected "is-client" but found "is-server"', | ||
'Hydration completed with errors.', | ||
], | ||
}); | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<div>{yolo}</div> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class Child extends LightningElement { | ||
yolo = 'woot'; | ||
|
||
connectedCallback() { | ||
this.setAttribute('data-mutatis', 'mutandis'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<x-child data-foo="bar" class={mismatchingClass}></x-child> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class Main extends LightningElement { | ||
@api ssr; | ||
|
||
get mismatchingClass() { | ||
return this.ssr ? 'is-server' : 'is-client'; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
export default { | ||
props: { | ||
ssr: true, | ||
}, | ||
clientProps: { | ||
ssr: false, | ||
}, | ||
snapshot(target) { | ||
const child = target.shadowRoot.querySelector('x-child'); | ||
const div = child.shadowRoot.querySelector('div'); | ||
|
||
return { | ||
child, | ||
div, | ||
}; | ||
}, | ||
test(target, snapshots, consoleCalls) { | ||
const snapshotAfterHydration = this.snapshot(target); | ||
expect(snapshotAfterHydration.child).not.toBe(snapshots.child); | ||
expect(snapshotAfterHydration.div).not.toBe(snapshots.div); | ||
|
||
const { child } = snapshotAfterHydration; | ||
expect(child.getAttribute('class')).toBe('static mutatis'); | ||
expect(child.getAttribute('data-mismatched-attr')).toBe('is-client'); | ||
|
||
TestUtils.expectConsoleCallsDev(consoleCalls, { | ||
warn: [], | ||
error: [ | ||
'Mismatch hydrating element <x-child>: attribute "data-mismatched-attr" has different values, expected "is-client" but found "is-server"', | ||
'Hydration completed with errors.', | ||
], | ||
}); | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<div>{yolo}</div> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class Child extends LightningElement { | ||
yolo = 'woot'; | ||
|
||
connectedCallback() { | ||
this.classList.add('mutatis'); | ||
this.classList.add('mutandis'); | ||
this.classList.remove('mutandis'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<x-child class="static" data-mismatched-attr={mismatchedAttr}></x-child> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class Main extends LightningElement { | ||
@api ssr; | ||
|
||
get mismatchedAttr() { | ||
return this.ssr ? 'is-server' : 'is-client'; | ||
} | ||
} |
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.
/ /
looks weird...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.
Sorry, too late. I did not see your comments before merging.
TypeScript for some weird reason complains if I use
' '
. So I used/ /
.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.
Well I was too slow to review. I bet it's because of our weird re-mapping losing overloaded function signatures.