fix(engine-core): unrender stylesheets in HMR #4156
Conversation
| content: string, | ||
| target: ShadowRoot | undefined, | ||
| signal: AbortSignal | undefined | ||
| ) => void; |
There was a problem hiding this comment.
I decided to use an AbortController because I think it's a nice way to have a clean separation between engine-core and engine-dom. engine-core only ever needs to deal with the AbortController, and engine-dom only ever needs to deal with the AbortSignal. engine-core is responsible for triggering the abort, and engine-dom just needs to respond to the abort event.
In the future, you could imagine abstracting this in such a way that each AbortController is tied to an individual StylesheetFactory rather than just a string; that might help with implementing #4155.
nolanlawson
force-pushed
the
nolan/swap-styles-squashed
branch
from
284027c
to
840999a
Compare
| let activeTemplates: WeakMultiMap<Template, VM> = /*@__PURE__@*/ new WeakMultiMap(); | ||
| let activeComponents: WeakMultiMap<LightningElementConstructor, VM> = | ||
| /*@__PURE__@*/ new WeakMultiMap(); | ||
| let activeStyles: WeakMultiMap<StylesheetFactory, VM> = /*@__PURE__@*/ new WeakMultiMap(); |
There was a problem hiding this comment.
Nothing really changed here; I just added PURE annotations because I'm paranoid. I don't think this actually makes a difference, but I prefer to be safe in case bundlers have different heuristics (Rollup vs Webpack vs ESBuild, etc.).
| activeComponents = new WeakMultiMap(); | ||
| activeStyles = new WeakMultiMap(); | ||
| }; | ||
| } |
There was a problem hiding this comment.
The previous tests were not extensive enough to hit cases where you actually need to reset this state properly between tests.
Details
Fixes #4115
This PR makes
swapStyle()unrender whatever style is being swapped out. This fixes the issue where stylesheets are only ever appended, never replaced.The basic invariants of this PR are:
Invariant 1: Do not change how production code works
I don't see a strong reason to change the current production system for rendering stylesheets, since it's heavily optimized. (E.g. we avoid re-rendering the same stylesheet twice, and we prefer constructable stylesheets over
<style>s.)Invariant 2: Avoid creating a difference between production and development, when HMR is not in play
It would be bad if a user tested their component in dev mode and found that it behaved differently in prod mode. This is especially important for stylesheets, which can have quirks due to the insertion ordering or deduplication in case of collisions. (We tell developers not to rely on these quirks, but c'mon, they still do.)
So I avoided making any changes that would cause differences between dev and prod, except when HMR is in play. When HMR is in play, I think it's a little more acceptable for there to be subtle differences. At the end of the day, a developer should refresh the page to see the "real" behavior and not fully trust the HMR'd behavior.
Design decisions
These invariants led to some design decisions:
<head>), then unrendering one causes the other to be unrendered. This is tracked in [HMR]swapStyle()API does not account for stylesheet collisions #4155.swap*APIs show stale content when swapping back and forth multiple times #4154.I think these issues are valid, but I wanted to start with a simple PR first.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
Not in prod anyway. And not even in dev mode, when HMR is not in play.
GUS work item
W-15347398