feat: refactored hydration logging and related tests #5086

jhefferman-sfdc · 2024-12-30T18:51:32Z

Details

TD-0232213 requests more explicit hydration errors.
Implement the changes proposed in this spike

Does this pull request introduce a breaking change?

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

🔬 Yes, it does include an observable change.

GUS work item:

W-17431135

…actor

jhefferman-sfdc · 2024-12-31T20:19:42Z

packages/@lwc/engine-core/src/framework/sanitized-html-content.ts

- * @param first
- * @param second
- */
-export function isSanitizedHtmlContentEqual(first: any, second: any) {


Not used anywhere else

jhefferman-sfdc · 2024-12-31T20:21:29Z

packages/@lwc/engine-core/src/framework/hydration.ts

+    ChildNode = 'child node',
+}
+
+class HydrationError {


More structured logging to keep errors consistent. The errors are stored and not logged until the source element is fully hydrated.

jhefferman-sfdc · 2024-12-31T20:24:08Z

packages/@lwc/engine-core/src/framework/hydration.ts

@@ -514,23 +615,6 @@ function patchElementPropsAndAttrsAndRefs(vnode: VBaseElement, renderer: Rendere
    applyRefs(vnode, vnode.owner);
 }

-function hasCorrectNodeType<T extends Node>(


replaced with dedicated guards (isTypeElement, etc) for type inference (avoids having to specify the type in addition to the type parameter)

jhefferman-sfdc · 2024-12-31T20:27:28Z

packages/@lwc/engine-core/src/framework/hydration.ts

    const { getProperty, getAttribute } = renderer;
-    if (getProperty(client, 'nodeType') === EnvNodeTypes.TEXT) {


IIUC none of this (782-800) would ever execute? areCompatibleStaticNodes is never called unless serverNode (ssr) is of type Element And the check for client can be moved to parent for symmetry? See here

We have code coverage that runs on every commit and can be downloaded from GitHub Actions. I checked, and yes the code is uncovered:

However, this could just be because the hydration checking for static nodes is wrong right now: #4867

Got you. The redundant logic only appeared to concern Element hydration... my assumption was this method was used for multiple node types in the past?

Missed this. Can we change the client type from Node to Element then?

jhefferman-sfdc · 2024-12-31T20:30:36Z

packages/@lwc/integration-karma/helpers/test-utils.js

                    }
                }
            }
        };
    }

+    function stringifyArg(arg) {


Better way to do this? I couldn't decide between just printing the nodeName Or the whole Node. I don't think printing the node makes the tests much better and just adds more to the output? There is also a lot more delta when DISABLE_STATIC_CONTENT_OPTIMIZATION is enabled

jhefferman-sfdc · 2024-12-31T20:34:59Z

packages/@lwc/engine-core/src/framework/hydration.ts

@@ -137,12 +234,29 @@ function hydrateNode(node: Node, vnode: VNode, renderer: RendererAPI): Node | nu
            hydratedNode = hydrateCustomElement(node, vnode, vnode.data.renderer ?? renderer);
            break;
    }
+
+    flushHydrationErrors(hydratedNode);


Errors are queued as they occur and then logged with the source element once it has been hydrated and mounted to the DOM. Means the element in the console matches what is on the page and the highlighting works properly when you hover over the elements in the console.

This would be helpful as a comment. Otherwise it's unclear why we are queueing hydration errors. 🙂

Sounds good, I put the explanation comment on the function definition but it probably makes sense to have them where they are used, too.

jhefferman-sfdc · 2025-01-01T01:17:28Z

packages/@lwc/engine-core/src/framework/hydration.ts

-            if (isSanitizedHtmlContentEqual(getProperty(elm, 'innerHTML'), props.innerHTML)) {
+            const unwrappedServerInnerHTML = unwrapIfNecessary(getProperty(elm, 'innerHTML'));
+            const unwrappedClientInnerHTML = unwrapIfNecessary(props.innerHTML);
+            if (unwrappedServerInnerHTML === unwrappedClientInnerHTML) {


Comparison is done here so the innerHTML can be logged if there is a hydration error

nolanlawson

First pass. This is a big PR, so it may benefit from being broken up. E.g. I would save removing dead code for a future PR.

nolanlawson · 2025-01-02T18:51:11Z

packages/@lwc/engine-core/src/framework/hydration.ts

+        this.serverRendered = serverRendered;
+        this.clientExpected = clientExpected;
+    }
+}


