chore: rewrite set_class() to handle directives

.changeset/stale-plums-drop.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: correctly override class attributes with class directives

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

@@ -767,66 +767,40 @@ export function analyze_component(root, source, options) {
 		if (!should_ignore_unused) {
 			warn_unused(analysis.css.ast);
 		}
+	}
 
-		outer: for (const node of analysis.elements) {
-			if (node.metadata.scoped) {
-				// Dynamic elements in dom mode always use spread for attributes and therefore shouldn't have a class attribute added to them
-				// TODO this happens during the analysis phase, which shouldn't know anything about client vs server
-				if (node.type === 'SvelteElement' && options.generate === 'client') continue;
-
-				/** @type {AST.Attribute | undefined} */
-				let class_attribute = undefined;
-
-				for (const attribute of node.attributes) {
-					if (attribute.type === 'SpreadAttribute') {
-						// The spread method appends the hash to the end of the class attribute on its own
-						continue outer;
-					}
+	for (const node of analysis.elements) {
+		if (node.metadata.scoped && is_custom_element_node(node)) {
+			mark_subtree_dynamic(node.metadata.path);
+		}
 
-					if (attribute.type !== 'Attribute') continue;
-					if (attribute.name.toLowerCase() !== 'class') continue;
-					// The dynamic class method appends the hash to the end of the class attribute on its own
-					if (attribute.metadata.needs_clsx) continue outer;
+		let has_class = false;
+		let has_spread = false;
+		let has_class_directive = false;
 
-					class_attribute = attribute;
-				}
+		for (const attribute of node.attributes) {
+			// The spread method appends the hash to the end of the class attribute on its own
+			if (attribute.type === 'SpreadAttribute') {
+				has_spread = true;
+				break;
+			}
+			has_class_directive ||= attribute.type === 'ClassDirective';
+			has_class ||= attribute.type === 'Attribute' && attribute.name.toLowerCase() === 'class';
+		}
 
-				if (class_attribute && class_attribute.value !== true) {
-					if (is_text_attribute(class_attribute)) {
-						class_attribute.value[0].data += ` ${analysis.css.hash}`;
-					} else {
-						/** @type {AST.Text} */
-						const css_text = {
-							type: 'Text',
-							data: ` ${analysis.css.hash}`,
-							raw: ` ${analysis.css.hash}`,
-							start: -1,
-							end: -1
-						};
-
-						if (Array.isArray(class_attribute.value)) {
-							class_attribute.value.push(css_text);
-						} else {
-							class_attribute.value = [class_attribute.value, css_text];
-						}
-					}
-				} else {
-					node.attributes.push(
-						create_attribute('class', -1, -1, [
-							{
-								type: 'Text',
-								data: analysis.css.hash,
-								raw: analysis.css.hash,
-								start: -1,
-								end: -1
-							}
-						])
-					);
-					if (is_custom_element_node(node) && node.attributes.length === 1) {
-						mark_subtree_dynamic(node.metadata.path);
+		// We need an empty class to generate the set_class() or class="" correctly
+		if (!has_spread && !has_class && (node.metadata.scoped || has_class_directive)) {
+			node.attributes.push(
+				create_attribute('class', -1, -1, [
+					{
+						type: 'Text',
+						data: '',
+						raw: '',
+						start: -1,
+						end: -1
 					}
-				}
-			}
+				])
+			);
 		}
 	}
 

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

@@ -1,4 +1,4 @@
-/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, Statement } from 'estree' */
+/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, ObjectExpression, Statement } from 'estree' */
 /** @import { AST } from '#compiler' */
 /** @import { SourceLocation } from '#shared' */
 /** @import { ComponentClientTransformState, ComponentContext } from '../types' */
