fix(engine-dom): remove stylesheets when unused #2797

nolanlawson · 2022-04-19T21:57:29Z

Details

This PR causes <style>s and adoptedStyleSheets to be removed from the DOM when the style is no longer needed. This happens in two cases:

A render() function switches from one template to another
A component is removed from the DOM

Note that multiple components/templates may depend on the same <style>/adoptedStyleSheet (e.g. multiple instances of the same component), so we have to do some bookkeeping to check that no components/templates are still relying on the style.

Upsides of this PR

Consistent behavior across hydration / non-hydration / SSR
Consistent behavior across light and shadow
Styles do not infinitely accumulate in the <head> (potential memory leak)

Downsides of this PR

Potential breaking changes (see below)
Impossible to use static analysis to identify breakages
Extra code to do bookkeeping of style usage (+0.18KB min+gz)
There is a small runtime cost for this bookkeeping:

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.

Styles are unrendered when dynamically swapped out, rather than remaining in the DOM. For instance, if a component has a render() function, and it switches from template A to template B, here is the behavior:

Old behavior: B's styles are added on top of A's styles. A's styles still remain.
New behavior: A's styles are removed and replaced with B's styles.

SSR'ed/hydrated components are a bit different, because they use inline <style>s rather than shared style sheets.

GUS work item

W-9789883

nolanlawson · 2022-04-20T22:00:05Z

Two thoughts on this PR:

The code is not very DRY. We can probably combine the add/remove logic for stylesheets, and possibly we can combine the global vs local stylesheets into one utility to handle both.
Since this is an observable change that may cause breakages, it may need a feature flag. Depends on how serious we think CSS breakages may be, and how widespread the breakages may be.
I should write hydration / SSR tests for this, since it behaves differently for hydrated/SSR vs non-hydrated.

Fixes #2466

This reverts commit 1b83ab6.

nolanlawson · 2022-04-28T23:20:19Z

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

+
+function insertStyleElement(content: string, target: ShadowRoot | Document) {
+    const elm = createStyleElement(content);
+    const targetAnchorPoint = isDocument(target) ? target.head : target;


Previously we were doing:

document.head || document.body || document

But I don't see the point of that. If it's the document, it has a head. And if it has neither a head nor a body, the browser won't let you append directly to the document anyway.

nolanlawson · 2022-04-28T23:22:08Z

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

+        adoptedStyleSheets.push(styleSheet);
+    } else {
+        target.adoptedStyleSheets = [...adoptedStyleSheets, styleSheet];
+    }


Previously we were not using constructable stylesheets for the global document. The main reason was performance (there are a lot of stylesheets on the document compared to a single shadow root). But now that we have mutable adopted stylesheets shipping in Chrome 99, there shouldn't be a perf penalty for appending to the adoptedStyleSheets array.

vovius · 2022-04-28T23:45:11Z

/nucleus test

This reverts commit 0bfb8b6.

nolanlawson · 2022-04-29T16:39:33Z

As it turns out, this is a really difficult feature to implement. And it causes a more severe perf impact than I expected:

Considering the perf impact, the extra complexity, the potential for backwards compatibility breakages, and the potential for unexpected breakages if our bookkeeping is buggy (e.g. styles get unrendered when they shouldn't), it's debatable whether we should even implement this.

This reverts commit 1a026c3.

nolanlawson · 2022-04-29T17:56:57Z

packages/integration-karma/test/swapping/styles/index.spec.js

+        if (document.adoptedStyleSheets) {
+            document.adoptedStyleSheets = [];
+        }
+    });


It would be great to fix this, but it only happens in dev mode, and hot-swapping is not even used on platform.

nolanlawson · 2022-04-29T18:00:43Z

packages/integration-karma/test/shadow-dom/multiple-templates/index.spec.js

+                expect(getComputedStyle(element.shadowRoot.querySelector('.green')).color).toEqual(
+                    'rgb(0, 0, 0)'
+                );
+                expect(getComputedStyle(element).marginLeft).toEqual('0px');


This is a good example of the behavior becoming more consistent with this PR. This is very subtle, because what's actually happening here is that, in synthetic, the stylesheets are also not removed from the DOM. But the reason the color is "correct" in synthetic mode is that the scope attributes are not applied to the DOM nodes (due to the stylesheets array being empty).

In any case, this PR makes the behavior consistent.

nolanlawson · 2022-04-29T18:02:12Z

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

        }
-    } else if (ssr || isHydrating()) {
+    } else if (ssr || vm.hydrated) {


It turns out that what we care about is if the vm was hydrated in the first place, not whether it's currently hydrating.

nolanlawson · 2022-04-29T18:26:48Z

packages/integration-karma/helpers/test-setup.js

-            throw new Error('Unexpected element left in the <head> by a test: ' + child.outerHTML);
-        }
-    });
+    var bodyChildren = getBodyChildren();