This class adds extra JS at runtime and is pretty unlike most of the other constructs in the LWC engine, which usually deals with plain objects and uses types/interfaces instead. I'd suggest an interface here.

nolanlawson · 2025-01-02T18:52:01Z

packages/@lwc/engine-core/src/framework/hydration.ts

+                `\n- expected on client:`,
+                error.clientExpected || source
+            )
+        );


Use a regular for loop to avoid forEach/ArrayForEach.call controversy here.

Thanks. What is the controversy?

See above about consistency in Array/Object calls. It's controversial because we are not consistent about it.

nolanlawson · 2025-01-02T18:53:13Z

packages/@lwc/engine-core/src/framework/hydration.ts

+function logWarn(...args: any) {
+    /* eslint-disable-next-line no-console */
+    console.warn('[LWC warn:', ...args);
+}


These helpers would be good to put in a hydration-utils.ts or similar. Also logWarn is confusingly named the same as another function in shared/logger.ts.

Good idea, thanks

nolanlawson · 2025-01-02T18:54:36Z

packages/@lwc/engine-core/src/framework/hydration.ts

@@ -137,12 +234,29 @@ function hydrateNode(node: Node, vnode: VNode, renderer: RendererAPI): Node | nu
            hydratedNode = hydrateCustomElement(node, vnode, vnode.data.renderer ?? renderer);
            break;
    }
+
+    flushHydrationErrors(hydratedNode);


This would be helpful as a comment. Otherwise it's unclear why we are queueing hydration errors. 🙂

nolanlawson · 2025-01-02T19:00:07Z

packages/@lwc/engine-core/src/framework/hydration.ts

    let nextNode: Node | null = node;
+    const serverNodes =
+        process.env.NODE_ENV !== 'production'
+            ? Array.from(parentNode?.children).map((c) => c.cloneNode(true))


Suggested change

? Array.from(parentNode?.children).map((c) => c.cloneNode(true))

? ArrayFrom(getChildNodes(parentNode), (child) => cloneNode(child, true))

getChildNodes and cloneNode should come from the owner's renderer.

parentNode should never be nullish here, and I believe childNodes makes more sense since we are analyzing at the node level, not the element level, and ShadowRoot has childNodes but not children.

Sounds good, thank you, although the parameter for childNodes is an Element so perhaps I'm misunderstanding something here. I have some questions around this and the renderer pattern in general but I can ask you later.

The types in renderer are probably just be wrong here. This function could definitely accept a Node and work the same:

lwc/packages/@lwc/engine-dom/src/renderer/index.ts

Lines 175 to 177 in 1a82b99

function getChildNodes(element: Element): NodeList {

return element.childNodes;

}

The renderer pattern dates back to the split between engine-server and engine-dom plus some Locker integration points. Happy to discuss offline.

nolanlawson · 2025-01-02T19:06:00Z

packages/@lwc/engine-core/src/framework/hydration.ts

