From 015b7ae7d76acd11d9b0a5e004bc6129f0bc749f Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Wed, 28 Jun 2023 12:28:05 +0200 Subject: [PATCH 1/5] fix: check srcset when hydrating to prevent needless requests fixes #8838 --- .changeset/six-teachers-divide.md | 5 ++++ .../render_dom/wrappers/Element/Attribute.js | 13 +++++++++- packages/svelte/src/runtime/internal/utils.js | 26 ++++++++++++++++++- 3 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 .changeset/six-teachers-divide.md diff --git a/.changeset/six-teachers-divide.md b/.changeset/six-teachers-divide.md new file mode 100644 index 000000000000..a11f257fefe9 --- /dev/null +++ b/.changeset/six-teachers-divide.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: check srcset when hydrating to prevent needless requests diff --git a/packages/svelte/src/compiler/compile/render_dom/wrappers/Element/Attribute.js b/packages/svelte/src/compiler/compile/render_dom/wrappers/Element/Attribute.js index d64626137725..0459b55b0357 100644 --- a/packages/svelte/src/compiler/compile/render_dom/wrappers/Element/Attribute.js +++ b/packages/svelte/src/compiler/compile/render_dom/wrappers/Element/Attribute.js @@ -64,6 +64,9 @@ export default class AttributeWrapper extends BaseAttributeWrapper { /** @type {boolean} */ is_src; + /** @type {boolean} */ + is_srcset; + /** @type {boolean} */ is_select_value_attribute; @@ -120,6 +123,9 @@ export default class AttributeWrapper extends BaseAttributeWrapper { this.is_src = this.name === 'src' && (!this.parent.node.namespace || this.parent.node.namespace === namespaces.html); + this.is_srcset = + this.name === 'srcset' && + (!this.parent.node.namespace || this.parent.node.namespace === namespaces.html); this.should_cache = should_cache(this); } @@ -164,6 +170,11 @@ export default class AttributeWrapper extends BaseAttributeWrapper { b`if (!@src_url_equal(${element.var}.src, ${init})) ${method}(${element.var}, "${name}", ${this.last});` ); updater = b`${method}(${element.var}, "${name}", ${should_cache ? this.last : value});`; + } else if (this.is_srcset) { + block.chunks.hydrate.push( + b`if (!@srcset_url_equal(${element.var}, ${init})) ${method}(${element.var}, "${name}", ${this.last});` + ); + updater = b`${method}(${element.var}, "${name}", ${should_cache ? this.last : value});`; } else if (property_name) { block.chunks.hydrate.push(b`${element.var}.${property_name} = ${init};`); updater = block.renderer.options.dev @@ -403,7 +414,7 @@ Object.keys(attribute_lookup).forEach((name) => { /** @param {AttributeWrapper} attribute */ function should_cache(attribute) { - return attribute.is_src || attribute.node.should_cache(); + return attribute.is_src || attribute.is_srcset || attribute.node.should_cache(); } const regex_contains_checked_or_group = /checked|group/; diff --git a/packages/svelte/src/runtime/internal/utils.js b/packages/svelte/src/runtime/internal/utils.js index df97d49598ab..846328bf05fb 100644 --- a/packages/svelte/src/runtime/internal/utils.js +++ b/packages/svelte/src/runtime/internal/utils.js @@ -68,7 +68,11 @@ export function safe_not_equal(a, b) { let src_url_equal_anchor; -/** @returns {boolean} */ +/** + * @param {string} element_src + * @param {string} url + * @returns {boolean} + */ export function src_url_equal(element_src, url) { if (!src_url_equal_anchor) { src_url_equal_anchor = document.createElement('a'); @@ -77,6 +81,26 @@ export function src_url_equal(element_src, url) { return element_src === src_url_equal_anchor.href; } +/** + * @param {HTMLSourceElement | HTMLImageElement} element_srcset + * @param {string} srcset + * @returns {boolean} + */ +export function srcset_url_equal(element_srcset, srcset) { + /** @param {string} _srcset */ + function split(_srcset) { + return _srcset.split(',').map((src) => src.trim().split(' ')[0]); + } + + const element_urls = split(element_srcset.srcset); + const urls = split(srcset); + + return ( + urls.length === element_urls.length && + urls.every((url, i) => src_url_equal(element_urls[i], url)) + ); +} + /** @returns {boolean} */ export function not_equal(a, b) { return a != a ? b == b : a !== b; From 5b4356c9b3fc2a99fed50268d02d1f13192d7b0a Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 29 Jun 2023 11:35:08 +0200 Subject: [PATCH 2/5] this should do it --- packages/svelte/src/runtime/internal/utils.js | 13 ++++- packages/svelte/test/utils/utils.test.js | 51 ++++++++++++++++++- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/packages/svelte/src/runtime/internal/utils.js b/packages/svelte/src/runtime/internal/utils.js index 846328bf05fb..2ca4b831f6ac 100644 --- a/packages/svelte/src/runtime/internal/utils.js +++ b/packages/svelte/src/runtime/internal/utils.js @@ -74,9 +74,11 @@ let src_url_equal_anchor; * @returns {boolean} */ export function src_url_equal(element_src, url) { + if (element_src === url) return true; if (!src_url_equal_anchor) { src_url_equal_anchor = document.createElement('a'); } + // This is actually faster than doing URL(..).href src_url_equal_anchor.href = url; return element_src === src_url_equal_anchor.href; } @@ -89,7 +91,7 @@ export function src_url_equal(element_src, url) { export function srcset_url_equal(element_srcset, srcset) { /** @param {string} _srcset */ function split(_srcset) { - return _srcset.split(',').map((src) => src.trim().split(' ')[0]); + return _srcset.split(',').map((src) => src.trim().split(' ').filter(Boolean)); } const element_urls = split(element_srcset.srcset); @@ -97,7 +99,14 @@ export function srcset_url_equal(element_srcset, srcset) { return ( urls.length === element_urls.length && - urls.every((url, i) => src_url_equal(element_urls[i], url)) + urls.every( + ([url, width], i) => + width === element_urls[i][1] && + // We need to test both ways because Vite/imagetools (but not other tools) will create an absolute URL + // for the client, and the relative URLs inside srcset are not automatically resolved to absolute URLs + // by browsers (in contrast to img.src). This means both SSR and DOM code could contain relative or absolute URLs. + (src_url_equal(element_urls[i][0], url) || src_url_equal(url, element_urls[i][0])) + ) ); } diff --git a/packages/svelte/test/utils/utils.test.js b/packages/svelte/test/utils/utils.test.js index cbdba7db320a..31aab3320fc9 100644 --- a/packages/svelte/test/utils/utils.test.js +++ b/packages/svelte/test/utils/utils.test.js @@ -1,4 +1,4 @@ -import { assert, describe, it } from 'vitest'; +import { afterAll, assert, beforeAll, describe, it } from 'vitest'; import '../../src/compiler/compile/nodes/Slot.js'; // this needs to come first to force ESM to load things in a specific order to prevent circular dependency errors import { CONTENTEDITABLE_BINDINGS, @@ -9,7 +9,7 @@ import { } from '../../src/compiler/compile/utils/contenteditable.js'; import get_name_from_filename from '../../src/compiler/compile/utils/get_name_from_filename.js'; import { trim_end, trim_start } from '../../src/compiler/utils/trim.js'; -import { split_css_unit } from '../../src/runtime/internal/utils.js'; +import { split_css_unit, srcset_url_equal } from '../../src/runtime/internal/utils.js'; describe('utils', () => { describe('trim', () => { @@ -116,4 +116,51 @@ describe('utils', () => { }); }); }); + + describe('srcset_url_equal', () => { + function create_element(srcset) { + return /** @type {HTMLImageElement} */ ({ + srcset + }); + } + + beforeAll(() => { + let host = 'https://svelte.dev'; + let _href = ''; + // @ts-ignore + global.document = { + createElement: () => + /** @type {any} */ ({ + get href() { + return _href; + }, + set href(value) { + _href = host + value; + } + }) + }; + }); + + afterAll(() => { + // @ts-ignore + delete global.document; + }); + + it('should return true if urls are equal', () => { + assert.ok(srcset_url_equal(create_element('a'), 'a')); + assert.ok(srcset_url_equal(create_element('a 1x'), 'a 1x')); + assert.ok(srcset_url_equal(create_element('a 1x, b 2x'), 'a 1x, b 2x')); + assert.ok(srcset_url_equal(create_element('a 1x, b 2x'), 'a 1x, b 2x')); + }); + + it('should return true if urls are equal (abs/rel URLs)', () => { + assert.ok(srcset_url_equal(create_element('https://svelte.dev/a'), '/a')); + assert.ok(srcset_url_equal(create_element('/a'), 'https://svelte.dev/a')); + }); + + it('should return false if urls are different', () => { + assert.notOk(srcset_url_equal(create_element('a 1x'), 'b 1x')); + assert.notOk(srcset_url_equal(create_element('a 2x'), 'a 1x')); + }); + }); }); From a572dcf8687005bea3480a3777a7fe7cb287a7ee Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Thu, 29 Jun 2023 10:35:01 -0700 Subject: [PATCH 3/5] use `const` --- packages/svelte/test/utils/utils.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/test/utils/utils.test.js b/packages/svelte/test/utils/utils.test.js index 31aab3320fc9..946f2d4bfcbf 100644 --- a/packages/svelte/test/utils/utils.test.js +++ b/packages/svelte/test/utils/utils.test.js @@ -125,7 +125,7 @@ describe('utils', () => { } beforeAll(() => { - let host = 'https://svelte.dev'; + const host = 'https://svelte.dev'; let _href = ''; // @ts-ignore global.document = { From 7a42ba534c6f6f37132d21af254f33a4c2c42ac4 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Fri, 30 Jun 2023 10:22:00 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com> --- packages/svelte/src/runtime/internal/utils.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/svelte/src/runtime/internal/utils.js b/packages/svelte/src/runtime/internal/utils.js index 2ca4b831f6ac..a8c6a6cffd52 100644 --- a/packages/svelte/src/runtime/internal/utils.js +++ b/packages/svelte/src/runtime/internal/utils.js @@ -90,10 +90,9 @@ export function src_url_equal(element_src, url) { */ export function srcset_url_equal(element_srcset, srcset) { /** @param {string} _srcset */ - function split(_srcset) { + const split = (_srcset) => { return _srcset.split(',').map((src) => src.trim().split(' ').filter(Boolean)); - } - + }; const element_urls = split(element_srcset.srcset); const urls = split(srcset); @@ -102,9 +101,11 @@ export function srcset_url_equal(element_srcset, srcset) { urls.every( ([url, width], i) => width === element_urls[i][1] && - // We need to test both ways because Vite/imagetools (but not other tools) will create an absolute URL - // for the client, and the relative URLs inside srcset are not automatically resolved to absolute URLs - // by browsers (in contrast to img.src). This means both SSR and DOM code could contain relative or absolute URLs. + // We need to test both ways because Vite will create an a full URL with + // `new URL(asset, import.meta.url).href` for the client when `base: './'`, and the + // relative URLs inside srcset are not automatically resolved to absolute URLs by + // browsers (in contrast to img.src). This means both SSR and DOM code could + // contain relative or absolute URLs. (src_url_equal(element_urls[i][0], url) || src_url_equal(url, element_urls[i][0])) ) ); From e9a1dd235a886d5d9f681475827d9c59e89a1568 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Fri, 30 Jun 2023 10:23:36 +0200 Subject: [PATCH 5/5] cleanup --- packages/svelte/src/runtime/internal/utils.js | 13 +++++++------ packages/svelte/test/utils/utils.test.js | 11 ++++++----- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/svelte/src/runtime/internal/utils.js b/packages/svelte/src/runtime/internal/utils.js index a8c6a6cffd52..190c4534c626 100644 --- a/packages/svelte/src/runtime/internal/utils.js +++ b/packages/svelte/src/runtime/internal/utils.js @@ -83,18 +83,19 @@ export function src_url_equal(element_src, url) { return element_src === src_url_equal_anchor.href; } +/** @param {string} srcset */ +function split_srcset(srcset) { + return srcset.split(',').map((src) => src.trim().split(' ').filter(Boolean)); +} + /** * @param {HTMLSourceElement | HTMLImageElement} element_srcset * @param {string} srcset * @returns {boolean} */ export function srcset_url_equal(element_srcset, srcset) { - /** @param {string} _srcset */ - const split = (_srcset) => { - return _srcset.split(',').map((src) => src.trim().split(' ').filter(Boolean)); - }; - const element_urls = split(element_srcset.srcset); - const urls = split(srcset); + const element_urls = split_srcset(element_srcset.srcset); + const urls = split_srcset(srcset); return ( urls.length === element_urls.length && diff --git a/packages/svelte/test/utils/utils.test.js b/packages/svelte/test/utils/utils.test.js index 946f2d4bfcbf..301361ea3d56 100644 --- a/packages/svelte/test/utils/utils.test.js +++ b/packages/svelte/test/utils/utils.test.js @@ -124,11 +124,13 @@ describe('utils', () => { }); } + let old_document; + beforeAll(() => { const host = 'https://svelte.dev'; let _href = ''; - // @ts-ignore - global.document = { + old_document = global.document; + global.document = /** @type {any} */ ({ createElement: () => /** @type {any} */ ({ get href() { @@ -138,12 +140,11 @@ describe('utils', () => { _href = host + value; } }) - }; + }); }); afterAll(() => { - // @ts-ignore - delete global.document; + global.document = old_document; }); it('should return true if urls are equal', () => {