We actually don't want the test to clear the <head> after every test, because that might hide bugs in the stylesheet count bookkeeping. Changing this code helped me find some bugs in how I was handling hydration, as well as the hot-swapping issue.

nolanlawson · 2022-04-29T22:07:00Z

I can't believe I got this working, so I guess this PR is ready for review now.

This should probably have a feature flag, but we need to fix #2811 first.

pmdartus · 2022-05-02T08:43:09Z

Let me play the devil's advocate and challenge the necessity of this change.

This change doesn't impact the native shadow DOM components. For native shadow DOM components, styles are either injected in the shadow tree via a <style> element or via ShadowRoot.prototype.adoptedStylesheets. Upon component removal, the styles are automatically cleaned up.

This leaves us with synthetic shadow DOM components and light DOM components. We all agree that synthetic shadow DOM isn't something we want to deprecate. So I am not much worried about the impact of leaving the stylesheet in place for synthetic shadow DOM components.

IMO to evaluate the necessity for this change we should answer the following question: "Should stylesheets injected by light DOM components be cleanup upon removal?". Note that this should only impact light DOM components that are injected inside the main document tree. Light DOM components injected within a shadow tree (light DOM components wrapped within shadow DOM component) aren't impacted as the stylesheet will be injected in the closest ancestor shadow root.

Going back to the original issue #2466 (comment), @nolanlawson you added an interesting comment:

It's worth noting that not every CSS-in-JS library or framework actually does this. So it might not be a big deal in practice.

While I understand why you are referring to the CSS-in-JS, I think a better comparison would be with Svelte and Vue component styling. Those frameworks have built-in styling support which is more relatable to LWC light DOM component styling. Upon inspection, none of those frameworks do clean up injected styles after unmounting a component tree. I haven't heard either strong feedback from developers using those frameworks complaining about the lack of style clean-up capability.

If the main concern here is the potential style clashing cross light DOM components, my answer would be to prefer scoped styles over global styles for light DOM components.

As a side note, there are a couple of interesting refactors in this PR, that we might want to land regardless of if we roll out style clean up or not.

nolanlawson · 2022-05-02T17:59:30Z

@pmdartus

This change doesn't impact the native shadow DOM components.

The most important case this PR solves is multi-template components, which does affect native shadow DOM. If a component switches from template A to template B, and then back to template A, then B's styles will still win because they are listed last in the DOM.