+*/
+function queueHydrationError(type: string, serverRendered?: any, clientExpected?: any) {
+    if (process.env.NODE_ENV !== 'production') {
+        hydrationErrors.push(new HydrationError(type, serverRendered, clientExpected));


Should be ArrayPush.call

Thanks. Why?

Historically we use language.ts from @lwc/shared for Array and Object prototype methods. Originally this had to do with Locker – now we do it for consistency and (likely) small performance improvements. Note we are not fully consistent about this across the codebase, but I prefer being as consistent as possible.

Note we only even attempt to consistently do this in client (browser) code. So you won't see this pattern in test code or server-side code at all.

nolanlawson · 2025-01-02T19:07:24Z

packages/@lwc/engine-core/src/framework/hydration.ts

+        hasMismatch = true;
+        // We can't know exactly which node(s) caused the delta, but we can provide context (parent) and the mismatched sets
+        if (process.env.NODE_ENV !== 'production') {
+            const clientNodes = children?.map((c) => c?.elm);


ArrayMap.call.

nolanlawson · 2025-01-02T19:11:04Z

packages/@lwc/engine-core/src/framework/hydration.ts

+                'attribute',
+                `${attrName}=${serverRendered}`,
+                `${attrName}=${clientExpected}`
+            );


I tested it out, and it seems Terser is not smart enough to remove the isNull checks and string construction here in development mode. (Nor would that be correct anyway, I guess – e.g. the object could throw an error on its toString() being called.) I'd suggest wrapping this entire call in a if (process.env.NODE_ENV !== 'production') check.

One pattern we have used in these kinds of situations is to have queueHydrationError do an assertNotProd() check rather than check process.env.NODE_ENV itself. This ensures that the caller only calls it in non-prod mode.

Nice, thank you I was trying to avoid executing logging logic in prod and missed this.

nolanlawson · 2025-01-02T19:12:14Z

packages/@lwc/engine-core/src/framework/hydration.ts

-                vnode.owner
-            );
-        }
+        queueHydrationError('attribute', `style="${elmStyle}"`, `style="${vnodeStyle}"`);


Same here about string construction.

Thanks, I wanted to avoid the !== production conditionals everywhere but def makes more sense in the consumer to avoid parameter logic. Changed to that + the assertNotProd in the utilities.

Also: we should use prettyPrintAttribute here for code deduplication, and to handle the undefined case for vnodeStyle. I guess prettyPrintAttribute will have to handle isUndefined as well, but not a big deal.

nolanlawson · 2025-01-02T19:16:30Z

packages/@lwc/engine-core/src/framework/hydration.ts

    const { getProperty, getAttribute } = renderer;
-    if (getProperty(client, 'nodeType') === EnvNodeTypes.TEXT) {


We have code coverage that runs on every commit and can be downloaded from GitHub Actions. I checked, and yes the code is uncovered:

However, this could just be because the hydration checking for static nodes is wrong right now: #4867

nolanlawson · 2025-01-03T17:36:14Z

packages/@lwc/integration-karma/helpers/test-utils.js

-                        }
+                        const args = calls[i];
+                        const argsString = args.map((arg) => stringifyArg(arg)).join(' ');
+                        expect(argsString).toMatch(matcher);


Looks like I missed that toMatch on a strig does the same thing as toContain. Good find!

nolanlawson · 2025-01-03T17:38:07Z

packages/@lwc/integration-karma/helpers/test-utils.js

