fix: simplify and robustify appending styles (#13303)

#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.
pull/13311/head
Simon H 3 months ago committed by GitHub
parent f852a6890b
commit d338e8083f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: simplify and robustify appending styles

@ -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)]))); state.hoisted.push(b.const('$$css', b.object([b.init('hash', hash), b.init('code', code)])));
component_block.body.unshift( component_block.body.unshift(
b.stmt( b.stmt(b.call('$.append_styles', b.id('$$anchor'), b.id('$$css')))
b.call('$.append_styles', b.id('$$anchor'), b.id('$$css'), options.customElement && b.true)
)
); );
} }

@ -2,25 +2,11 @@ import { DEV } from 'esm-env';
import { queue_micro_task } from './task.js'; import { queue_micro_task } from './task.js';
import { register_style } from '../dev/css.js'; import { register_style } from '../dev/css.js';
var roots = new WeakMap();
/** /**
* @param {Node} anchor * @param {Node} anchor
* @param {{ hash: string, code: string }} css * @param {{ hash: string, code: string }} css
* @param {boolean} [is_custom_element]
*/ */
export function append_styles(anchor, css, is_custom_element) { export function append_styles(anchor, css) {
// 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);
}
// Use `queue_micro_task` to ensure `anchor` is in the DOM, otherwise getRootNode() will yield wrong results // Use `queue_micro_task` to ensure `anchor` is in the DOM, otherwise getRootNode() will yield wrong results
queue_micro_task(() => { queue_micro_task(() => {
var root = anchor.getRootNode(); var root = anchor.getRootNode();
@ -29,6 +15,8 @@ export function append_styles(anchor, css, is_custom_element) {
? /** @type {ShadowRoot} */ (root) ? /** @type {ShadowRoot} */ (root)
: /** @type {Document} */ (root).head ?? /** @type {Document} */ (root.ownerDocument).head; : /** @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)) { if (!target.querySelector('#' + css.hash)) {
const style = document.createElement('style'); const style = document.createElement('style');
style.id = css.hash; style.id = css.hash;

@ -1,10 +1,13 @@
<svelte:options css="injected" /> <svelte:options css="injected" />
<script> <script>
import GrandChild from "./GrandChild.svelte";
let { count } = $props(); let { count } = $props();
</script> </script>
<h1>count: {count}</h1> <h1>count: {count}</h1>
<GrandChild {count} />
<style> <style>
h1 { h1 {

@ -0,0 +1,9 @@
<svelte:options css="injected" />
<h1>inner</h1>
<style>
h1 {
color: blue;
}
</style>

@ -5,18 +5,20 @@ export default test({
async test({ target, assert }) { async test({ target, assert }) {
const button = target.querySelector('button'); const button = target.querySelector('button');
const h1 = () => const h1 = () =>
/** @type {HTMLHeadingElement} */ ( /** @type {NodeListOf<HTMLHeadingElement>} */ (
/** @type {Window} */ ( /** @type {Window} */ (
target.querySelector('iframe')?.contentWindow target.querySelector('iframe')?.contentWindow
).document.querySelector('h1') ).document.querySelectorAll('h1')
); );
assert.equal(h1().textContent, 'count: 0'); assert.equal(h1()[0].textContent, 'count: 0');
assert.equal(getComputedStyle(h1()).color, 'rgb(255, 0, 0)'); assert.equal(getComputedStyle(h1()[0]).color, 'rgb(255, 0, 0)');
assert.equal(getComputedStyle(h1()[1]).color, 'rgb(0, 0, 255)');
flushSync(() => button?.click()); flushSync(() => button?.click());
assert.equal(h1().textContent, 'count: 1'); assert.equal(h1()[0].textContent, 'count: 1');
assert.equal(getComputedStyle(h1()).color, 'rgb(255, 0, 0)'); assert.equal(getComputedStyle(h1()[0]).color, 'rgb(255, 0, 0)');
assert.equal(getComputedStyle(h1()[1]).color, 'rgb(0, 0, 255)');
} }
}); });

Loading…
Cancel
Save