-
Notifications
You must be signed in to change notification settings - Fork 393
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
Conversation
// which does the same thing as an explicit `static validationOptOut = ['attr1', 'attr2']`. | ||
const hostMutatedValue = renderer.getAttribute(elm, 'data-lwc-host-mutated'); | ||
if (isString(hostMutatedValue)) { | ||
const mutatedAttrValues = new Set(StringSplit.call(hostMutatedValue, / /)); |
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.
(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 comment
The reason will be displayed to describe this comment to others. Learn more.
existingMutationAttribute ? existingMutationAttribute.value.split(' ') : [] | |
existingMutationAttribute ? existingMutationAttribute.value.split(/ /) : [] |
.split(/ /)
is faster than .split(' ')
or StringSplit.call(value, delimiter)
according to a quick lil jsperf comparison.
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.
We're less concerned about compiler perf than runtime perf, but yeah it would have been nice to use / /
consistently as well.
); | ||
attrNameValues.add(attributeName.toLowerCase()); | ||
|
||
const newMutationAttributeValue = [...attrNameValues].sort().join(' '); |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Consistent output formatting and to look nice, yeah.
Details
A flaw with #4358 is that it ignores all hydration mismatches whenever any attribute is mutated server-side in a
connectedCallback
. This PR fixes that, by encoding the mutated attributes in thedata-lwc-host-mutated
attribute and processing those on the client side during hydration.This ensures that we are maximally strict about hydration validation, while still ignoring any attributes mutated in
connectedCallback
.Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
Kinda? But this feature hasn't really shipped yet, so it should be safe to change.