fix: skip hydration validation when static parts have static classes

[jmsjtu]

Details

Fixes #4270

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.

Hydration validation will no longer fail incorrectly for statically optimized elements.

GUS work item

W-15963098

[jmsjtu]

validateStyleAttr already checks if the style attribute exists in the static parts but I decided to check here as well to make it clear why we can skip validating it for statically optimized elements.

[jmsjtu]

This test fails without the fix

[nolanlawson]

I'm concerned we are squelching true mismatch warnings, see comment above.

[nolanlawson]

Should we also test the reverse case, where the style is static but the class is dynamic?

[nolanlawson]

I'm not so sure about this logic. Consider this case: 8a22043.

If you have a static element:

<p class="c1" lwc:ref="p">text</p>

but then you mutate it in renderedCallback:

renderedCallback() {
    this.refs.p.classList.add('hahaha!');
}

then we would expect a hydration mismatch error, because the client side is adding the new class (via renderedCallback), and renderedCallback is not invoked server-side.

This works correctly in master (hydration mismatch warning). However, with this PR, the hydration mismatch warning actually gets squelched.

[jmsjtu]

That's a good point!

I hadn't considered that scenario, I'll add a test case for it and look into how to address it.

We'll need to have a way to determine what the static class/style values are during hydration.

[jmsjtu]

I think there might be an additional scenario we're skipping here.

If the renderedCallback dynamically adds a class to an element without static parts, we won't be able to validate against it either:

<p class="c1">text</p>

renderedCallback() {
    this.template.querySelector('p').classList.add('hahaha!');
}

Since the <p> doesn't have any static parts we're bypassing the hydration validation.

Here's a demo 628e711 from master.

The classes don't match but there's no hydration validation error.

The same might apply for style as well.

[divmain]

During this function, we are comparing two things:

1. the element created as a result of SSR HTML
2. the VDOM generated client-side

renderedCallback is not involved in creating thing 1. It is also not involved in creating thing 2; renderedCallback may mutate the DOM directly, but it has no mechanism to mutate the VDOM. Additionally, at the point in time when this code is running, renderedCallback has not executed yet. It operates after VDOM is generated & the diffing algo updates corresponding elements.

In other words, this scenario should not be a concern & I believe the logic in the PR should be fine.

[divmain]

That said, I don't know why it results in a hydration mismatch in main... that's a little concerning. Let me take a quick look at that.

[divmain]

Looking at 8a22043, I don't know why that would fail in main. I'm kind of thinking that may have been a failure case that shouldn't have been a failure case, i.e. it should pass hydration without issue.

[nolanlawson]

I was wondering if this was something that changed due to native custom element lifecycle, but I just tested with DISABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE = true and I'm still getting the same result (hydration mismatch) from 8a22043.

I think you might be right @divmain – my test case is a little busted because the ordering goes like this:

• hydrateComponent()
• renderedCallback()
• test() <-- checks that the p.className is what we expect

I guess if we only care about validating SSR'd HTML against the VDOM at the moment before connectedCallback/renderedCallback, then yes the current code should be fine.

[nolanlawson]

Looking at 8a22043, I don't know why that would fail in main.

It fails because it needs the fix from this PR. 🙂 If you apply this PR on top of that commit, then the only error is from the test() function itself (because it runs after renderedCallback and checks that the p.className is what it expects). There are no hydration mismatches.

[divmain]

Once you have the test for the case "where the style is static but the class is dynamic", I think this PR looks good to me.

[divmain]

Looking at 8a22043, I don't know why that would fail in main. I'm kind of thinking that may have been a failure case that shouldn't have been a failure case, i.e. it should pass hydration without issue.