@@ -20,9 +20,9 @@ import { build_getter } from '../utils.js';
 import {
 	get_attribute_name,
 	build_attribute_value,
-	build_class_directives,
 	build_style_directives,
-	build_set_attributes
+	build_set_attributes,
+	build_set_class
 } from './shared/element.js';
 import { process_children } from './shared/fragment.js';
 import {
@@ -223,13 +223,13 @@ export function RegularElement(node, context) {
 
 		build_set_attributes(
 			attributes,
+			class_directives,
 			context,
 			node,
 			node_id,
 			attributes_id,
 			(node.metadata.svg || node.metadata.mathml || is_custom_element_node(node)) && b.true,
-			is_custom_element_node(node) && b.true,
-			context.state
+			is_custom_element_node(node) && b.true
 		);
 
 		// If value binding exists, that one takes care of calling $.init_select
@@ -270,13 +270,22 @@ export function RegularElement(node, context) {
 				continue;
 			}
 
+			const name = get_attribute_name(node, attribute);
 			if (
 				!is_custom_element &&
 				!cannot_be_set_statically(attribute.name) &&
-				(attribute.value === true || is_text_attribute(attribute))
+				(attribute.value === true || is_text_attribute(attribute)) &&
+				(name !== 'class' || class_directives.length === 0)
 			) {
-				const name = get_attribute_name(node, attribute);
-				const value = is_text_attribute(attribute) ? attribute.value[0].data : true;
+				let value = is_text_attribute(attribute) ? attribute.value[0].data : true;
+
+				if (name === 'class' && node.metadata.scoped && context.state.analysis.css.hash) {
+					if (value === true || value === '') {
+						value = context.state.analysis.css.hash;
+					} else {
+						value += ' ' + context.state.analysis.css.hash;
+					}
+				}
 
 				if (name !== 'class' || value) {
 					context.state.template.push(
@@ -290,15 +299,22 @@ export function RegularElement(node, context) {
 				continue;
 			}
 
-			const is = is_custom_element
-				? build_custom_element_attribute_update_assignment(node_id, attribute, context)
-				: build_element_attribute_update_assignment(node, node_id, attribute, attributes, context);
+			const is =
+				is_custom_element && name !== 'class'
+					? build_custom_element_attribute_update_assignment(node_id, attribute, context)
+					: build_element_attribute_update_assignment(
+							node,
+							node_id,
+							attribute,
+							attributes,
+							class_directives,
+							context
+						);
 			if (is) is_attributes_reactive = true;
 		}
 	}
 
-	// class/style directives must be applied last since they could override class/style attributes
-	build_class_directives(class_directives, node_id, context, is_attributes_reactive);
+	// style directives must be applied last since they could override class/style attributes
 	build_style_directives(style_directives, node_id, context, is_attributes_reactive);
 
 	if (
@@ -492,6 +508,27 @@ function setup_select_synchronization(value_binding, context) {
 	);
 }
 
+/**
+ * @param {AST.ClassDirective[]} class_directives
+ * @param {ComponentContext} context
+ * @return {ObjectExpression}
+ */
+export function build_class_directives_object(class_directives, context) {
+	let properties = [];
+
+	for (const d of class_directives) {
+		let expression = /** @type Expression */ (context.visit(d.expression));
+
+		if (d.metadata.expression.has_call) {
+			expression = get_expression_id(context.state, expression);
+		}
+
+		properties.push(b.init(d.name, expression));
+	}
+
+	return b.object(properties);
+}
+
 /**
  * Serializes an assignment to an element property by adding relevant statements to either only
  * the init or the the init and update arrays, depending on whether or not the value is dynamic.
@@ -518,6 +555,7 @@ function setup_select_synchronization(value_binding, context) {
  * @param {Identifier} node_id
  * @param {AST.Attribute} attribute
  * @param {Array<AST.Attribute | AST.SpreadAttribute>} attributes
+ * @param {AST.ClassDirective[]} class_directives
  * @param {ComponentContext} context
  * @returns {boolean}
  */
@@ -526,6 +564,7 @@ function build_element_attribute_update_assignment(
 	node_id,
 	attribute,
 	attributes,
+	class_directives,
 	context
 ) {
 	const state = context.state;
@@ -564,19 +603,15 @@ function build_element_attribute_update_assignment(
 	let update;
 
 	if (name === 'class') {
-		if (attribute.metadata.needs_clsx) {
-			value = b.call('$.clsx', value);
-		}
-
-		update = b.stmt(
-			b.call(
-				is_svg ? '$.set_svg_class' : is_mathml ? '$.set_mathml_class' : '$.set_class',
-				node_id,
-				value,
-				attribute.metadata.needs_clsx && context.state.analysis.css.hash
-					? b.literal(context.state.analysis.css.hash)
-					: undefined
-			)
+		return build_set_class(
+			element,
+			node_id,
+			attribute,
+			value,
+			has_state,
+			class_directives,
+			context,
+			!is_svg && !is_mathml
 		);
 	} else if (name === 'value') {
 		update = b.stmt(b.call('$.set_value', node_id, value));
@@ -640,14 +675,6 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co
 	const name = attribute.name; // don't lowercase, as we set the element's property, which might be case sensitive
 	let { value, has_state } = build_attribute_value(attribute.value, context);
 
-	// 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
-	if (name === 'class' && attribute.metadata.needs_clsx) {
-		if (context.state.analysis.css.hash) {
-			value = b.array([value, b.literal(context.state.analysis.css.hash)]);
-		}
-		value = b.call('$.clsx', value);
-	}
-
 	const update = b.stmt(b.call('$.set_custom_element_data', node_id, b.literal(name), value));
 
 	if (has_state) {

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

@@ -7,11 +7,11 @@ import * as b from '../../../../utils/builders.js';
 import { determine_namespace_for_children } from '../../utils.js';
 import {
 	build_attribute_value,
-	build_class_directives,
 	build_set_attributes,
+	build_set_class,
 	build_style_directives
 } from './shared/element.js';
-import { build_render_statement } from './shared/utils.js';
+import { build_render_statement, get_expression_id } from './shared/utils.js';
 
 /**
  * @param {AST.SvelteElement} node
@@ -80,31 +80,46 @@ export function SvelteElement(node, context) {
 	// Then do attributes
 	let is_attributes_reactive = false;
 
-	if (attributes.length === 0) {
-		if (context.state.analysis.css.hash) {
-			inner_context.state.init.push(
-				b.stmt(b.call('$.set_class', element_id, b.literal(context.state.analysis.css.hash)))
-			);
-		}
-	} else {
+	if (
+		attributes.length === 1 &&
+		attributes[0].type === 'Attribute' &&
+		attributes[0].name.toLowerCase() === 'class'
+	) {
+		// special case when there only a class attribute
+		let { value, has_state } = build_attribute_value(
+			attributes[0].value,
+			context,
+			(value, metadata) => (metadata.has_call ? get_expression_id(context.state, value) : value)
+		);
+
+		is_attributes_reactive = build_set_class(
+			node,
+			element_id,
+			attributes[0],
+			value,
+			has_state,
+			class_directives,
+			inner_context,
+			false
+		);
+	} else if (attributes.length) {
 		const attributes_id = b.id(context.state.scope.generate('attributes'));
 
 		// Always use spread because we don't know whether the element is a custom element or not,
 		// therefore we need to do the "how to set an attribute" logic at runtime.
 		is_attributes_reactive = build_set_attributes(
 			attributes,
+			class_directives,
 			inner_context,
 			node,
 			element_id,
 			attributes_id,
 			b.binary('===', b.member(element_id, 'namespaceURI'), b.id('$.NAMESPACE_SVG')),
-			b.call(b.member(b.member(element_id, 'nodeName'), 'includes'), b.literal('-')),
-			context.state
+			b.call(b.member(b.member(element_id, 'nodeName'), 'includes'), b.literal('-'))
 		);
 	}
 
-	// class/style directives must be applied last since they could override class/style attributes
-	build_class_directives(class_directives, element_id, inner_context, is_attributes_reactive);
+	// style directives must be applied last since they could override class/style attributes
 	build_style_directives(style_directives, element_id, inner_context, is_attributes_reactive);
 
 	const get_tag = b.thunk(/** @type {Expression} */ (context.visit(node.tag)));