+            return arg.map((v) => stringifyArg(v));
+        } else if (arg?.nodeName) {
+            // Browsers render nodes differently (class order, etc). Node.nodeName is universal and sufficient for testing.
+            return arg.nodeName;


I think I would use

`<${tagName.toLowerCase()}>`

...if tagName is available. To the average web developer, <li> is much more legible than LI. For non-elements, the nodeName is fine (#comment, #text, etc.).

Perfect, thank you. This stringify only impacts the format of the matchers in our karma tests but I still think it is worth changing. The framework consumers will see elements in the console and not strings - when they hover over them it will higlight the matching element on the page.

I missed this but finally grokked it after watching your demo video. Even for the tests though, I think it's a nice touch. But yeah not super important, I get that now. 😅

nolanlawson · 2025-01-03T17:39:54Z

packages/@lwc/integration-karma/test-hydration/attributes/falsy-mismatch/index.spec.js

-                'Mismatch hydrating element <div>: attribute "data-foo" has different values, expected "undefined" but found null',
-                'Mismatch hydrating element <div>: attribute "data-foo" has different values, expected "null" but found null',
+                'Hydration attribute mismatch on: DIV - rendered on server: data-foo=null - expected on client: data-foo="undefined"',
+                'Hydration attribute mismatch on: DIV - rendered on server: data-foo=null - expected on client: data-foo="null"',


This is much more legible than before, but again I think <div> is better than DIV here.

nolanlawson · 2025-01-03T17:42:56Z

packages/@lwc/engine-core/src/framework/hydration.ts

+                `\n- rendered on server:`,
+                error.serverRendered,
+                `\n- expected on client:`,
+                error.clientExpected || source


The fact that the newlines and string length cause everything to line up is a really nice touch.

jhefferman-sfdc · 2025-01-03T17:47:27Z

First pass. This is a big PR, so it may benefit from being broken up. E.g. I would save removing dead code for a future PR.

Thank you very much for the review, for the dead code I removed it as it would have required a refactor, increases bundle size and isn't executed? In terms of size I figured a good chunk of it was the repetitive test coverage changes but you are right that re-introducing the dead code is probably the easiest candidate for size reduction as everything else is part of a single feature really.

If you feel strongly I will refactor the dead code to use the new type guards and re-introduce it, as you want.

nolanlawson · 2025-01-03T17:48:10Z

packages/@lwc/engine-core/src/framework/hydration.ts

+                queueHydrationError(
+                    'attribute',
+                    `${attrName}="${serverAttributeValue}"`,
+                    `${attrName}="${clientAttributeValue}"`


This won't pretty-print properly if getAttribute returns null. In that case, it should print without the double-quotes.

Note null vs string is the only case we have to care about here – getAttribute won't return anything else.

Thanks, replaced with a utility

nolanlawson

Overall this LGTM, I just have a few nitpicks. The new warning messages are looking way better than the old ones.

nolanlawson · 2025-01-03T17:54:14Z

If you feel strongly I will refactor the dead code to use the new type guards and re-introduce it, as you want.

I looked into it again, and you're right, this code is truly dead. I missed what you had said here. The hasCorrectNodeType check already ensures that we aren't comparing an Element against a non-Element. Feel free to remove it.

jhefferman-sfdc · 2025-01-03T19:18:45Z

Overall this LGTM, I just have a few nitpicks. The new warning messages are looking way better than the old ones.

Thank you, I think some of the big wins will be:

logging the actual elements so they are higlighted on hover
no more generic errors, e.g. 'Hydration mismatch: incorrect node type received' is now 'Hydration node mismatch on: #text - rendered on server: #comment - expected on client: #text',

I hope LWR like it! If you have time, feel free to take a look at this spike doc which has a comparison with other frameworks, demos, etc.

nolanlawson

Looking very good to me! A few remaining nitpicks, but otherwise

nolanlawson · 2025-01-03T21:12:33Z

packages/@lwc/engine-core/src/framework/hydration-utils.ts

+ */
+export function flushHydrationErrors(source?: Node | null) {
+    assertNotProd(); // this method should never leak to prod
+    for (let i = 0; i < hydrationErrors.length; i++) {


No biggie, but for future reference for...of is fine. I think the old-school for loop still wins on micro-benchmarks, but it's not important for dev-only code.

nolanlawson · 2025-01-03T21:14:41Z

packages/@lwc/engine-core/src/framework/hydration.ts

+        */
+        flushHydrationErrors(vm.renderRoot);
+        if (hasMismatch) {
+            logHydrationError('Hydration completed with errors.');


Probably time to rename this from "error" to "warning" but no big deal.

nolanlawson · 2025-01-03T21:16:02Z

packages/@lwc/engine-core/src/framework/hydration.ts

+
+    const serverNodes =
+        process.env.NODE_ENV !== 'production'
+            ? Array.from(getChildNodes(parentNode)).map((c) => c.cloneNode(true))


ArrayFrom, ArrayMap, renderer.cloneNode, etc. Also the ArrayMap is not necessary because the second argument to ArrayFrom can be a map function.

nolanlawson · 2025-01-03T21:21:00Z

packages/@lwc/engine-core/src/framework/hydration.ts

-                vnode.owner
-            );
-        }
+        queueHydrationError('attribute', `style="${elmStyle}"`, `style="${vnodeStyle}"`);


Also: we should use prettyPrintAttribute here for code deduplication, and to handle the undefined case for vnodeStyle. I guess prettyPrintAttribute will have to handle isUndefined as well, but not a big deal.

nolanlawson · 2025-01-03T21:23:22Z

packages/@lwc/integration-karma/test-hydration/mismatches/favors-client-side-text/index.spec.js

+                    'Hydration text content mismatch on: <p> - rendered on server: hello! - expected on client: bye!',
+                ],
+            });
+        }


This is a bit odd – we could potentially rewrite the hydration warnings to align these two cases. It's not a big deal though.

