Skip to content

chore: optimise attributes #10916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 25, 2024
Merged

chore: optimise attributes #10916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49595b4
avoid getAttribute outside hydration
Rich-Harris Mar 25, 2024
eb08c68
Merge branch 'main' into optimise-attributes
Rich-Harris Mar 25, 2024
650db08
tidy up
Rich-Harris Mar 25, 2024
c984385
simplify
Rich-Harris Mar 25, 2024
df7071c
dom -> element
Rich-Harris Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 42 additions & 47 deletions packages/svelte/src/internal/client/dom/elements/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,34 +31,37 @@ export function attr_effect(dom, attribute, value) {
}

/**
* @param {Element} dom
* @param {Element} element
* @param {string} attribute
* @param {string | null} value
*/
export function attr(dom, attribute, value) {
export function attr(element, attribute, value) {
value = value == null ? null : value + '';

if (DEV) {
check_src_in_dev_hydration(dom, attribute, value);
}
// @ts-expect-error
var attributes = (element.__attributes ??= {});

if (hydrating) {
attributes[attribute] = element.getAttribute(attribute);

if (attribute === 'src' || attribute === 'href' || attribute === 'srcset') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing the DEV condition

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These attributes are skipped in dev and prod mode. The only difference is that in dev mode we additionally warn, which is what happens on the next line

check_src_in_dev_hydration(element, attribute, value);

if (
!hydrating ||
(dom.getAttribute(attribute) !== value &&
// If we reset those, they would result in another network request, which we want to avoid.
// If we reset these attributes, they would result in another network request, which we want to avoid.
// We assume they are the same between client and server as checking if they are equal is expensive
// (we can't just compare the strings as they can be different between client and server but result in the
// same url, so we would need to create hidden anchor elements to compare them)
attribute !== 'src' &&
attribute !== 'href' &&
attribute !== 'srcset')
) {
if (value === null) {
dom.removeAttribute(attribute);
} else {
dom.setAttribute(attribute, value);
return;
}
}

if (attributes[attribute] === (attributes[attribute] = value)) return;

if (value === null) {
element.removeAttribute(attribute);
} else {
element.setAttribute(attribute, value);
}
}

/**
Expand Down Expand Up @@ -123,14 +126,14 @@ export function spread_attributes_effect(dom, attrs, lowercase_attributes, css_h

/**
* Spreads attributes onto a DOM element, taking into account the currently set attributes
* @param {Element & ElementCSSInlineStyle} dom
* @param {Element & ElementCSSInlineStyle} element
* @param {Record<string, unknown> | undefined} prev
* @param {Record<string, unknown>[]} attrs
* @param {boolean} lowercase_attributes
* @param {string} css_hash
* @returns {Record<string, unknown>}
*/
export function spread_attributes(dom, prev, attrs, lowercase_attributes, css_hash) {
export function spread_attributes(element, prev, attrs, lowercase_attributes, css_hash) {
var next = object_assign({}, ...attrs);
var has_hash = css_hash.length !== 0;

Expand All @@ -144,8 +147,8 @@ export function spread_attributes(dom, prev, attrs, lowercase_attributes, css_ha
next.class = '';
}

var setters = map_get(setters_cache, dom.nodeName);
if (!setters) map_set(setters_cache, dom.nodeName, (setters = get_setters(dom)));
var setters = map_get(setters_cache, element.nodeName);
if (!setters) map_set(setters_cache, element.nodeName, (setters = get_setters(element)));

for (key in next) {
var value = next[key];
Expand All @@ -170,27 +173,27 @@ export function spread_attributes(dom, prev, attrs, lowercase_attributes, css_ha
}

if (!delegated && prev?.[key]) {
dom.removeEventListener(event_name, /** @type {any} */ (prev[key]), opts);
element.removeEventListener(event_name, /** @type {any} */ (prev[key]), opts);
}

if (value != null) {
if (!delegated) {
dom.addEventListener(event_name, value, opts);
element.addEventListener(event_name, value, opts);
} else {
// @ts-ignore
dom[`__${event_name}`] = value;
element[`__${event_name}`] = value;
delegate([event_name]);
}
}
} else if (value == null) {
dom.removeAttribute(key);
element.removeAttribute(key);
} else if (key === 'style') {
dom.style.cssText = value + '';
element.style.cssText = value + '';
} else if (key === 'autofocus') {
autofocus(/** @type {HTMLElement} */ (dom), Boolean(value));
autofocus(/** @type {HTMLElement} */ (element), Boolean(value));
} else if (key === '__value' || key === 'value') {
// @ts-ignore
dom.value = dom[key] = dom.__value = value;
element.value = element[key] = element.__value = value;
} else {
var name = key;
if (lowercase_attributes) {
Expand All @@ -199,25 +202,19 @@ export function spread_attributes(dom, prev, attrs, lowercase_attributes, css_ha
}

if (setters.includes(name)) {
if (DEV) {
check_src_in_dev_hydration(dom, name, value);
}

if (
!hydrating ||
// @ts-ignore see attr method for an explanation of src/srcset
(dom[name] !== value && name !== 'src' && name !== 'href' && name !== 'srcset')
) {
if (hydrating && (name === 'src' || name === 'href' || name === 'srcset')) {
check_src_in_dev_hydration(element, name, value);
} else {
// @ts-ignore
dom[name] = value;
element[name] = value;
}
} else if (typeof value !== 'function') {
if (has_hash && name === 'class') {
if (value) value += ' ';
value += css_hash;
}

attr(dom, name, value);
attr(element, name, value);
}
}
}
Expand Down Expand Up @@ -301,22 +298,20 @@ function get_setters(element) {
}

/**
* @param {any} dom
* @param {any} element
* @param {string} attribute
* @param {string | null} value
*/
function check_src_in_dev_hydration(dom, attribute, value) {
if (!hydrating) return;
if (attribute !== 'src' && attribute !== 'href' && attribute !== 'srcset') return;

if (attribute === 'srcset' && srcset_url_equal(dom, value)) return;
if (src_url_equal(dom.getAttribute(attribute) ?? '', value ?? '')) return;
function check_src_in_dev_hydration(element, attribute, value) {
if (!DEV) return;
if (attribute === 'srcset' && srcset_url_equal(element, value)) return;
if (src_url_equal(element.getAttribute(attribute) ?? '', value ?? '')) return;

// eslint-disable-next-line no-console
console.error(
`Detected a ${attribute} attribute value change during hydration. This will not be repaired during hydration, ` +
`the ${attribute} value that came from the server will be used. Related element:`,
dom,
element,
' Differing value:',
value
);
Expand Down
2 changes: 2 additions & 0 deletions packages/svelte/src/internal/client/dom/operations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ export function init_operations() {
text_prototype.__nodeValue = ' ';
// @ts-expect-error
element_prototype.__className = '';
// @ts-expect-error
element_prototype.__attributes = null;

first_child_get = /** @type {(this: Node) => ChildNode | null} */ (
// @ts-ignore
Expand Down