Skip to content

Commit

Permalink
fix: simplify and robustify appending styles (#13303)
Browse files Browse the repository at this point in the history
#13225 did not fully fix the described issue: As soon as you have a child component, that child component's anchor will not be in the right ownerDocument yet, and so the logic falls down.

To fix that, we would need to move the ownerDocument check into the microtask - and that map check was mainly done to avoid just that. So instead I opted to simplify the code and just remove the map checks. According to my benchmarking (https://jsbench.me/3hm17l0bxl/1) this has no impact on performance assuming that you'll have cache hits roughly half the time - and even if you'd have much more cache hits, the performance loss is microscopic. Given that the default mode is to not inject styles, this will not be common anyway, so I favor removing that map.
  • Loading branch information
dummdidumm committed Sep 18, 2024
1 parent f852a68 commit d338e80
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 24 deletions.
5 changes: 5 additions & 0 deletions .changeset/silent-elephants-film.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: simplify and robustify appending styles
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,7 @@ export function client_component(analysis, options) {
state.hoisted.push(b.const('$$css', b.object([b.init('hash', hash), b.init('code', code)])));

component_block.body.unshift(
b.stmt(
b.call('$.append_styles', b.id('$$anchor'), b.id('$$css'), options.customElement && b.true)
)
b.stmt(b.call('$.append_styles', b.id('$$anchor'), b.id('$$css')))
);
}

Expand Down
18 changes: 3 additions & 15 deletions packages/svelte/src/internal/client/dom/css.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,11 @@ import { DEV } from 'esm-env';
import { queue_micro_task } from './task.js';
import { register_style } from '../dev/css.js';

var roots = new WeakMap();

/**
* @param {Node} anchor
* @param {{ hash: string, code: string }} css
* @param {boolean} [is_custom_element]
*/
export function append_styles(anchor, css, is_custom_element) {
// in dev, always check the DOM, so that styles can be replaced with HMR
if (!DEV && !is_custom_element) {
var doc = /** @type {Document} */ (anchor.ownerDocument);

if (!roots.has(doc)) roots.set(doc, new Set());
const seen = roots.get(doc);

if (seen.has(css)) return;
seen.add(css);
}

export function append_styles(anchor, css) {
// Use `queue_micro_task` to ensure `anchor` is in the DOM, otherwise getRootNode() will yield wrong results
queue_micro_task(() => {
var root = anchor.getRootNode();
Expand All @@ -29,6 +15,8 @@ export function append_styles(anchor, css, is_custom_element) {
? /** @type {ShadowRoot} */ (root)
: /** @type {Document} */ (root).head ?? /** @type {Document} */ (root.ownerDocument).head;

// Always querying the DOM is roughly the same perf as additionally checking for presence in a map first assuming
// that you'll get cache hits half of the time, so we just always query the dom for simplicity and code savings.
if (!target.querySelector('#' + css.hash)) {
const style = document.createElement('style');
style.id = css.hash;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
<svelte:options css="injected" />

<script>
import GrandChild from "./GrandChild.svelte";
let { count } = $props();
</script>

<h1>count: {count}</h1>
<GrandChild {count} />

<style>
h1 {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<svelte:options css="injected" />

<h1>inner</h1>

<style>
h1 {
color: blue;
}
</style>
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,20 @@ export default test({
async test({ target, assert }) {
const button = target.querySelector('button');
const h1 = () =>
/** @type {HTMLHeadingElement} */ (
/** @type {NodeListOf<HTMLHeadingElement>} */ (
/** @type {Window} */ (
target.querySelector('iframe')?.contentWindow
).document.querySelector('h1')
).document.querySelectorAll('h1')
);

assert.equal(h1().textContent, 'count: 0');
assert.equal(getComputedStyle(h1()).color, 'rgb(255, 0, 0)');
assert.equal(h1()[0].textContent, 'count: 0');
assert.equal(getComputedStyle(h1()[0]).color, 'rgb(255, 0, 0)');
assert.equal(getComputedStyle(h1()[1]).color, 'rgb(0, 0, 255)');

flushSync(() => button?.click());

assert.equal(h1().textContent, 'count: 1');
assert.equal(getComputedStyle(h1()).color, 'rgb(255, 0, 0)');
assert.equal(h1()[0].textContent, 'count: 1');
assert.equal(getComputedStyle(h1()[0]).color, 'rgb(255, 0, 0)');
assert.equal(getComputedStyle(h1()[1]).color, 'rgb(0, 0, 255)');
}
});

0 comments on commit d338e80

Please sign in to comment.