Skip to content

fix: only create document.title effect if value is dynamic #12698

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.

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 7 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/strange-pears-perform.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: only create `document.title` effect if value is dynamic
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,11 @@ export function BindDirective(node, context) {
)
)?.value
);

if (value !== undefined) {
group_getter = b.thunk(
b.block([
b.stmt(build_attribute_value(value, context)[1]),
b.stmt(build_attribute_value(value, context).value),
b.return(/** @type {Expression} */ (context.visit(expression)))
])
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,17 +229,16 @@ export function RegularElement(node, context) {
(attribute.value === true || is_text_attribute(attribute))
) {
const name = get_attribute_name(node, attribute, context);
const literal_value = /** @type {Literal} */ (
build_attribute_value(attribute.value, context)[1]
).value;
if (name !== 'class' || literal_value) {
const value = is_text_attribute(attribute) ? attribute.value[0].data : true;

if (name !== 'class' || value) {
// TODO namespace=foreign probably doesn't want to do template stuff at all and instead use programmatic methods
// to create the elements it needs.
context.state.template.push(
` ${attribute.name}${
is_boolean_attribute(name) && literal_value === true
is_boolean_attribute(name) && value === true
? ''
: `="${literal_value === true ? '' : escape_html(literal_value, true)}"`
: `="${value === true ? '' : escape_html(value, true)}"`
}`
);
continue;
Expand Down Expand Up @@ -440,7 +439,7 @@ function build_element_spread_attributes(
if (attribute.type === 'Attribute') {
const name = get_attribute_name(element, attribute, context);
// TODO: handle has_call
const [, value] = build_attribute_value(attribute.value, context);
const { value } = build_attribute_value(attribute.value, context);

if (
name === 'is' &&
Expand Down Expand Up @@ -554,7 +553,7 @@ function build_element_attribute_update_assignment(element, node_id, attribute,
const name = get_attribute_name(element, attribute, context);
const is_svg = context.state.metadata.namespace === 'svg' || element.name === 'svg';
const is_mathml = context.state.metadata.namespace === 'mathml';
let [has_call, value] = build_attribute_value(attribute.value, context);
let { has_call, value } = build_attribute_value(attribute.value, context);

// The foreign namespace doesn't have any special handling, everything goes through the attr function
if (context.state.metadata.namespace === 'foreign') {
Expand Down Expand Up @@ -636,7 +635,7 @@ function build_element_attribute_update_assignment(element, node_id, attribute,
function build_custom_element_attribute_update_assignment(node_id, attribute, context) {
const state = context.state;
const name = attribute.name; // don't lowercase, as we set the element's property, which might be case sensitive
let [has_call, value] = build_attribute_value(attribute.value, context);
let { has_call, value } = build_attribute_value(attribute.value, context);

const update = b.stmt(b.call('$.set_custom_element_data', node_id, b.literal(name), value));

Expand Down Expand Up @@ -665,7 +664,7 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co
*/
function build_element_special_value_attribute(element, node_id, attribute, context) {
const state = context.state;
const [, value] = build_attribute_value(attribute.value, context);
const { value } = build_attribute_value(attribute.value, context);

const inner_assignment = b.assignment(
'=',
Expand All @@ -676,7 +675,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont
value
)
);
const is_reactive = attribute.metadata.expression.has_state;

const is_select_with_value =
// attribute.metadata.dynamic would give false negatives because even if the value does not change,
// the inner options could still change, so we need to always treat it as reactive
Expand All @@ -699,7 +698,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont
state.init.push(b.stmt(b.call('$.init_select', node_id, b.thunk(value))));
}

if (is_reactive) {
if (attribute.metadata.expression.has_state) {
const id = state.scope.generate(`${node_id.name}_value`);
build_update_assignment(
state,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ export function SlotElement(node, context) {
if (attribute.type === 'SpreadAttribute') {
spreads.push(b.thunk(/** @type {Expression} */ (context.visit(attribute))));
} else if (attribute.type === 'Attribute') {
const [, value] = build_attribute_value(attribute.value, context);
const { value } = build_attribute_value(attribute.value, context);

if (attribute.name === 'name') {
name = value;
is_default = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export function SvelteElement(node, context) {
get_tag,
node.metadata.svg || node.metadata.mathml ? b.true : b.false,
inner.length > 0 && b.arrow([element_id, b.id('$$anchor')], b.block(inner)),
dynamic_namespace && b.thunk(build_attribute_value(dynamic_namespace, context)[1]),
dynamic_namespace && b.thunk(build_attribute_value(dynamic_namespace, context).value),
location && b.array([b.literal(location.line), b.literal(location.column)])
)
)
Expand Down Expand Up @@ -161,7 +161,7 @@ function build_dynamic_element_attributes(attributes, context, element_id) {

for (const attribute of attributes) {
if (attribute.type === 'Attribute') {
const [, value] = build_attribute_value(attribute.value, context);
const { value } = build_attribute_value(attribute.value, context);

if (
is_event_attribute(attribute) &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,17 @@ import { build_template_literal } from './shared/utils.js';
* @param {ComponentContext} context
*/
export function TitleElement(node, context) {
// TODO throw validation error when attributes present / when children something else than text/expression tags
// TODO only create update when expression is dynamic
const { has_state, value } = build_template_literal(
/** @type {any} */ (node.fragment.nodes),
context.visit,
context.state
);

if (node.fragment.nodes.length === 1 && node.fragment.nodes[0].type === 'Text') {
context.state.init.push(
b.stmt(
b.assignment(
'=',
b.member(b.id('$.document'), b.id('title')),
b.literal(/** @type {Text} */ (node.fragment.nodes[0]).data)
)
)
);
const statement = b.stmt(b.assignment('=', b.member(b.id('$.document'), b.id('title')), value));

if (has_state) {
context.state.update.push(statement);
} else {
context.state.update.push(
b.stmt(
b.assignment(
'=',
b.member(b.id('$.document'), b.id('title')),
build_template_literal(
/** @type {any} */ (node.fragment.nodes),
context.visit,
context.state
)[1]
)
)
);
context.state.init.push(statement);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ export function build_component(node, component_name, context, anchor = context.
} else if (attribute.type === 'Attribute') {
if (attribute.name.startsWith('--')) {
custom_css_props.push(
b.init(attribute.name, build_attribute_value(attribute.value, context)[1])
b.init(attribute.name, build_attribute_value(attribute.value, context).value)
);
continue;
}
Expand All @@ -106,7 +106,7 @@ export function build_component(node, component_name, context, anchor = context.
has_children_prop = true;
}

const [, value] = build_attribute_value(attribute.value, context);
const { value } = build_attribute_value(attribute.value, context);

if (attribute.metadata.expression.has_state) {
let arg = value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export function build_style_directives(
let value =
directive.value === true
? build_getter({ name: directive.name, type: 'Identifier' }, context.state)
: build_attribute_value(directive.value, context)[1];
: build_attribute_value(directive.value, context).value;

const update = b.stmt(
b.call(
Expand Down Expand Up @@ -92,24 +92,24 @@ export function build_class_directives(
/**
* @param {Attribute['value']} value
* @param {ComponentContext} context
* @returns {[has_call: boolean, Expression]}
*/
export function build_attribute_value(value, context) {
if (value === true) {
return [false, b.literal(true)];
return { has_state: false, has_call: false, value: b.literal(true) };
}

if (!Array.isArray(value) || value.length === 1) {
const chunk = Array.isArray(value) ? value[0] : value;

if (chunk.type === 'Text') {
return [false, b.literal(chunk.data)];
return { has_state: false, has_call: false, value: b.literal(chunk.data) };
}

return [
chunk.metadata.expression.has_call,
/** @type {Expression} */ (context.visit(chunk.expression))
];
return {
has_state: chunk.metadata.expression.has_call,
has_call: chunk.metadata.expression.has_call,
value: /** @type {Expression} */ (context.visit(chunk.expression))
};
}

return build_template_literal(value, context.visit, context.state);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,13 @@ export function process_children(nodes, expression, is_element, { visit, state }

state.template.push(' ');

const [has_call, value] = build_template_literal(sequence, visit, state);
const { has_state, has_call, value } = build_template_literal(sequence, visit, state);

const update = b.stmt(b.call('$.set_text', text_id, value));

if (has_call && !within_bound_contenteditable) {
state.init.push(build_update(update));
} else if (
sequence.some(
(node) => node.type === 'ExpressionTag' && node.metadata.expression.has_state
) &&
!within_bound_contenteditable
) {
} else if (has_state && !within_bound_contenteditable) {
state.update.push(update);
} else {
state.init.push(b.stmt(b.assignment('=', b.member(text_id, b.id('nodeValue')), value)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,39 +14,41 @@ import { locator } from '../../../../../state.js';
* @param {Array<Text | ExpressionTag>} values
* @param {(node: SvelteNode, state: any) => any} visit
* @param {ComponentClientTransformState} state
* @returns {[boolean, TemplateLiteral]}
*/
export function build_template_literal(values, visit, state) {
/** @type {TemplateElement[]} */
const quasis = [];

/** @type {Expression[]} */
const expressions = [];

let quasi = b.quasi('');
const quasis = [quasi];

let has_call = false;
let has_state = false;
let contains_multiple_call_expression = false;
quasis.push(b.quasi(''));

for (let i = 0; i < values.length; i++) {
const node = values[i];

if (node.type === 'ExpressionTag' && node.metadata.expression.has_call) {
if (has_call) {
contains_multiple_call_expression = true;
if (node.type === 'ExpressionTag') {
if (node.metadata.expression.has_call) {
if (has_call) {
contains_multiple_call_expression = true;
}
has_call = true;
}
has_call = true;

has_state ||= node.metadata.expression.has_state;
}
}

for (let i = 0; i < values.length; i++) {
const node = values[i];

if (node.type === 'Text') {
const last = /** @type {TemplateElement} */ (quasis.at(-1));
last.value.raw += sanitize_template_string(node.data);
quasi.value.raw += sanitize_template_string(node.data);
} else if (node.type === 'ExpressionTag' && node.expression.type === 'Literal') {
const last = /** @type {TemplateElement} */ (quasis.at(-1));
if (node.expression.value != null) {
last.value.raw += sanitize_template_string(node.expression.value + '');
quasi.value.raw += sanitize_template_string(node.expression.value + '');
}
} else {
if (contains_multiple_call_expression) {
Expand All @@ -65,12 +67,15 @@ export function build_template_literal(values, visit, state) {
} else {
expressions.push(b.logical('??', visit(node.expression, state), b.literal('')));
}
quasis.push(b.quasi('', i + 1 === values.length));

quasi = b.quasi('', i + 1 === values.length);
quasis.push(quasi);
}
}

// TODO instead of this tuple, return a `{ dynamic, complex, value }` object. will DRY stuff out
return [has_call, b.template(quasis, expressions)];
const value = b.template(quasis, expressions);

return { value, has_state, has_call };
}

/**
Expand Down
Loading