Skip to content

breaking: warn on quotes single-expression attributes in runes mode #12479

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 6 commits into from
Jul 17, 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/plenty-items-build.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

breaking: warn on quoted single-expression attributes in runes mode
4 changes: 4 additions & 0 deletions packages/svelte/messages/compile-warnings/template.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@

> '%wrong%' is not a valid HTML attribute. Did you mean '%right%'?

## attribute_quoted

> Quoted attributes on components and custom elements will be stringified in a future version of Svelte. If this isn't what you want, remove the quotes

## bind_invalid_each_rest

> The rest operator (...) will create a new object and binding '%name%' with the original object will not work
Expand Down
28 changes: 28 additions & 0 deletions packages/svelte/src/compiler/legacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,34 @@ export function convert(source, ast) {
)
};
},
Attribute(node, { visit, next, path }) {
if (node.value !== true && !Array.isArray(node.value)) {
path.push(node);
const value = /** @type {Legacy.LegacyAttribute['value']} */ ([visit(node.value)]);
path.pop();

return {
...node,
value
};
} else {
return next();
}
},
StyleDirective(node, { visit, next, path }) {
if (node.value !== true && !Array.isArray(node.value)) {
path.push(node);
const value = /** @type {Legacy.LegacyStyleDirective['value']} */ ([visit(node.value)]);
path.pop();

return {
...node,
value
};
} else {
return next();
}
},
SpreadAttribute(node) {
return { ...node, type: 'Spread' };
},
Expand Down
8 changes: 5 additions & 3 deletions packages/svelte/src/compiler/migrate/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -532,11 +532,13 @@ const template = {
if (attr.name === 'name') {
slot_name = /** @type {any} */ (attr.value)[0].data;
} else {
const attr_value =
attr.value === true || Array.isArray(attr.value) ? attr.value : [attr.value];
const value =
attr.value !== true
attr_value !== true
? state.str.original.substring(
attr.value[0].start,
attr.value[attr.value.length - 1].end
attr_value[0].start,
attr_value[attr_value.length - 1].end
)
: 'true';
slot_props += value === attr.name ? `${value}, ` : `${attr.name}: ${value}, `;
Expand Down
10 changes: 8 additions & 2 deletions packages/svelte/src/compiler/phases/1-parse/read/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ export default function read_options(node) {
case 'customElement': {
/** @type {SvelteOptions['customElement']} */
const ce = { tag: '' };
const { value: v } = attribute;
const value = v === true || Array.isArray(v) ? v : [v];

const { value } = attribute;
if (value === true) {
e.svelte_options_invalid_customelement(attribute);
} else if (value[0].type === 'Text') {
Expand Down Expand Up @@ -199,7 +200,11 @@ export default function read_options(node) {
*/
function get_static_value(attribute) {
const { value } = attribute;
const chunk = value[0];

if (value === true) return true;

const chunk = Array.isArray(value) ? value[0] : value;

if (!chunk) return true;
if (value.length > 1) {
return null;
Expand All @@ -208,6 +213,7 @@ function get_static_value(attribute) {
if (chunk.expression.type !== 'Literal') {
return null;
}

return chunk.expression.value;
}

Expand Down
37 changes: 23 additions & 14 deletions packages/svelte/src/compiler/phases/1-parse/state/element.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as e from '../../../errors.js';
import * as w from '../../../warnings.js';
import { create_fragment } from '../utils/create.js';
import { create_attribute } from '../../nodes.js';
import { get_attribute_expression, is_expression_attribute } from '../../../utils/ast.js';

// eslint-disable-next-line no-useless-escape
const valid_tag_name = /^\!?[a-zA-Z]{1,}:?[a-zA-Z0-9\-]*/;
Expand Down Expand Up @@ -241,15 +242,11 @@ export default function element(parser) {
}

const definition = /** @type {Compiler.Attribute} */ (element.attributes.splice(index, 1)[0]);
if (
definition.value === true ||
definition.value.length !== 1 ||
definition.value[0].type === 'Text'
) {
if (!is_expression_attribute(definition)) {
e.svelte_component_invalid_this(definition.start);
}

element.expression = definition.value[0].expression;
element.expression = get_attribute_expression(definition);
}

if (element.type === 'SvelteElement') {
Expand All @@ -267,15 +264,16 @@ export default function element(parser) {
e.svelte_element_missing_this(definition);
}

const chunk = definition.value[0];

if (definition.value.length !== 1 || chunk.type !== 'ExpressionTag') {
if (!is_expression_attribute(definition)) {
w.svelte_element_invalid_this(definition);

// note that this is wrong, in the case of e.g. `this="h{n}"` — it will result in `<h>`.
// it would be much better to just error here, but we are preserving the existing buggy
// Svelte 4 behaviour out of an overabundance of caution regarding breaking changes.
// TODO in 6.0, error
const chunk = /** @type {Array<Compiler.ExpressionTag | Compiler.Text>} */ (
definition.value
)[0];
element.tag =
chunk.type === 'Text'
? {
Expand All @@ -287,7 +285,7 @@ export default function element(parser) {
}
: chunk.expression;
} else {
element.tag = chunk.expression;
element.tag = get_attribute_expression(definition);
}
}

Expand Down Expand Up @@ -543,7 +541,7 @@ function read_attribute(parser) {
}
};

return create_attribute(name, start, parser.index, [expression]);
return create_attribute(name, start, parser.index, expression);
}
}

Expand All @@ -557,7 +555,7 @@ function read_attribute(parser) {
const colon_index = name.indexOf(':');
const type = colon_index !== -1 && get_directive_type(name.slice(0, colon_index));

/** @type {true | Array<Compiler.Text | Compiler.ExpressionTag>} */
/** @type {true | Compiler.ExpressionTag | Array<Compiler.Text | Compiler.ExpressionTag>} */
let value = true;
if (parser.eat('=')) {
parser.allow_whitespace();
Expand Down Expand Up @@ -589,7 +587,9 @@ function read_attribute(parser) {
};
}

const first_value = value === true ? undefined : value[0];
const first_value = value === true ? undefined : Array.isArray(value) ? value[0] : value;

/** @type {import('estree').Expression | null} */
let expression = null;

if (first_value) {
Expand All @@ -598,6 +598,8 @@ function read_attribute(parser) {
if (attribute_contains_text) {
e.directive_invalid_value(/** @type {number} */ (first_value.start));
} else {
// TODO throw a parser error in a future version here if this `[ExpressionTag]` instead of `ExpressionTag`,
// which means stringified value, which isn't allowed for some directives?
expression = first_value.expression;
}
}
Expand Down Expand Up @@ -662,6 +664,7 @@ function get_directive_type(name) {

/**
* @param {Parser} parser
* @return {Compiler.ExpressionTag | Array<Compiler.ExpressionTag | Compiler.Text>}
*/
function read_attribute_value(parser) {
const quote_mark = parser.eat("'") ? "'" : parser.eat('"') ? '"' : null;
Expand All @@ -678,6 +681,7 @@ function read_attribute_value(parser) {
];
}

/** @type {Array<Compiler.ExpressionTag | Compiler.Text>} */
let value;
try {
value = read_sequence(
Expand Down Expand Up @@ -708,7 +712,12 @@ function read_attribute_value(parser) {
}

if (quote_mark) parser.index += 1;
return value;

if (quote_mark || value.length > 1 || value[0].type === 'Text') {
return value;
} else {
return value[0];
}
}

/**
Expand Down
10 changes: 4 additions & 6 deletions packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import { walk } from 'zimmerframe';
import { get_possible_values } from './utils.js';
import { regex_ends_with_whitespace, regex_starts_with_whitespace } from '../../patterns.js';
import { get_attribute_chunks, is_text_attribute } from '../../../utils/ast.js';

/**
* @typedef {{
Expand Down Expand Up @@ -444,14 +445,11 @@ function attribute_matches(node, name, expected_value, operator, case_insensitiv
if (attribute.value === true) return operator === null;
if (expected_value === null) return true;

const chunks = attribute.value;
if (chunks.length === 1) {
const value = chunks[0];
if (value.type === 'Text') {
return test_attribute(operator, expected_value, case_insensitive, value.data);
}
if (is_text_attribute(attribute)) {
return test_attribute(operator, expected_value, case_insensitive, attribute.value[0].data);
}

const chunks = get_attribute_chunks(attribute.value);
const possible_values = new Set();

/** @type {string[]} */
Expand Down
28 changes: 17 additions & 11 deletions packages/svelte/src/compiler/phases/2-analyze/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import {
is_event_attribute,
is_text_attribute,
object,
unwrap_optional
unwrap_optional,
get_attribute_expression,
get_attribute_chunks
} from '../../utils/ast.js';
import * as b from '../../utils/builders.js';
import { MathMLElements, ReservedKeywords, Runes, SVGElements } from '../constants.js';
Expand Down Expand Up @@ -597,19 +599,24 @@ export function analyze_component(root, source, options) {
}

if (class_attribute && class_attribute.value !== true) {
const chunks = class_attribute.value;

if (chunks.length === 1 && chunks[0].type === 'Text') {
chunks[0].data += ` ${analysis.css.hash}`;
if (is_text_attribute(class_attribute)) {
class_attribute.value[0].data += ` ${analysis.css.hash}`;
} else {
chunks.push({
/** @type {import('#compiler').Text} */
const css_text = {
type: 'Text',
data: ` ${analysis.css.hash}`,
raw: ` ${analysis.css.hash}`,
start: -1,
end: -1,
parent: null
});
};

if (Array.isArray(class_attribute.value)) {
class_attribute.value.push(css_text);
} else {
class_attribute.value = [class_attribute.value, css_text];
}
}
} else {
element.attributes.push(
Expand Down Expand Up @@ -1171,7 +1178,7 @@ const common_visitors = {

context.next();

node.metadata.dynamic = node.value.some((chunk) => {
node.metadata.dynamic = get_attribute_chunks(node.value).some((chunk) => {
if (chunk.type !== 'ExpressionTag') {
return false;
}
Expand All @@ -1192,8 +1199,7 @@ const common_visitors = {
context.state.analysis.uses_event_attributes = true;
}

const expression = node.value[0].expression;

const expression = get_attribute_expression(node);
const delegated_event = get_delegated_event(node.name.slice(2), expression, context);

if (delegated_event !== null) {
Expand Down Expand Up @@ -1228,7 +1234,7 @@ const common_visitors = {
}
} else {
context.next();
node.metadata.dynamic = node.value.some(
node.metadata.dynamic = get_attribute_chunks(node.value).some(
(node) => node.type === 'ExpressionTag' && node.metadata.dynamic
);
}
Expand Down
34 changes: 26 additions & 8 deletions packages/svelte/src/compiler/phases/2-analyze/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
import * as e from '../../errors.js';
import {
extract_identifiers,
get_attribute_expression,
get_parent,
is_expression_attribute,
is_text_attribute,
Expand All @@ -33,9 +34,26 @@ import { Scope, get_rune } from '../scope.js';
import { merge } from '../visitors.js';
import { a11y_validators } from './a11y.js';

/** @param {import('#compiler').Attribute} attribute */
function validate_attribute(attribute) {
if (attribute.value === true || attribute.value.length === 1) return;
/**
* @param {import('#compiler').Attribute} attribute
* @param {import('#compiler').ElementLike} parent
*/
function validate_attribute(attribute, parent) {
if (
Array.isArray(attribute.value) &&
attribute.value.length === 1 &&
attribute.value[0].type === 'ExpressionTag' &&
(parent.type === 'Component' ||
parent.type === 'SvelteComponent' ||
parent.type === 'SvelteSelf' ||
(parent.type === 'RegularElement' && is_custom_element_node(parent)))
) {
w.attribute_quoted(attribute);
}

if (attribute.value === true || !Array.isArray(attribute.value) || attribute.value.length === 1) {
return;
}

const is_quoted = attribute.value.at(-1)?.end !== attribute.end;

Expand Down Expand Up @@ -69,10 +87,10 @@ function validate_component(node, context) {

if (attribute.type === 'Attribute') {
if (context.state.analysis.runes) {
validate_attribute(attribute);
validate_attribute(attribute, node);

if (is_expression_attribute(attribute)) {
const expression = attribute.value[0].expression;
const expression = get_attribute_expression(attribute);
if (expression.type === 'SequenceExpression') {
let i = /** @type {number} */ (expression.start);
while (--i > 0) {
Expand Down Expand Up @@ -122,10 +140,10 @@ function validate_element(node, context) {
const is_expression = is_expression_attribute(attribute);

if (context.state.analysis.runes) {
validate_attribute(attribute);
validate_attribute(attribute, node);

if (is_expression) {
const expression = attribute.value[0].expression;
const expression = get_attribute_expression(attribute);
if (expression.type === 'SequenceExpression') {
let i = /** @type {number} */ (expression.start);
while (--i > 0) {
Expand All @@ -146,7 +164,7 @@ function validate_element(node, context) {
e.attribute_invalid_event_handler(attribute);
}

const value = attribute.value[0].expression;
const value = get_attribute_expression(attribute);
if (
value.type === 'Identifier' &&
value.name === attribute.name &&
Expand Down
Loading
Loading