What prompted me to write this PR is someone pointing out that our own documentation is incorrect on this (#2793 (comment)). Probably this worked at some point in the past (when native shadow injected inline <style>s), but it was broken when we introduced Constructable Stylesheets. It never worked in synthetic shadow AFAIK.

Light DOM components injected within a shadow tree (light DOM components wrapped within shadow DOM component) aren't impacted as the stylesheet will be injected in the closest ancestor shadow root

It does affect light DOM components inside of shadow roots. If the shadow root is never removed from the DOM, then the styles are never cleaned up. So this may happen if a large omnipresent container component is never removed. Any light DOM components inside of that shadow root will just continually append style sheets and never remove them.

If the main concern here is the potential style clashing cross light DOM components, my answer would be to prefer scoped styles over global styles for light DOM components.

Scoped styles actually have exactly the same problem with multi-template components. Scoped styles use the same scoping token for all templates owned by a given component. So if that component switches templates, the two style sheets will still conflict with each other.

I haven't heard either strong feedback from developers using those frameworks complaining about the lack of style clean-up capability.

I didn't find anything for Vue, but I did find some discussion in Svelte and certain CSS-in-JS libraries about this. It seems some developers would like this functionality:

For LWC, I think an argument could be made that we should only remove styles for multi-template components. This case feels clearly "wrong" to me, whereas removing styles on component unmount feels a bit like an implementation detail, or a memory leak optimization. (E.g. for dynamically-created components, we don't want to keep adding <style> tags to the <head>. Which may or may not be a huge memory leak in practice.)

This PR could be refactored to only handle multi-template components. But I haven't thought much about how we would implement it, or how we would handle corner cases. (E.g. simply reordering the style sheets is not sufficient for the case where template A has no styles, but template B does.)

nolanlawson · 2022-05-03T18:03:42Z

@pmdartus OK, after Dev Sync, I realized I was wrong about two things:

This does work correctly in synthetic shadow, because the scope token is different for template A vs template B.
This does work correctly in scoped styles, for the same reason.

So maybe our message to developers should be: "If you want multi-template components to work as expected when switching back and forth, then use scoped styles." Ditto for light DOM components, as you said.

nolanlawson · 2022-05-09T21:37:23Z

Closing in favor of #2827

nolanlawson requested review from pmdartus and jodarove April 19, 2022 23:12

nolanlawson force-pushed the nolan/unrender-stylesheets branch from 4aab717 to c354b27 Compare April 26, 2022 23:49

nolanlawson mentioned this pull request Apr 27, 2022

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

Closed

nolanlawson added 19 commits April 28, 2022 09:15

fix(engine-dom): remove stylesheets when unused

0e3058a

Fixes #2466

fix: remove unnecessary change

d276c7e

test: add more tests

944a55d

test: add hydration test, fix unrendering styles

d6bcd59

test: more tests

88f7edd

test: re-enable tests

49953e7

refactor: refactor

7f9afd9

refactor: refactor

8b5feab

test: fix test in ie11

14b97f8

test: fix ie11 again

d2561cc

fix: headers

a442aa5

Revert "test: fix ie11 again"

6bbd4af

This reverts commit 1b83ab6.

test: actually fix ie11, rename api to avoid ambiguity

435b290

fix: add comment

b5fa193

refactor: refactor part one

4a012c1

refactor: refactor part 2

29424ff

refactor: refactor part 3

53f68e9

refactor: refactor part 4

d69c65c

fix: fix comment

79ecf74

nolanlawson force-pushed the nolan/unrender-stylesheets branch from 5573b54 to 79ecf74 Compare April 28, 2022 17:36

nolanlawson added 5 commits April 28, 2022 10:37

fix: fix comment

6e1e70b

fix: work around ie11 bug

8d918b3

refactor: refactor

dab7e56

fix: fix for race condition

5a5e97b

fix: another undefined case

f6479ea

nolanlawson commented Apr 28, 2022

View reviewed changes

nolanlawson added 3 commits April 28, 2022 16:52

fix: remove flag for now

0bfb8b6

test: remove <head> cleanup from tests

4882534

Revert "fix: remove flag for now"

1a026c3

This reverts commit 0bfb8b6.

nolanlawson added 3 commits April 29, 2022 10:37

fix: try to fix hydration and style removal

25f005f

Revert "Revert "fix: remove flag for now""

e78ad66

This reverts commit 1a026c3.

fix: remove unnecessary test util

52a5309

nolanlawson commented Apr 29, 2022

View reviewed changes

nolanlawson added 3 commits April 29, 2022 11:05

refactor: make code DRYer

eb677d4

refactor: refactor test

f6ad82f

refactor: refactor test

88bbb3c

nolanlawson commented Apr 29, 2022

View reviewed changes

chore: empty commit to poke CI

dbb1870

nolanlawson marked this pull request as ready for review April 29, 2022 22:07

This was referenced May 3, 2022

Switching between multiple templates leaks css #2793

Closed

Stylesheets should be removed when components/templates are disconnected #2466

Closed

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

Merged

nolanlawson closed this May 9, 2022

nolanlawson deleted the nolan/unrender-stylesheets branch July 25, 2022 16:20

nolanlawson mentioned this pull request Sep 27, 2022

Programmatic style injection salesforce/lwc-rfcs#56

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(engine-dom): remove stylesheets when unused #2797

fix(engine-dom): remove stylesheets when unused #2797

nolanlawson commented Apr 19, 2022 •

edited

Loading

nolanlawson commented Apr 20, 2022 •

edited

Loading

nolanlawson Apr 28, 2022

nolanlawson Apr 28, 2022

vovius commented Apr 28, 2022

nolanlawson commented Apr 29, 2022

nolanlawson Apr 29, 2022

nolanlawson Apr 29, 2022

nolanlawson Apr 29, 2022

nolanlawson Apr 29, 2022

nolanlawson commented Apr 29, 2022

pmdartus commented May 2, 2022

nolanlawson commented May 2, 2022

nolanlawson commented May 3, 2022

nolanlawson commented May 9, 2022

fix(engine-dom): remove stylesheets when unused #2797

fix(engine-dom): remove stylesheets when unused #2797

Conversation

nolanlawson commented Apr 19, 2022 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

nolanlawson commented Apr 20, 2022 • edited Loading

nolanlawson Apr 28, 2022

Choose a reason for hiding this comment

nolanlawson Apr 28, 2022

Choose a reason for hiding this comment

vovius commented Apr 28, 2022

nolanlawson commented Apr 29, 2022

nolanlawson Apr 29, 2022

Choose a reason for hiding this comment

nolanlawson Apr 29, 2022

Choose a reason for hiding this comment

nolanlawson Apr 29, 2022

Choose a reason for hiding this comment

nolanlawson Apr 29, 2022

Choose a reason for hiding this comment

nolanlawson commented Apr 29, 2022

pmdartus commented May 2, 2022

nolanlawson commented May 2, 2022

nolanlawson commented May 3, 2022

nolanlawson commented May 9, 2022

nolanlawson commented Apr 19, 2022 •

edited

Loading

nolanlawson commented Apr 20, 2022 •

edited

Loading