diff --git a/packages/svelte/messages/client-warnings/warnings.md b/packages/svelte/messages/client-warnings/warnings.md index 643de0ed1d..865d968144 100644 --- a/packages/svelte/messages/client-warnings/warnings.md +++ b/packages/svelte/messages/client-warnings/warnings.md @@ -4,4 +4,10 @@ ## ownership_invalid_binding -> %parent% passed a value to %child% with `bind:`, but the value is owned by %owner%. Consider creating a binding between %owner% and %parent% \ No newline at end of file +> %parent% passed a value to %child% with `bind:`, but the value is owned by %owner%. Consider creating a binding between %owner% and %parent% + +## ownership_invalid_mutation + +> Mutating a value outside the component that created it is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead + +> %component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead diff --git a/packages/svelte/messages/compile-errors/bindings.md b/packages/svelte/messages/compile-errors/bindings.md index 9afa81d1f2..1a7ef806ac 100644 --- a/packages/svelte/messages/compile-errors/bindings.md +++ b/packages/svelte/messages/compile-errors/bindings.md @@ -14,8 +14,6 @@ > `bind:%name%` is not a valid binding -## bind_invalid_detailed - > `bind:%name%` is not a valid binding. %explanation% ## invalid_type_attribute @@ -32,4 +30,4 @@ ## dynamic_contenteditable_attribute -> 'contenteditable' attribute cannot be dynamic if element uses two-way binding \ No newline at end of file +> 'contenteditable' attribute cannot be dynamic if element uses two-way binding diff --git a/packages/svelte/messages/compile-warnings/a11y.md b/packages/svelte/messages/compile-warnings/a11y.md index feb602344d..e520f01aba 100644 --- a/packages/svelte/messages/compile-warnings/a11y.md +++ b/packages/svelte/messages/compile-warnings/a11y.md @@ -6,8 +6,6 @@ > Unknown aria attribute 'aria-%attribute%' -## a11y_unknown_aria_attribute_suggestion - > Unknown aria attribute 'aria-%attribute%'. Did you mean '%suggestion%'? ## a11y_hidden @@ -62,8 +60,6 @@ > Unknown role '%role%' -## a11y_unknown_role_suggestion - > Unknown role '%role%'. Did you mean '%suggestion%'? ## a11y_no_redundant_roles @@ -168,4 +164,4 @@ ## a11y_missing_content -> <%name%> element should have child content \ No newline at end of file +> <%name%> element should have child content diff --git a/packages/svelte/messages/compile-warnings/options.md b/packages/svelte/messages/compile-warnings/options.md index 5513f05f6e..2bd4d7ca7a 100644 --- a/packages/svelte/messages/compile-warnings/options.md +++ b/packages/svelte/messages/compile-warnings/options.md @@ -1,6 +1,6 @@ ## options_deprecated_accessors -The `accessors` option has been deprecated. It will have no effect in runes mode +> The `accessors` option has been deprecated. It will have no effect in runes mode ## options_deprecated_immutable @@ -24,4 +24,4 @@ The `accessors` option has been deprecated. It will have no effect in runes mode ## options_removed_loop_guard_timeout -> The `loopGuardTimeout` option has been removed \ No newline at end of file +> The `loopGuardTimeout` option has been removed diff --git a/packages/svelte/scripts/process-messages/index.js b/packages/svelte/scripts/process-messages/index.js index ace3a5e66c..fc4ac6ee20 100644 --- a/packages/svelte/scripts/process-messages/index.js +++ b/packages/svelte/scripts/process-messages/index.js @@ -4,6 +4,7 @@ import * as acorn from 'acorn'; import { walk } from 'zimmerframe'; import * as esrap from 'esrap'; +/** @type {Record>} */ const messages = {}; const seen = new Set(); @@ -24,12 +25,21 @@ for (const category of fs.readdirSync('messages')) { throw new Error(`Duplicate message code ${category}/${code}`); } + const sections = text.trim().split('\n\n'); + let details = null; + if (!sections[sections.length - 1].startsWith('> ')) { + details = /** @type {string} */ (sections.pop()); + } + + if (sections.length === 0) { + throw new Error('No message text'); + } + seen.add(code); - messages[category][code] = text - .trim() - .split('\n') - .map((line) => line.slice(2)) - .join('\n'); + messages[category][code] = { + messages: sections.map((section) => section.replace(/^> /gm, '')), + details + }; } } } @@ -102,13 +112,89 @@ function transform(name, dest) { ast.body.splice(index, 1); for (const code in category) { - const message = category[code]; + const { messages } = category[code]; const vars = []; - for (const match of message.matchAll(/%(\w+)%/g)) { - const name = match[1]; - if (!vars.includes(name)) { - vars.push(match[1]); + + const group = messages.map((text, i) => { + for (const match of text.matchAll(/%(\w+)%/g)) { + const name = match[1]; + if (!vars.includes(name)) { + vars.push(match[1]); + } + } + + return { + text, + vars: vars.slice() + }; + }); + + /** @type {import('estree').Expression} */ + let message = { type: 'Literal', value: '' }; + let prev_vars; + + for (let i = 0; i < group.length; i += 1) { + const { text, vars } = group[i]; + + if (vars.length === 0) { + message = { + type: 'Literal', + value: text + }; + continue; } + + const parts = text.split(/(%\w+%)/); + + /** @type {import('estree').Expression[]} */ + const expressions = []; + + /** @type {import('estree').TemplateElement[]} */ + const quasis = []; + + for (let i = 0; i < parts.length; i += 1) { + const part = parts[i]; + if (i % 2 === 0) { + const str = part.replace(/(`|\${)/g, '\\$1'); + quasis.push({ + type: 'TemplateElement', + value: { raw: str, cooked: str }, + tail: i === parts.length - 1 + }); + } else { + expressions.push({ + type: 'Identifier', + name: part.slice(1, -1) + }); + } + } + + /** @type {import('estree').Expression} */ + const expression = { + type: 'TemplateLiteral', + expressions, + quasis + }; + + if (prev_vars) { + if (vars.length === prev_vars.length) { + throw new Error('Message overloads must have new parameters'); + } + + message = { + type: 'ConditionalExpression', + test: { + type: 'Identifier', + name: vars[prev_vars.length] + }, + consequent: expression, + alternate: message + }; + } else { + message = expression; + } + + prev_vars = vars; } const clone = walk(/** @type {import('estree').Node} */ (template_node), null, { @@ -120,14 +206,22 @@ function transform(name, dest) { .split('\n') .map((line) => { if (line === ' * MESSAGE') { - return message + return messages[messages.length - 1] .split('\n') .map((line) => ` * ${line}`) .join('\n'); } if (line.includes('PARAMETER')) { - return vars.map((name) => ` * @param {string} ${name}`).join('\n'); + return vars + .map((name, i) => { + const optional = i >= group[0].vars.length; + + return optional + ? ` * @param {string | undefined | null} [${name}]` + : ` * @param {string} ${name}`; + }) + .join('\n'); } return line; @@ -171,44 +265,7 @@ function transform(name, dest) { }, Identifier(node) { if (node.name !== 'MESSAGE') return; - - if (/%\w+%/.test(message)) { - const parts = message.split(/(%\w+%)/); - - /** @type {import('estree').Expression[]} */ - const expressions = []; - - /** @type {import('estree').TemplateElement[]} */ - const quasis = []; - - for (let i = 0; i < parts.length; i += 1) { - const part = parts[i]; - if (i % 2 === 0) { - const str = part.replace(/(`|\${)/g, '\\$1'); - quasis.push({ - type: 'TemplateElement', - value: { raw: str, cooked: str }, - tail: i === parts.length - 1 - }); - } else { - expressions.push({ - type: 'Identifier', - name: part.slice(1, -1) - }); - } - } - - return { - type: 'TemplateLiteral', - expressions, - quasis - }; - } - - return { - type: 'Literal', - value: message - }; + return message; } }); diff --git a/packages/svelte/src/compiler/errors.js b/packages/svelte/src/compiler/errors.js index 096a2ef97c..810c1582be 100644 --- a/packages/svelte/src/compiler/errors.js +++ b/packages/svelte/src/compiler/errors.js @@ -227,25 +227,15 @@ export function bind_invalid_target(node, name, elements) { e(node, "bind_invalid_target", `\`bind:${name}\` can only be used with ${elements}`); } -/** - * `bind:%name%` is not a valid binding - * @param {null | number | NodeLike} node - * @param {string} name - * @returns {never} - */ -export function bind_invalid(node, name) { - e(node, "bind_invalid", `\`bind:${name}\` is not a valid binding`); -} - /** * `bind:%name%` is not a valid binding. %explanation% * @param {null | number | NodeLike} node * @param {string} name - * @param {string} explanation + * @param {string | undefined | null} [explanation] * @returns {never} */ -export function bind_invalid_detailed(node, name, explanation) { - e(node, "bind_invalid_detailed", `\`bind:${name}\` is not a valid binding. ${explanation}`); +export function bind_invalid(node, name, explanation) { + e(node, "bind_invalid", explanation ? `\`bind:${name}\` is not a valid binding. ${explanation}` : `\`bind:${name}\` is not a valid binding`); } /** diff --git a/packages/svelte/src/compiler/phases/2-analyze/a11y.js b/packages/svelte/src/compiler/phases/2-analyze/a11y.js index 0b54af4b85..91dc7cf8fb 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/a11y.js +++ b/packages/svelte/src/compiler/phases/2-analyze/a11y.js @@ -739,12 +739,7 @@ function check_element(node, state) { const type = name.slice(5); if (!aria_attributes.includes(type)) { const match = fuzzymatch(type, aria_attributes); - if (match) { - // TODO allow 'overloads' in messages, so that we can use the same code with and without suggestions - w.a11y_unknown_aria_attribute_suggestion(attribute, type, match); - } else { - w.a11y_unknown_aria_attribute(attribute, type); - } + w.a11y_unknown_aria_attribute(attribute, type, match); } if (name === 'aria-hidden' && regex_heading_tags.test(node.name)) { @@ -792,11 +787,7 @@ function check_element(node, state) { w.a11y_no_abstract_role(attribute, current_role); } else if (current_role && !aria_roles.includes(current_role)) { const match = fuzzymatch(current_role, aria_roles); - if (match) { - w.a11y_unknown_role_suggestion(attribute, current_role, match); - } else { - w.a11y_unknown_role(attribute, current_role); - } + w.a11y_unknown_role(attribute, current_role, match); } // no-redundant-roles diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index 55ae8c64d2..eaa486cd21 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -373,7 +373,7 @@ const validation = { parent?.type === 'SvelteBody' ) { if (context.state.options.namespace === 'foreign' && node.name !== 'this') { - e.bind_invalid_detailed(node, node.name, 'Foreign elements only support `bind:this`'); + e.bind_invalid(node, node.name, 'Foreign elements only support `bind:this`'); } if (node.name in binding_properties) { @@ -442,7 +442,7 @@ const validation = { if (match) { const property = binding_properties[match]; if (!property.valid_elements || property.valid_elements.includes(parent.name)) { - e.bind_invalid_detailed(node, node.name, `Did you mean '${match}'?`); + e.bind_invalid(node, node.name, `Did you mean '${match}'?`); } } e.bind_invalid(node, node.name); diff --git a/packages/svelte/src/compiler/warnings.js b/packages/svelte/src/compiler/warnings.js index 2d1d80c403..1dbfcf9705 100644 --- a/packages/svelte/src/compiler/warnings.js +++ b/packages/svelte/src/compiler/warnings.js @@ -49,23 +49,14 @@ export function a11y_aria_attributes(node, name) { w(node, "a11y_aria_attributes", `<${name}> should not have aria-* attributes`); } -/** - * Unknown aria attribute 'aria-%attribute%' - * @param {null | NodeLike} node - * @param {string} attribute - */ -export function a11y_unknown_aria_attribute(node, attribute) { - w(node, "a11y_unknown_aria_attribute", `Unknown aria attribute 'aria-${attribute}'`); -} - /** * Unknown aria attribute 'aria-%attribute%'. Did you mean '%suggestion%'? * @param {null | NodeLike} node * @param {string} attribute - * @param {string} suggestion + * @param {string | undefined | null} [suggestion] */ -export function a11y_unknown_aria_attribute_suggestion(node, attribute, suggestion) { - w(node, "a11y_unknown_aria_attribute_suggestion", `Unknown aria attribute 'aria-${attribute}'. Did you mean '${suggestion}'?`); +export function a11y_unknown_aria_attribute(node, attribute, suggestion) { + w(node, "a11y_unknown_aria_attribute", suggestion ? `Unknown aria attribute 'aria-${attribute}'. Did you mean '${suggestion}'?` : `Unknown aria attribute 'aria-${attribute}'`); } /** @@ -178,23 +169,14 @@ export function a11y_no_abstract_role(node, role) { w(node, "a11y_no_abstract_role", `Abstract role '${role}' is forbidden`); } -/** - * Unknown role '%role%' - * @param {null | NodeLike} node - * @param {string} role - */ -export function a11y_unknown_role(node, role) { - w(node, "a11y_unknown_role", `Unknown role '${role}'`); -} - /** * Unknown role '%role%'. Did you mean '%suggestion%'? * @param {null | NodeLike} node * @param {string} role - * @param {string} suggestion + * @param {string | undefined | null} [suggestion] */ -export function a11y_unknown_role_suggestion(node, role, suggestion) { - w(node, "a11y_unknown_role_suggestion", `Unknown role '${role}'. Did you mean '${suggestion}'?`); +export function a11y_unknown_role(node, role, suggestion) { + w(node, "a11y_unknown_role", suggestion ? `Unknown role '${role}'. Did you mean '${suggestion}'?` : `Unknown role '${role}'`); } /** @@ -545,11 +527,11 @@ export function invalid_self_closing_tag(node, name) { } /** - * e `accessors` option has been deprecated. It will have no effect in runes mode + * The `accessors` option has been deprecated. It will have no effect in runes mode * @param {null | NodeLike} node */ export function options_deprecated_accessors(node) { - w(node, "options_deprecated_accessors", "e `accessors` option has been deprecated. It will have no effect in runes mode"); + w(node, "options_deprecated_accessors", "The `accessors` option has been deprecated. It will have no effect in runes mode"); } /** diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index 4fac7837fd..e9fe0f4fb4 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -225,18 +225,13 @@ export function check_ownership(metadata) { if (component && !has_owner(metadata, component)) { let original = get_owner(metadata); - let message = + // @ts-expect-error + if (original.filename !== component.filename) { // @ts-expect-error - original.filename !== component.filename - ? // @ts-expect-error - `${component.filename} mutated a value owned by ${original.filename}. This is strongly discouraged` - : 'Mutating a value outside the component that created it is strongly discouraged'; - - // TODO get rid of this, but implement message overloads first - // eslint-disable-next-line no-console - console.warn( - `${message}. Consider passing values to child components with \`bind:\`, or use a callback instead.` - ); + w.ownership_invalid_mutation(component.filename, original.filename); + } else { + w.ownership_invalid_mutation(); + } // eslint-disable-next-line no-console console.trace(); diff --git a/packages/svelte/src/internal/client/warnings.js b/packages/svelte/src/internal/client/warnings.js index 66d72758bf..eec28ac1c6 100644 --- a/packages/svelte/src/internal/client/warnings.js +++ b/packages/svelte/src/internal/client/warnings.js @@ -30,4 +30,18 @@ export function ownership_invalid_binding(parent, child, owner) { // TODO print a link to the documentation console.warn("ownership_invalid_binding"); } +} + +/** + * %component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead + * @param {string | undefined | null} [component] + * @param {string | undefined | null} [owner] + */ +export function ownership_invalid_mutation(component, owner) { + if (DEV) { + console.warn(`%c[svelte] ${"ownership_invalid_mutation"}\n%c${`${component} mutated a value owned by ${owner}. This is strongly discouraged. Consider passing values to child components with \`bind:\`, or use a callback instead`}`, bold, normal); + } else { + // TODO print a link to the documentation + console.warn("ownership_invalid_mutation"); + } } \ No newline at end of file diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js index 7a78630da5..5cef52a86b 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js @@ -1,14 +1,8 @@ import { test } from '../../test'; -/** @type {typeof console.warn} */ -let warn; - /** @type {typeof console.trace} */ let trace; -/** @type {any[]} */ -let warnings = []; - export default test({ html: ``, @@ -17,31 +11,22 @@ export default test({ }, before_test: () => { - warn = console.warn; trace = console.trace; - - console.warn = (...args) => { - warnings.push(...args); - }; - console.trace = () => {}; }, after_test: () => { - console.warn = warn; console.trace = trace; - - warnings = []; }, - async test({ assert, target }) { + async test({ assert, target, warnings }) { const btn = target.querySelector('button'); await btn?.click(); assert.htmlEqual(target.innerHTML, ``); assert.deepEqual(warnings, [ - '.../samples/non-local-mutation-discouraged/Counter.svelte mutated a value owned by .../samples/non-local-mutation-discouraged/main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead.' + '.../samples/non-local-mutation-discouraged/Counter.svelte mutated a value owned by .../samples/non-local-mutation-discouraged/main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead' ]); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/_config.js index 0d075cca25..7f0581d6c4 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/_config.js @@ -1,31 +1,12 @@ import { tick } from 'svelte'; import { test } from '../../test'; -/** @type {typeof console.warn} */ -let warn; - -/** @type {any[]} */ -let warnings = []; - export default test({ compileOptions: { dev: true }, - before_test: () => { - warn = console.warn; - - console.warn = (...args) => { - warnings.push(...args); - }; - }, - - after_test: () => { - console.warn = warn; - warnings = []; - }, - - async test({ assert, target }) { + async test({ assert, target, warnings }) { const [btn1, btn2] = target.querySelectorAll('button'); await btn1.click(); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/_config.js index df3ca08f03..ec8e5ada28 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/_config.js @@ -1,31 +1,12 @@ import { tick } from 'svelte'; import { test } from '../../test'; -/** @type {typeof console.warn} */ -let warn; - -/** @type {any[]} */ -let warnings = []; - export default test({ compileOptions: { dev: true }, - before_test: () => { - warn = console.warn; - - console.warn = (...args) => { - warnings.push(...args); - }; - }, - - after_test: () => { - console.warn = warn; - warnings = []; - }, - - async test({ assert, target }) { + async test({ assert, target, warnings }) { const [btn1, btn2] = target.querySelectorAll('button'); btn1.click(); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js index 6fc699efb6..fd0ace4fda 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js @@ -33,7 +33,7 @@ export default test({ assert.htmlEqual(target.innerHTML, ``); assert.deepEqual(warnings, [ - '.../samples/non-local-mutation-with-binding-3/Counter.svelte mutated a value owned by .../samples/non-local-mutation-with-binding-3/main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead.' + '.../samples/non-local-mutation-with-binding-3/Counter.svelte mutated a value owned by .../samples/non-local-mutation-with-binding-3/main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead' ]); } }); diff --git a/packages/svelte/tests/validator/samples/a11y-aria-props/warnings.json b/packages/svelte/tests/validator/samples/a11y-aria-props/warnings.json index cd36ffd42d..bdb7a976a2 100644 --- a/packages/svelte/tests/validator/samples/a11y-aria-props/warnings.json +++ b/packages/svelte/tests/validator/samples/a11y-aria-props/warnings.json @@ -1,6 +1,6 @@ [ { - "code": "a11y_unknown_aria_attribute_suggestion", + "code": "a11y_unknown_aria_attribute", "message": "Unknown aria attribute 'aria-labeledby'. Did you mean 'labelledby'?", "start": { "line": 1, diff --git a/packages/svelte/tests/validator/samples/a11y-aria-role/warnings.json b/packages/svelte/tests/validator/samples/a11y-aria-role/warnings.json index 3b250958b0..a0133cc1bb 100644 --- a/packages/svelte/tests/validator/samples/a11y-aria-role/warnings.json +++ b/packages/svelte/tests/validator/samples/a11y-aria-role/warnings.json @@ -1,6 +1,6 @@ [ { - "code": "a11y_unknown_role_suggestion", + "code": "a11y_unknown_role", "message": "Unknown role 'toooltip'. Did you mean 'tooltip'?", "start": { "line": 6, @@ -12,7 +12,7 @@ } }, { - "code": "a11y_unknown_role_suggestion", + "code": "a11y_unknown_role", "message": "Unknown role 'toooltip'. Did you mean 'tooltip'?", "start": { "line": 7, diff --git a/packages/svelte/tests/validator/samples/binding-invalid-foreign-namespace/errors.json b/packages/svelte/tests/validator/samples/binding-invalid-foreign-namespace/errors.json index de997e5394..aa2d7c1621 100644 --- a/packages/svelte/tests/validator/samples/binding-invalid-foreign-namespace/errors.json +++ b/packages/svelte/tests/validator/samples/binding-invalid-foreign-namespace/errors.json @@ -1,6 +1,6 @@ [ { - "code": "bind_invalid_detailed", + "code": "bind_invalid", "message": "`bind:value` is not a valid binding. Foreign elements only support `bind:this`", "start": { "line": 6, diff --git a/packages/svelte/tests/validator/samples/window-binding-invalid-innerwidth/errors.json b/packages/svelte/tests/validator/samples/window-binding-invalid-innerwidth/errors.json index 99a1c892d3..a63e8a0877 100644 --- a/packages/svelte/tests/validator/samples/window-binding-invalid-innerwidth/errors.json +++ b/packages/svelte/tests/validator/samples/window-binding-invalid-innerwidth/errors.json @@ -1,6 +1,6 @@ [ { - "code": "bind_invalid_detailed", + "code": "bind_invalid", "message": "`bind:innerwidth` is not a valid binding. Did you mean 'innerWidth'?", "start": { "line": 5,