From d338e8083f13aa04c5197468eaf695f726de8696 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Wed, 18 Sep 2024 20:11:52 +0200 Subject: [PATCH] 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. --- .changeset/silent-elephants-film.md | 5 +++++ .../3-transform/client/transform-client.js | 4 +--- packages/svelte/src/internal/client/dom/css.js | 18 +++--------------- .../samples/mount-in-iframe/Child.svelte | 3 +++ .../samples/mount-in-iframe/GrandChild.svelte | 9 +++++++++ .../samples/mount-in-iframe/_config.js | 14 ++++++++------ 6 files changed, 29 insertions(+), 24 deletions(-) create mode 100644 .changeset/silent-elephants-film.md create mode 100644 packages/svelte/tests/runtime-browser/samples/mount-in-iframe/GrandChild.svelte diff --git a/.changeset/silent-elephants-film.md b/.changeset/silent-elephants-film.md new file mode 100644 index 0000000000..1182b687f5 --- /dev/null +++ b/.changeset/silent-elephants-film.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: simplify and robustify appending styles diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index 45e48e565e..04815dbc28 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js @@ -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'))) ); } diff --git a/packages/svelte/src/internal/client/dom/css.js b/packages/svelte/src/internal/client/dom/css.js index 2c7efe8994..52be36aa1f 100644 --- a/packages/svelte/src/internal/client/dom/css.js +++ b/packages/svelte/src/internal/client/dom/css.js @@ -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(); @@ -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; diff --git a/packages/svelte/tests/runtime-browser/samples/mount-in-iframe/Child.svelte b/packages/svelte/tests/runtime-browser/samples/mount-in-iframe/Child.svelte index dac93751b8..9f2fc953cf 100644 --- a/packages/svelte/tests/runtime-browser/samples/mount-in-iframe/Child.svelte +++ b/packages/svelte/tests/runtime-browser/samples/mount-in-iframe/Child.svelte @@ -1,10 +1,13 @@

count: {count}

+ diff --git a/packages/svelte/tests/runtime-browser/samples/mount-in-iframe/_config.js b/packages/svelte/tests/runtime-browser/samples/mount-in-iframe/_config.js index 0f04e9659a..efc8db5870 100644 --- a/packages/svelte/tests/runtime-browser/samples/mount-in-iframe/_config.js +++ b/packages/svelte/tests/runtime-browser/samples/mount-in-iframe/_config.js @@ -5,18 +5,20 @@ export default test({ async test({ target, assert }) { const button = target.querySelector('button'); const h1 = () => - /** @type {HTMLHeadingElement} */ ( + /** @type {NodeListOf} */ ( /** @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)'); } });