Skip to content

Commit ac7e160

Browse files
LeeWxx7nikRich-Harris
authored
fix: SSR scoped classes for <select value> elements (#16821)
* fix: preserve scoped classes on select during ssr * test: cover select scoped class regression * chore: changeset * chore: format files after lint * fix: unify <select> attribute handling and prevent double-await * test: update renderer.select call order * fix: restore scoped classes on <option> * test: cover scoped class for <option> * dry * de-diff * tweak changeset --------- Co-authored-by: 7nik <[email protected]> Co-authored-by: Rich Harris <[email protected]>
1 parent 24944e6 commit ac7e160

File tree

9 files changed

+172
-66
lines changed

9 files changed

+172
-66
lines changed

‎.changeset/nasty-comics-play.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'svelte': patch
3+
---
4+
5+
fix: SSR regression of processing attributes of `<select>` and `<option>`

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

Lines changed: 30 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,10 @@ import { is_void } from '../../../../../utils.js';
77
import { dev, locator } from '../../../../state.js';
88
import * as b from '#compiler/builders';
99
import { clean_nodes, determine_namespace_for_children } from '../../utils.js';
10-
import { build_element_attributes, build_spread_object } from './shared/element.js';
10+
import { build_element_attributes, prepare_element_spread_object } from './shared/element.js';
1111
import {
1212
process_children,
1313
build_template,
14-
build_attribute_value,
1514
create_child_block,
1615
PromiseOptimiser
1716
} from './shared/utils.js';
@@ -37,9 +36,27 @@ export function RegularElement(node, context) {
3736

3837
const optimiser = new PromiseOptimiser();
3938

40-
state.template.push(b.literal(`<${node.name}`));
41-
const body = build_element_attributes(node, { ...context, state }, optimiser.transform);
42-
state.template.push(b.literal(node_is_void ? '/>' : '>')); // add `/>` for XHTML compliance
39+
// If this element needs special handling (like <select value> / <option>),
40+
// avoid calling build_element_attributes here to prevent evaluating/awaiting
41+
// attribute expressions twice. We'll handle attributes in the special branch.
42+
const is_select_special =
43+
node.name === 'select' &&
44+
node.attributes.some(
45+
(attribute) =>
46+
((attribute.type === 'Attribute' || attribute.type === 'BindDirective') &&
47+
attribute.name === 'value') ||
48+
attribute.type === 'SpreadAttribute'
49+
);
50+
const is_option_special = node.name === 'option';
51+
const is_special = is_select_special || is_option_special;
52+
53+
let body = /** @type {Expression | null} */ (null);
54+
if (!is_special) {
55+
// only open the tag in the non-special path
56+
state.template.push(b.literal(`<${node.name}`));
57+
body = build_element_attributes(node, { ...context, state }, optimiser.transform);
58+
state.template.push(b.literal(node_is_void ? '/>' : '>')); // add `/>` for XHTML compliance
59+
}
4360

4461
if ((node.name === 'script' || node.name === 'style') && node.fragment.nodes.length === 1) {
4562
state.template.push(
@@ -95,27 +112,7 @@ export function RegularElement(node, context) {
95112
);
96113
}
97114

98-
if (
99-
node.name === 'select' &&
100-
node.attributes.some(
101-
(attribute) =>
102-
((attribute.type === 'Attribute' || attribute.type === 'BindDirective') &&
103-
attribute.name === 'value') ||
104-
attribute.type === 'SpreadAttribute'
105-
)
106-
) {
107-
const attributes = build_spread_object(
108-
node,
109-
node.attributes.filter(
110-
(attribute) =>
111-
attribute.type === 'Attribute' ||
112-
attribute.type === 'BindDirective' ||
113-
attribute.type === 'SpreadAttribute'
114-
),
115-
context,
116-
optimiser.transform
117-
);
118-
115+
if (is_select_special) {
119116
const inner_state = { ...state, template: [], init: [] };
120117
process_children(trimmed, { ...context, state: inner_state });
121118

@@ -124,7 +121,9 @@ export function RegularElement(node, context) {
124121
b.block([...state.init, ...build_template(inner_state.template)])
125122
);
126123

127-
const statement = b.stmt(b.call('$$renderer.select', attributes, fn));
124+
const [attributes, ...rest] = prepare_element_spread_object(node, context, optimiser.transform);
125+
126+
const statement = b.stmt(b.call('$$renderer.select', attributes, fn, ...rest));
128127

129128
if (optimiser.expressions.length > 0) {
130129
context.state.template.push(
@@ -137,19 +136,7 @@ export function RegularElement(node, context) {
137136
return;
138137
}
139138

140-
if (node.name === 'option') {
141-
const attributes = build_spread_object(
142-
node,
143-
node.attributes.filter(
144-
(attribute) =>
145-
attribute.type === 'Attribute' ||
146-
attribute.type === 'BindDirective' ||
147-
attribute.type === 'SpreadAttribute'
148-
),
149-
context,
150-
optimiser.transform
151-
);
152-
139+
if (is_option_special) {
153140
let body;
154141

155142
if (node.metadata.synthetic_value_node) {
@@ -167,7 +154,9 @@ export function RegularElement(node, context) {
167154
);
168155
}
169156

170-
const statement = b.stmt(b.call('$$renderer.option', attributes, body));
157+
const [attributes, ...rest] = prepare_element_spread_object(node, context, optimiser.transform);
158+
159+
const statement = b.stmt(b.call('$$renderer.option', attributes, body, ...rest));
171160

172161
if (optimiser.expressions.length > 0) {
173162
context.state.template.push(

‎packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js‎

Lines changed: 82 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -358,42 +358,108 @@ function build_element_spread_attributes(
358358
context,
359359
transform
360360
) {
361+
const args = prepare_element_spread(
362+
element,
363+
/** @type {Array<AST.Attribute | AST.SpreadAttribute | AST.BindDirective>} */ (attributes),
364+
style_directives,
365+
class_directives,
366+
context,
367+
transform
368+
);
369+
370+
let call = b.call('$.attributes', ...args);
371+
372+
context.state.template.push(call);
373+
}
374+
375+
/**
376+
* Prepare args for $.attributes(...): compute object, css_hash, classes, styles and flags.
377+
* @param {AST.RegularElement | AST.SvelteElement} element
378+
* @param {ComponentContext} context
379+
* @param {(expression: Expression, metadata: ExpressionMetadata) => Expression} transform
380+
* @returns {[ObjectExpression,Literal | undefined, ObjectExpression | undefined, ObjectExpression | undefined, Literal | undefined]}
381+
*/
382+
export function prepare_element_spread_object(element, context, transform) {
383+
/** @type {Array<AST.Attribute | AST.SpreadAttribute | AST.BindDirective>} */
384+
const select_attributes = [];
385+
/** @type {AST.ClassDirective[]} */
386+
const class_directives = [];
387+
/** @type {AST.StyleDirective[]} */
388+
const style_directives = [];
389+
390+
for (const attribute of element.attributes) {
391+
if (
392+
attribute.type === 'Attribute' ||
393+
attribute.type === 'BindDirective' ||
394+
attribute.type === 'SpreadAttribute'
395+
) {
396+
select_attributes.push(attribute);
397+
} else if (attribute.type === 'ClassDirective') {
398+
class_directives.push(attribute);
399+
} else if (attribute.type === 'StyleDirective') {
400+
style_directives.push(attribute);
401+
}
402+
}
403+
404+
return prepare_element_spread(
405+
element,
406+
select_attributes,
407+
style_directives,
408+
class_directives,
409+
context,
410+
transform
411+
);
412+
}
413+
414+
/**
415+
* Prepare args for $.attributes(...): compute object, css_hash, classes, styles and flags.
416+
* @param {AST.RegularElement | AST.SvelteElement} element
417+
* @param {Array<AST.Attribute | AST.SpreadAttribute | AST.BindDirective>} attributes
418+
* @param {AST.StyleDirective[]} style_directives
419+
* @param {AST.ClassDirective[]} class_directives
420+
* @param {ComponentContext} context
421+
* @param {(expression: Expression, metadata: ExpressionMetadata) => Expression} transform
422+
* @returns {[ObjectExpression,Literal | undefined, ObjectExpression | undefined, ObjectExpression | undefined, Literal | undefined]}
423+
*/
424+
export function prepare_element_spread(
425+
element,
426+
attributes,
427+
style_directives,
428+
class_directives,
429+
context,
430+
transform
431+
) {
432+
/** @type {ObjectExpression | undefined} */
361433
let classes;
434+
/** @type {ObjectExpression | undefined} */
362435
let styles;
363436
let flags = 0;
364437

365-
let has_await = false;
366-
367438
if (class_directives.length) {
368-
const properties = class_directives.map((directive) => {
369-
has_await ||= directive.metadata.expression.has_await;
370-
371-
return b.init(
439+
const properties = class_directives.map((directive) =>
440+
b.init(
372441
directive.name,
373442
directive.expression.type === 'Identifier' && directive.expression.name === directive.name
374443
? b.id(directive.name)
375444
: transform(
376445
/** @type {Expression} */ (context.visit(directive.expression)),
377446
directive.metadata.expression
378447
)
379-
);
380-
});
448+
)
449+
);
381450

382451
classes = b.object(properties);
383452
}
384453

385454
if (style_directives.length > 0) {
386-
const properties = style_directives.map((directive) => {
387-
has_await ||= directive.metadata.expression.has_await;
388-
389-
return b.init(
455+
const properties = style_directives.map((directive) =>
456+
b.init(
390457
directive.name,
391458
directive.value === true
392459
? b.id(directive.name)
393460
: build_attribute_value(directive.value, context, transform, true)
394-
);
395-
});
396-
461+
)
462+
);
397463
styles = b.object(properties);
398464
}
399465

@@ -406,17 +472,12 @@ function build_element_spread_attributes(
406472
}
407473

408474
const object = build_spread_object(element, attributes, context, transform);
409-
410475
const css_hash =
411476
element.metadata.scoped && context.state.analysis.css.hash
412477
? b.literal(context.state.analysis.css.hash)
413478
: undefined;
414479

415-
const args = [object, css_hash, classes, styles, flags ? b.literal(flags) : undefined];
416-
417-
let call = b.call('$.attributes', ...args);
418-
419-
context.state.template.push(call);
480+
return [object, css_hash, classes, styles, flags ? b.literal(flags) : undefined];
420481
}
421482

422483
/**

‎packages/svelte/src/internal/server/renderer.js‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,9 +160,16 @@ export class Renderer {
160160
/**
161161
* @param {Record<string, any>} attrs
162162
* @param {(renderer: Renderer) => void} fn
163+
* @param {string | undefined} [css_hash]
164+
* @param {Record<string, boolean> | undefined} [classes]
165+
* @param {Record<string, string> | undefined} [styles]
166+
* @param {number | undefined} [flags]
167+
* @returns {void}
163168
*/
164-
select({ value, ...attrs }, fn) {
165-
this.push(`<select${attributes(attrs)}>`);
169+
select(attrs, fn, css_hash, classes, styles, flags) {
170+
const { value, ...select_attrs } = attrs;
171+
172+
this.push(`<select${attributes(select_attrs, css_hash, classes, styles, flags)}>`);
166173
this.child((renderer) => {
167174
renderer.local.select_value = value;
168175
fn(renderer);
@@ -173,9 +180,13 @@ export class Renderer {
173180
/**
174181
* @param {Record<string, any>} attrs
175182
* @param {string | number | boolean | ((renderer: Renderer) => void)} body
183+
* @param {string | undefined} [css_hash]
184+
* @param {Record<string, boolean> | undefined} [classes]
185+
* @param {Record<string, string> | undefined} [styles]
186+
* @param {number | undefined} [flags]
176187
*/
177-
option(attrs, body) {
178-
this.#out.push(`<option${attributes(attrs)}`);
188+
option(attrs, body, css_hash, classes, styles, flags) {
189+
this.#out.push(`<option${attributes(attrs, css_hash, classes, styles, flags)}`);
179190

180191
/**
181192
* @param {Renderer} renderer

‎packages/svelte/src/internal/server/renderer.test.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,24 @@ test('selects an option with an implicit value', () => {
207207
);
208208
});
209209

210+
test('select merges scoped css hash with static class', () => {
211+
const component = (renderer: Renderer) => {
212+
renderer.select(
213+
{ class: 'foo', value: 'foo' },
214+
(renderer) => {
215+
renderer.option({ value: 'foo' }, (renderer) => renderer.push('foo'));
216+
},
217+
'svelte-hash'
218+
);
219+
};
220+
221+
const { head, body } = Renderer.render(component as unknown as Component);
222+
expect(head).toBe('');
223+
expect(body).toBe(
224+
'<!--[--><select class="foo svelte-hash"><option value="foo" selected>foo</option></select><!--]-->'
225+
);
226+
});
227+
210228
describe('async', () => {
211229
beforeAll(() => {
212230
enable_async_mode_flag();

‎packages/svelte/tests/server-side-rendering/samples/option-scoped-class/_expected.html‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
<select><option class="opt svelte-1l69ci" value="foo" selected>foo</option></select>
2+

‎packages/svelte/tests/server-side-rendering/samples/option-scoped-class/main.svelte‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<select value="foo">
2+
<option class="opt" value="foo">foo</option>
3+
</select>
4+
5+
<style>
6+
option {
7+
color: red;
8+
}
9+
</style>
10+

‎packages/svelte/tests/server-side-rendering/samples/select-value-scoped-class/_expected.html‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
<select class="foo svelte-18avu97"><option value="foo" selected>foo</option></select>

‎packages/svelte/tests/server-side-rendering/samples/select-value-scoped-class/main.svelte‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<select class="foo" value="foo">
2+
<option value="foo">foo</option>
3+
</select>
4+
5+
<style>
6+
select {
7+
color: red;
8+
}
9+
</style>

0 commit comments

Comments
 (0)