Skip to content

Commit

Permalink
chore: more default value spread work
Browse files Browse the repository at this point in the history 
This is a WIP, which I'm not sure if it goes anywhere. It started out to deeper understand the fix in #14640, and to fix some more theoretical loopholes, but this turns out to not fix the issue yet, and I'm not sure if it even will, so I'm punting on it for now but putting it up for others to see.
  • Loading branch information
@dummdidumm
dummdidumm committed Dec 10, 2024
1 parent ce373ff commit 95b87ff
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 24 deletions.
34 changes: 27 additions & 7 deletions packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export function build_element_attributes(node, context) {
/** @type {Expression | null} */
let content = null;

let has_spread = false;
let needs_spread = false;
// Use the index to keep the attributes order which is important for spreading
let class_index = -1;
let style_index = -1;
Expand Down Expand Up @@ -82,8 +82,28 @@ export function build_element_attributes(node, context) {
) {
events_to_capture.add(attribute.name);
}
// the defaultValue/defaultChecked properties don't exist as attributes
} else if (attribute.name !== 'defaultValue' && attribute.name !== 'defaultChecked') {
} else if (
(node.name === 'input' || node.name === 'textarea') &&
(attribute.name === 'defaultValue' || attribute.name === 'defaultChecked')
) {
// If there's a defaultValue but not value attribute, we turn it into a value attribute (same for checked).
// If we can't, then we need to use a spread in order to be able to detect at runtime whether or not
// the value/checked value is nullish, in which case defaultValue/defaultChecked should be used.
const replacement_name = attribute.name === 'defaultValue' ? 'value' : 'checked';
if (
!node.attributes.some(
(attr) =>
attr.type === 'SpreadAttribute' ||
((attr.type === 'BindDirective' || attr.type === 'Attribute') &&
attr.name === replacement_name)
)
) {
attributes.push({ ...attribute, name: replacement_name });
} else {
needs_spread = true;
attributes.push(attribute);
}
} else {
if (attribute.name === 'class') {
class_index = attributes.length;
} else if (attribute.name === 'style') {
Expand Down Expand Up @@ -173,7 +193,7 @@ export function build_element_attributes(node, context) {
}
} else if (attribute.type === 'SpreadAttribute') {
attributes.push(attribute);
has_spread = true;
needs_spread = true;
if (is_load_error_element(node.name)) {
events_to_capture.add('onload');
events_to_capture.add('onerror');
Expand All @@ -194,7 +214,7 @@ export function build_element_attributes(node, context) {
}
}

if (class_directives.length > 0 && !has_spread) {
if (class_directives.length > 0 && !needs_spread) {
const class_attribute = build_class_directives(
class_directives,
/** @type {AST.Attribute | null} */ (attributes[class_index] ?? null)
Expand All @@ -204,7 +224,7 @@ export function build_element_attributes(node, context) {
}
}

if (style_directives.length > 0 && !has_spread) {
if (style_directives.length > 0 && !needs_spread) {
build_style_directives(
style_directives,
/** @type {AST.Attribute | null} */ (attributes[style_index] ?? null),
Expand All @@ -215,7 +235,7 @@ export function build_element_attributes(node, context) {
}
}

if (has_spread) {
if (needs_spread) {
build_element_spread_attributes(node, attributes, style_directives, class_directives, context);
} else {
for (const attribute of /** @type {AST.Attribute[]} */ (attributes)) {
Expand Down
56 changes: 39 additions & 17 deletions packages/svelte/src/internal/client/dom/elements/attributes.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,36 +344,58 @@ export function set_attributes(
element.style.cssText = value + '';
} else if (key === 'autofocus') {
autofocus(/** @type {HTMLElement} */ (element), Boolean(value));
} else if (key === '__value' || (key === 'value' && value != null)) {
// @ts-ignore
element.value = element[key] = element.__value = value;
} else if (key === 'selected' && is_option_element) {
} else if (
key === '__value' ||
(key === 'value' &&
// For <input> element we don't want to fall through to removeAttribute below,
// because that attribute corresponds to the defaultValue, not the value property
(value != null || !is_custom_element))
) {
set_value(element, value);
} else if (key === 'checked' && (value != null || !is_custom_element)) {
set_checked(element, value);
} else if (is_option_element && key === 'selected') {
set_selected(/** @type {HTMLOptionElement} */ (element), value);
} else {
var name = key;
if (!preserve_attribute_case) {
name = normalize_attribute(name);
}
let is_default_value_or_checked = name === 'defaultValue' || name === 'defaultChecked';

if (value == null && !is_custom_element && !is_default_value_or_checked) {
if (value == null && !is_custom_element && key !== 'checked') {
attributes[key] = null;
// if we remove the value/checked attributes this also for some reasons reset
// the default value so we need to keep track of it and reassign it after the remove
let default_value_reset = /**@type {HTMLInputElement}*/ (element).defaultValue;
let default_checked_reset = /**@type {HTMLInputElement}*/ (element).defaultChecked;
element.removeAttribute(key);
if (key === 'value') {
/**@type {HTMLInputElement}*/ (element).defaultValue = default_value_reset;
} else if (key === 'checked') {
/**@type {HTMLInputElement}*/ (element).defaultChecked = default_checked_reset;
}
} else if (
is_default_value_or_checked ||
(setters.includes(name) && (is_custom_element || typeof value !== 'string'))
// is_default_value_or_checked ||
setters.includes(name) &&
(is_custom_element ||
typeof value !== 'string' ||
name === 'defaultValue' ||
name === 'defaultChecked')
) {
// if we adjust the value/checked attributes this also for some reasons reset
// the default value so we need to keep track of it and reassign it after the remove
let default_value_reset;
let default_checked_reset;
if (hydrating) {
default_value_reset = /**@type {HTMLInputElement}*/ (element).value;
default_checked_reset = /**@type {HTMLInputElement}*/ (element).checked;
}

// @ts-ignore
element[name] = value;

if (hydrating) {
if (key === 'defaultValue') {
/**@type {HTMLInputElement}*/ (element).value = /** @type {string} */ (
default_value_reset
);
} else if (key === 'defaultChecked') {
/**@type {HTMLInputElement}*/ (element).checked = /** @type {boolean} */ (
default_checked_reset
);
}
}
} else if (typeof value !== 'function') {
if (hydrating && (name === 'src' || name === 'href' || name === 'srcset')) {
if (!skip_warning) check_src_in_dev_hydration(element, name, value ?? '');
Expand Down
16 changes: 16 additions & 0 deletions packages/svelte/src/internal/server/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,22 @@ export function spread_attributes(attrs, classes, styles, flags = 0) {

var value = attrs[name];

if (is_html) {
if (name === 'defaultvalue') {
if (attrs.value == null) {
name = 'value';
} else {
continue;
}
} else if (name === 'defaultchecked') {
if (attrs.checked == null) {
name = 'checked';
} else {
continue;
}
}
}

if (lowercase) {
name = name.toLowerCase();
}
Expand Down

0 comments on commit 95b87ff

Please sign in to comment.