refactor(engine): removing isHydrating global flag used across packages #2806

caridy · 2022-04-26T23:50:54Z

Details

If seems that isHydrating was used for two main things:

to know whether or not to call attachShadow
to treat style nodes differently

For 1, we can solve that using element internals, since IE11 doesn't support SSR, we can rely on internals or attach a new shadow, and not having to observe anything about rehydration.

For 2, I don't know what the deal is.

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

✅ No, it does not introduce an observable change.

caridy · 2022-04-26T23:52:37Z

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

@@ -209,12 +208,8 @@ export function createStylesheet(vm: VM, stylesheets: string[]): VNode | null {
        for (let i = 0; i < stylesheets.length; i++) {
            insertGlobalStylesheet(stylesheets[i]);
        }
-    } else if (ssr || isHydrating()) {


This is the concerning part. It seems that during rehydration, the style vnode is very different from a regular rendering, and the diffing algo has special behavior for that... I don't know what the deal is... but I will like to know.

If it turns out that this is the case, maybe then the hydration process will NOT necessary try to preserve the original style, but just go with the replacement of it since that will NOT blink due to the insertion/removal.

@caridy The reason is that, in non-SSR, we do two things differently:

Use Constructable Stylesheets where supported (perf boost)

Hoist style nodes to the nearest native shadow root (e.g. for light DOM styles) (also a perf boost)

Neither is possible in SSR. (The first because there's no browser standard for it, and the second because we only do one pass through the DOM tree during the rendering.) So we just emit inline <style> nodes.

For hydration, we can potentially work around this. When hydrating, we can just remove the inline <style>s and do our normal flow. I'm also working on a separate PR to properly remove stylesheets when unused: #2797.

FWIW in #2797 I am also getting rid of this usage of isHydrating, but for a different reason. So if that PR gets merged first, then you won't have to remove it here.

caridy · 2022-04-26T23:53:56Z

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

-        return element.shadowRoot!;
-    }
-    return element.attachShadow(options);
+    const internals = (element as any).attachInternals?.();


This is the most important part of the PR. We rely on attachInternals to access any shadowRoot (open or closed), and that will take care of returning the right shadow. This method in general should probably be renamed in the future to something more like: attachShadowIfNeeded

attachInternals is not supported in Safari. Would we need to polyfill this?

no, not in this case. In the previous incarnation of this code, the option was always to assume that an open shadow was created, and therefor the .shadowRoot value was there. This one assumes that attachInternals might not be there, and then use the dot notation to read open shadowRoots, and only at that point, decide to attach it. Basically, the new code covers more ground than the original code for closed shadows.

So if we assume we're running in Safari, then this code would boil down to:

return element.shadowRoot || element.attachShadow(options)

Can we use this instead of attachInternals? What benefit do we get from using attachInternals in Firefox and Chrome?

exactly, the same old code.

now, in terms of benefits, two main benefits:

avoid users to poison the engine, e.g.: class Foo extends HTMLElement { attachShadow() { return something; } }.

closed shadows hydration.

For # 1, couldn't users poison the engine using class Foo extends HTMLElement { attachInternals() { return something; } } instead?

For # 2, I'm a bit surprised because it seems attachInternals defeats the whole purpose of closed shadow DOM. But I confirmed that you're right, you can access the shadow root that way:

Well, for #1, LWC is NOT protecting against poisoning. LWS does everywhere. You don't protect about them providing attachShadow() jejejeje. You can simply cache the method, and call it to avoid dot notation, but again, that's not a thing... it was a some point, but for some reason, that was removed everywhere.

For #2, NO, attachInternals can only be called during the construction phase... after that it throws.

Based on my testing (see screenshot above), attachInternals can be called after the constructor phase. But it can't be called twice. So I guess a closed-shadow-root component would want to call it during the constructor phase so that someone else can't get access to its shadow root.

If attachInternals is not supported in Safari, and if there's no advantage in Chrome/Firefox to using it, then I kinda feel we are better off avoiding it… otherwise we have the added complexity of 2 code paths for 2 different browsers, with no real advantage.

All the hydration tests currently fails, because the hydrated elements are never registered as custom elements.

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

pmdartus · 2022-05-02T09:23:17Z

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

 export function attachShadow(element: Element, options: ShadowRootInit): ShadowRoot {
-    if (hydrating) {
-        return element.shadowRoot!;
-    }
-    return element.attachShadow(options);
+    const internals = (element as any).attachInternals?.();


You can remove the any if you change the element type from Element to HTMLElement.

export function attachShadow(element: HTMLElement, options: ShadowRootInit): ShadowRoot { const internals = element.attachInternals?.();

pmdartus · 2022-05-02T09:26:37Z

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

-        return element.shadowRoot!;
-    }
-    return element.attachShadow(options);
+    const internals = (element as any).attachInternals?.();


All the hydration tests currently fails, because the hydrated elements are never registered as custom elements.

caridy · 2022-05-03T20:18:23Z

Let's keep this as a draft since there are intersection with other various PRs.

All the hydration tests currently fails, because the hydrated elements are never registered as custom elements.

@pmdartus that is precisely one of the problems here. It is a gap in our tests, but let me explain in details what is that a problem:

If you have a component A that is being hydrated from SSR, and that component uses component B and C in its shadow root content (also from SSR) - in that order, when component B is going to be hydrated, an error of some sort occur, e.g.: mismatch or the content of B's shadow is just empty and will be rendered on the first pass, and it uses C inside B, which is not going to be hydrated, but created, it is going to get registered. So, by the time you get to C inside A, C was already registered. I hope that this explains the unpredictable aspect of the current code. This PR can solve that because it doesn't matter when something is registered, but when the engine attempts to upgrade it instead.

I need some help to investigate what's going on here.

nolanlawson · 2022-05-11T17:56:19Z

isHydrating is no longer used in stylesheet.ts now: #2827

caridy · 2022-05-12T19:19:46Z

isHydrating is no longer used in stylesheet.ts now: #2827

awesome... I will update this PR.

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

nolanlawson · 2022-05-16T22:05:07Z

Looks like the hydration tests are still failing.

caridy · 2022-05-24T03:41:40Z

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

-    }
-    return element.attachShadow(options);
+export function attachShadow(element: HTMLElement, options: ShadowRootInit): ShadowRoot {
+    const internals = element.attachInternals?.();


@nolanlawson yes, as @pmdartus pointed out, the problem persist on this line. The fact that tagNames are not registered during rehydration makes this line to throw when attachInternals is present, but the element was not upgraded by the UA. I don't have a solution just yet for this problem, in another PR I'm proposing to always register before any form of upgrading, but this PR should get in first... maybe just adding a try/catch for now and eventually removing the try/catch.

I have added the try/catch.

nolanlawson

I looked into this and it seems to me that isHydrating is not necessary. And in fact it causes problems for me in pivots (#2564), so I need to get rid of it anyway (AFAICT).

Just one small nitpick and then this LGTM. @caridy

nolanlawson · 2022-08-02T23:20:08Z

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

+    let internals: ElementInternals | undefined;
+    try {
+        internals = element.attachInternals?.();
+    } catch {
+        // unregistered elements will throw, in which case we go with plan B
+        // to inspect the open shadowRoot on the element.
    }
-    return element.attachShadow(options);
+    return internals?.shadowRoot || element.shadowRoot || element.attachShadow(options);


I would suggest replacing this function with:

if (!isNull(element.shadowRoot)) { return element.shadowRoot; } return element.attachShadow(options);

The attachInternals() is not strictly related to this PR and can be handled in a follow-up PR.

I agree to do the internals on another PR.

caridy · 2022-08-03T12:59:46Z

closing in favor of #2975

caridy requested review from ekashida, nolanlawson and pmdartus April 26, 2022 23:50

caridy commented Apr 26, 2022

View reviewed changes

caridy commented Apr 28, 2022

View reviewed changes

packages/@lwc/engine-dom/src/renderer.ts Outdated Show resolved Hide resolved

pmdartus reviewed May 2, 2022

View reviewed changes

caridy marked this pull request as draft May 3, 2022 20:13

nolanlawson mentioned this pull request May 9, 2022

fix(engine-dom): refactor stylesheet API for consistency #2827

Merged

caridy added 2 commits May 12, 2022 15:59

refactor(engine): removing isHydrating global flag used across packages

6b0bdda

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

refactor(engine): more clean up

bb02d34

caridy force-pushed the caridy/remove-is-hydrating-global-flag branch from 5093d44 to bb02d34 Compare May 12, 2022 20:06

caridy commented May 24, 2022

View reviewed changes

fix(engine): elementInternals might not be available for unregistered CE

2446251

caridy marked this pull request as ready for review June 3, 2022 22:42

nolanlawson requested changes Aug 2, 2022

View reviewed changes

nolanlawson mentioned this pull request Aug 2, 2022

refactor(engine): remove isHydrating global flag #2975

Merged

caridy closed this Aug 3, 2022

ravijayaramappa deleted the caridy/remove-is-hydrating-global-flag branch December 13, 2022 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(engine): removing isHydrating global flag used across packages #2806

refactor(engine): removing isHydrating global flag used across packages #2806

caridy commented Apr 26, 2022

caridy Apr 26, 2022

nolanlawson Apr 27, 2022 •

edited

Loading

nolanlawson Apr 29, 2022

caridy Apr 26, 2022

nolanlawson Apr 27, 2022 •

edited

Loading

caridy Apr 28, 2022 •

edited

Loading

nolanlawson Apr 28, 2022

caridy Apr 28, 2022

nolanlawson Apr 28, 2022

caridy Apr 29, 2022

nolanlawson Apr 29, 2022

pmdartus May 2, 2022

pmdartus May 2, 2022

pmdartus May 2, 2022

caridy commented May 3, 2022 •

edited

Loading

nolanlawson commented May 11, 2022

caridy commented May 12, 2022

nolanlawson commented May 16, 2022

caridy May 24, 2022

caridy Jun 3, 2022

nolanlawson left a comment

nolanlawson Aug 2, 2022

caridy Aug 3, 2022

caridy commented Aug 3, 2022

refactor(engine): removing isHydrating global flag used across packages #2806

refactor(engine): removing isHydrating global flag used across packages #2806

Conversation

caridy commented Apr 26, 2022

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

Choose a reason for hiding this comment

nolanlawson Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

caridy Apr 28, 2022 • 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

caridy commented May 3, 2022 • edited Loading

nolanlawson commented May 11, 2022

caridy commented May 12, 2022

nolanlawson commented May 16, 2022

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

caridy commented Aug 3, 2022

nolanlawson Apr 27, 2022 •

edited

Loading

nolanlawson Apr 27, 2022 •

edited

Loading

caridy Apr 28, 2022 •

edited

Loading

caridy commented May 3, 2022 •

edited

Loading