Skip to content
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

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

nolanlawson
Copy link
Collaborator

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 the data-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?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🔬 Yes, it does include an observable change.

Kinda? But this feature hasn't really shipped yet, so it should be safe to change.

@nolanlawson nolanlawson merged commit 4d13710 into master Jul 17, 2024
11 checks passed
@nolanlawson nolanlawson deleted the nolan/hydration-only-those-attrs branch July 17, 2024 15:09
// 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, / /));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ / looks weird...

Copy link
Collaborator Author

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

Copy link
Contributor

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(' ') : []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

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(' ');
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants