Skip to content

Commit d4360af

Browse files
adigubaRich-Harris
adiguba
and
Rich-Harris
authored
chore: rewrite set_class() to handle directives (#15352)
* set_class with class: directives * update expected result (remove leading space) * fix * optimize literals * add test * add test for mutations on hydration * clean observer * Update packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js unused Co-authored-by: Rich Harris <[email protected]> * Update packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js unused for now Co-authored-by: Rich Harris <[email protected]> * Update packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js unused for now Co-authored-by: Rich Harris <[email protected]> * Update packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js nit Co-authored-by: Rich Harris <[email protected]> * Update packages/svelte/src/internal/client/dom/elements/attributes.js nit Co-authored-by: Rich Harris <[email protected]> * Update packages/svelte/src/internal/shared/attributes.js rename clazz to value :D Co-authored-by: Rich Harris <[email protected]> * remove unused + fix JSDoc * drive-by fix * minor style tweaks * tweak test * this is faster * tweak * tweak * this is faster * typo * tweak * changeset --------- Co-authored-by: Rich Harris <[email protected]> Co-authored-by: Rich Harris <[email protected]>
1 parent 5a946e7 commit d4360af

File tree

19 files changed

+688
-329
lines changed

19 files changed

+688
-329
lines changed

‎.changeset/stale-plums-drop.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'svelte': patch
3+
---
4+
5+
fix: correctly override class attributes with class directives

‎packages/svelte/src/compiler/phases/2-analyze/index.js

+29-55
Original file line numberDiff line numberDiff line change
@@ -767,66 +767,40 @@ export function analyze_component(root, source, options) {
767767
if (!should_ignore_unused) {
768768
warn_unused(analysis.css.ast);
769769
}
770+
}
770771

771-
outer: for (const node of analysis.elements) {
772-
if (node.metadata.scoped) {
773-
// Dynamic elements in dom mode always use spread for attributes and therefore shouldn't have a class attribute added to them
774-
// TODO this happens during the analysis phase, which shouldn't know anything about client vs server
775-
if (node.type === 'SvelteElement' && options.generate === 'client') continue;
776-
777-
/** @type {AST.Attribute | undefined} */
778-
let class_attribute = undefined;
779-
780-
for (const attribute of node.attributes) {
781-
if (attribute.type === 'SpreadAttribute') {
782-
// The spread method appends the hash to the end of the class attribute on its own
783-
continue outer;
784-
}
772+
for (const node of analysis.elements) {
773+
if (node.metadata.scoped && is_custom_element_node(node)) {
774+
mark_subtree_dynamic(node.metadata.path);
775+
}
785776

786-
if (attribute.type !== 'Attribute') continue;
787-
if (attribute.name.toLowerCase() !== 'class') continue;
788-
// The dynamic class method appends the hash to the end of the class attribute on its own
789-
if (attribute.metadata.needs_clsx) continue outer;
777+
let has_class = false;
778+
let has_spread = false;
779+
let has_class_directive = false;
790780

791-
class_attribute = attribute;
792-
}
781+
for (const attribute of node.attributes) {
782+
// The spread method appends the hash to the end of the class attribute on its own
783+
if (attribute.type === 'SpreadAttribute') {
784+
has_spread = true;
785+
break;
786+
}
787+
has_class_directive ||= attribute.type === 'ClassDirective';
788+
has_class ||= attribute.type === 'Attribute' && attribute.name.toLowerCase() === 'class';
789+
}
793790

794-
if (class_attribute && class_attribute.value !== true) {
795-
if (is_text_attribute(class_attribute)) {
796-
class_attribute.value[0].data += ` ${analysis.css.hash}`;
797-
} else {
798-
/** @type {AST.Text} */
799-
const css_text = {
800-
type: 'Text',
801-
data: ` ${analysis.css.hash}`,
802-
raw: ` ${analysis.css.hash}`,
803-
start: -1,
804-
end: -1
805-
};
806-
807-
if (Array.isArray(class_attribute.value)) {
808-
class_attribute.value.push(css_text);
809-
} else {
810-
class_attribute.value = [class_attribute.value, css_text];
811-
}
812-
}
813-
} else {
814-
node.attributes.push(
815-
create_attribute('class', -1, -1, [
816-
{
817-
type: 'Text',
818-
data: analysis.css.hash,
819-
raw: analysis.css.hash,
820-
start: -1,
821-
end: -1
822-
}
823-
])
824-
);
825-
if (is_custom_element_node(node) && node.attributes.length === 1) {
826-
mark_subtree_dynamic(node.metadata.path);
791+
// We need an empty class to generate the set_class() or class="" correctly
792+
if (!has_spread && !has_class && (node.metadata.scoped || has_class_directive)) {
793+
node.attributes.push(
794+
create_attribute('class', -1, -1, [
795+
{
796+
type: 'Text',
797+
data: '',
798+
raw: '',
799+
start: -1,
800+
end: -1
827801
}
828-
}
829-
}
802+
])
803+
);
830804
}
831805
}
832806

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js

+61-34
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, Statement } from 'estree' */
1+
/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, ObjectExpression, Statement } from 'estree' */
22
/** @import { AST } from '#compiler' */
33
/** @import { SourceLocation } from '#shared' */
44
/** @import { ComponentClientTransformState, ComponentContext } from '../types' */
@@ -20,9 +20,9 @@ import { build_getter } from '../utils.js';
2020
import {
2121
get_attribute_name,
2222
build_attribute_value,
23-
build_class_directives,
2423
build_style_directives,
25-
build_set_attributes
24+
build_set_attributes,
25+
build_set_class
2626
} from './shared/element.js';
2727
import { process_children } from './shared/fragment.js';
2828
import {
@@ -223,13 +223,13 @@ export function RegularElement(node, context) {
223223

224224
build_set_attributes(
225225
attributes,
226+
class_directives,
226227
context,
227228
node,
228229
node_id,
229230
attributes_id,
230231
(node.metadata.svg || node.metadata.mathml || is_custom_element_node(node)) && b.true,
231-
is_custom_element_node(node) && b.true,
232-
context.state
232+
is_custom_element_node(node) && b.true
233233
);
234234

235235
// If value binding exists, that one takes care of calling $.init_select
@@ -270,13 +270,22 @@ export function RegularElement(node, context) {
270270
continue;
271271
}
272272

273+
const name = get_attribute_name(node, attribute);
273274
if (
274275
!is_custom_element &&
275276
!cannot_be_set_statically(attribute.name) &&
276-
(attribute.value === true || is_text_attribute(attribute))
277+
(attribute.value === true || is_text_attribute(attribute)) &&
278+
(name !== 'class' || class_directives.length === 0)
277279
) {
278-
const name = get_attribute_name(node, attribute);
279-
const value = is_text_attribute(attribute) ? attribute.value[0].data : true;
280+
let value = is_text_attribute(attribute) ? attribute.value[0].data : true;
281+
282+
if (name === 'class' && node.metadata.scoped && context.state.analysis.css.hash) {
283+
if (value === true || value === '') {
284+
value = context.state.analysis.css.hash;
285+
} else {
286+
value += ' ' + context.state.analysis.css.hash;
287+
}
288+
}
280289

281290
if (name !== 'class' || value) {
282291
context.state.template.push(
@@ -290,15 +299,22 @@ export function RegularElement(node, context) {
290299
continue;
291300
}
292301

293-
const is = is_custom_element
294-
? build_custom_element_attribute_update_assignment(node_id, attribute, context)
295-
: build_element_attribute_update_assignment(node, node_id, attribute, attributes, context);
302+
const is =
303+
is_custom_element && name !== 'class'
304+
? build_custom_element_attribute_update_assignment(node_id, attribute, context)
305+
: build_element_attribute_update_assignment(
306+
node,
307+
node_id,
308+
attribute,
309+
attributes,
310+
class_directives,
311+
context
312+
);
296313
if (is) is_attributes_reactive = true;
297314
}
298315
}
299316

300-
// class/style directives must be applied last since they could override class/style attributes
301-
build_class_directives(class_directives, node_id, context, is_attributes_reactive);
317+
// style directives must be applied last since they could override class/style attributes
302318
build_style_directives(style_directives, node_id, context, is_attributes_reactive);
303319

304320
if (
@@ -491,6 +507,27 @@ function setup_select_synchronization(value_binding, context) {
491507
);
492508
}
493509

510+
/**
511+
* @param {AST.ClassDirective[]} class_directives
512+
* @param {ComponentContext} context
513+
* @return {ObjectExpression}
514+
*/
515+
export function build_class_directives_object(class_directives, context) {
516+
let properties = [];
517+
518+
for (const d of class_directives) {
519+
let expression = /** @type Expression */ (context.visit(d.expression));
520+
521+
if (d.metadata.expression.has_call) {
522+
expression = get_expression_id(context.state, expression);
523+
}
524+
525+
properties.push(b.init(d.name, expression));
526+
}
527+
528+
return b.object(properties);
529+
}
530+
494531
/**
495532
* Serializes an assignment to an element property by adding relevant statements to either only
496533
* the init or the the init and update arrays, depending on whether or not the value is dynamic.
@@ -517,6 +554,7 @@ function setup_select_synchronization(value_binding, context) {
517554
* @param {Identifier} node_id
518555
* @param {AST.Attribute} attribute
519556
* @param {Array<AST.Attribute | AST.SpreadAttribute>} attributes
557+
* @param {AST.ClassDirective[]} class_directives
520558
* @param {ComponentContext} context
521559
* @returns {boolean}
522560
*/
@@ -525,6 +563,7 @@ function build_element_attribute_update_assignment(
525563
node_id,
526564
attribute,
527565
attributes,
566+
class_directives,
528567
context
529568
) {
530569
const state = context.state;
@@ -563,19 +602,15 @@ function build_element_attribute_update_assignment(
563602
let update;
564603

565604
if (name === 'class') {
566-
if (attribute.metadata.needs_clsx) {
567-
value = b.call('$.clsx', value);
568-
}
569-
570-
update = b.stmt(
571-
b.call(
572-
is_svg ? '$.set_svg_class' : is_mathml ? '$.set_mathml_class' : '$.set_class',
573-
node_id,
574-
value,
575-
attribute.metadata.needs_clsx && context.state.analysis.css.hash
576-
? b.literal(context.state.analysis.css.hash)
577-
: undefined
578-
)
605+
return build_set_class(
606+
element,
607+
node_id,
608+
attribute,
609+
value,
610+
has_state,
611+
class_directives,
612+
context,
613+
!is_svg && !is_mathml
579614
);
580615
} else if (name === 'value') {
581616
update = b.stmt(b.call('$.set_value', node_id, value));
@@ -639,14 +674,6 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co
639674
const name = attribute.name; // don't lowercase, as we set the element's property, which might be case sensitive
640675
let { value, has_state } = build_attribute_value(attribute.value, context);
641676

642-
// We assume that noone's going to redefine the semantics of the class attribute on custom elements, i.e. it's still used for CSS classes
643-
if (name === 'class' && attribute.metadata.needs_clsx) {
644-
if (context.state.analysis.css.hash) {
645-
value = b.array([value, b.literal(context.state.analysis.css.hash)]);
646-
}
647-
value = b.call('$.clsx', value);
648-
}
649-
650677
const update = b.stmt(b.call('$.set_custom_element_data', node_id, b.literal(name), value));
651678

652679
if (has_state) {

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js

+28-13
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ import * as b from '../../../../utils/builders.js';
77
import { determine_namespace_for_children } from '../../utils.js';
88
import {
99
build_attribute_value,
10-
build_class_directives,
1110
build_set_attributes,
11+
build_set_class,
1212
build_style_directives
1313
} from './shared/element.js';
14-
import { build_render_statement } from './shared/utils.js';
14+
import { build_render_statement, get_expression_id } from './shared/utils.js';
1515

1616
/**
1717
* @param {AST.SvelteElement} node
@@ -80,31 +80,46 @@ export function SvelteElement(node, context) {
8080
// Then do attributes
8181
let is_attributes_reactive = false;
8282

83-
if (attributes.length === 0) {
84-
if (context.state.analysis.css.hash) {
85-
inner_context.state.init.push(
86-
b.stmt(b.call('$.set_class', element_id, b.literal(context.state.analysis.css.hash)))
87-
);
88-
}
89-
} else {
83+
if (
84+
attributes.length === 1 &&
85+
attributes[0].type === 'Attribute' &&
86+
attributes[0].name.toLowerCase() === 'class'
87+
) {
88+
// special case when there only a class attribute
89+
let { value, has_state } = build_attribute_value(
90+
attributes[0].value,
91+
context,
92+
(value, metadata) => (metadata.has_call ? get_expression_id(context.state, value) : value)
93+
);
94+
95+
is_attributes_reactive = build_set_class(
96+
node,
97+
element_id,
98+
attributes[0],
99+
value,
100+
has_state,
101+
class_directives,
102+
inner_context,
103+
false
104+
);
105+
} else if (attributes.length) {
90106
const attributes_id = b.id(context.state.scope.generate('attributes'));
91107

92108
// Always use spread because we don't know whether the element is a custom element or not,
93109
// therefore we need to do the "how to set an attribute" logic at runtime.
94110
is_attributes_reactive = build_set_attributes(
95111
attributes,
112+
class_directives,
96113
inner_context,
97114
node,
98115
element_id,
99116
attributes_id,
100117
b.binary('===', b.member(element_id, 'namespaceURI'), b.id('$.NAMESPACE_SVG')),
101-
b.call(b.member(b.member(element_id, 'nodeName'), 'includes'), b.literal('-')),
102-
context.state
118+
b.call(b.member(b.member(element_id, 'nodeName'), 'includes'), b.literal('-'))
103119
);
104120
}
105121

106-
// class/style directives must be applied last since they could override class/style attributes
107-
build_class_directives(class_directives, element_id, inner_context, is_attributes_reactive);
122+
// style directives must be applied last since they could override class/style attributes
108123
build_style_directives(style_directives, element_id, inner_context, is_attributes_reactive);
109124

110125
const get_tag = b.thunk(/** @type {Expression} */ (context.visit(node.tag)));

0 commit comments

Comments
 (0)