What did you have in mind? I didn't take a deep look but my understanding was the generic Hydration mismatch: text values do not match, will recover from the difference message masked the fact that the nodes were different when the flag was enabled. Could open an issue for further enhancement?

It's not a big deal. The issue here is that, for the static content optimization, we detect the mismatch at the element level, whereas without the optimization, we detect it at the text level.

Likely we should just figure out what's wrong with static content mismatches entirely: #4867. We have also seen this kind of issue before: #2962

feat: refactored hydration logging, related test changes

326648b

jhefferman-sfdc requested a review from a team as a code owner December 30, 2024 18:51

jhefferman-sfdc changed the title ~~feat: refactored hydration logging, related test changes~~ feat: refactored hydration logging and related tests Dec 30, 2024

jhefferman-sfdc added 4 commits December 30, 2024 18:28

fix: remove console usage

e8106d5

fix: stringify

cb87c1f

fix: reduce nodes to nodeName for browser parity

a4fabab

fix: disable_static_content_optimization test exceptions, logging ref…

e210d8f

…actor

jhefferman-sfdc commented Dec 31, 2024

View reviewed changes

fix: changes for bundle size reduction

b5c9c74

jhefferman-sfdc commented Jan 1, 2025

View reviewed changes

jhefferman-sfdc added 2 commits January 1, 2025 09:58

fix: remove redundant non-prod checks

1e67d7e

fix: irregular naming, type guard

c6fa46c

nolanlawson reviewed Jan 2, 2025

View reviewed changes

nolanlawson reviewed Jan 3, 2025

View reviewed changes

jhefferman-sfdc added 2 commits January 3, 2025 12:05

fix: review comments

9c74f88

fix: typing for static element comparison

bfe4c54

fix: missing util

37fd585

fix: move pretty print classes to util

678fc13

nolanlawson approved these changes Jan 3, 2025

View reviewed changes

jhefferman-sfdc added 2 commits January 3, 2025 15:25

fix: naming, attribute prettifier

869caee

fix: rename logging function

63de336

jhefferman-sfdc merged commit 47c46eb into master Jan 9, 2025
10 checks passed

jhefferman-sfdc deleted the jhefferman/hydration-logging branch January 9, 2025 22:36

		const { getProperty, getAttribute } = renderer;
		if (getProperty(client, 'nodeType') === EnvNodeTypes.TEXT) {

	? Array.from(parentNode?.children).map((c) => c.cloneNode(true))
	? ArrayFrom(getChildNodes(parentNode), (child) => cloneNode(child, true))

	function getChildNodes(element: Element): NodeList {
	return element.childNodes;
	}

feat: refactored hydration logging and related tests #5086

feat: refactored hydration logging and related tests #5086

Conversation

jhefferman-sfdc commented Dec 30, 2024 • edited Loading

Details

Does this pull request introduce a breaking change?

GUS work item:

Choose a reason for hiding this comment

jhefferman-sfdc Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

jhefferman-sfdc Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

jhefferman-sfdc Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhefferman-sfdc Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

jhefferman-sfdc Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhefferman-sfdc Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhefferman-sfdc commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson left a comment

Choose a reason for hiding this comment

nolanlawson commented Jan 3, 2025

jhefferman-sfdc commented Jan 3, 2025 • edited Loading

nolanlawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhefferman-sfdc Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

nolanlawson Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

jhefferman-sfdc commented Dec 30, 2024 •

edited

Loading

jhefferman-sfdc Dec 31, 2024 •

edited

Loading

jhefferman-sfdc Dec 31, 2024 •

edited

Loading

jhefferman-sfdc Dec 31, 2024 •

edited

Loading

jhefferman-sfdc Dec 31, 2024 •

edited

Loading

jhefferman-sfdc Dec 31, 2024 •

edited

Loading

jhefferman-sfdc Jan 3, 2025 •

edited

Loading

nolanlawson Jan 3, 2025 •

edited

Loading

jhefferman-sfdc commented Jan 3, 2025 •

edited

Loading

jhefferman-sfdc Jan 3, 2025 •

edited

Loading

nolanlawson Jan 3, 2025 •

edited

Loading