From 2d378bb76284d19ec843482086cf5d631ba238a7 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Tue, 23 Apr 2024 15:29:02 +0200 Subject: [PATCH] breaking: disallow binding to component exports in runes mode (#11238) * breaking: disallow binding to component exports in runes mode Svelte 4 allowed you to have `export const foo = ..` in component A and then do ``. This is confusing because it's not clear whether the binding is for a property or an export, and we have to sanitize rest props from the export bindings. This PR therefore introduces a breaking change in runes mode: You cannot bind to these exports anymore. Instead use `` and then do `a.foo` - makes things easier to reason about. * Update sites/svelte-5-preview/src/routes/docs/content/03-appendix/02-breaking-changes.md Co-authored-by: Rich Harris * tweak messages * fix tests * use component.name * oops --------- Co-authored-by: Rich Harris --- .changeset/beige-seas-share.md | 5 +++++ .../3-transform/client/transform-client.js | 14 ++++++++++-- .../client/visitors/javascript-runes.js | 3 ++- .../3-transform/server/transform-server.js | 22 ++----------------- .../svelte/src/internal/client/validate.js | 22 ++++++++++++++----- .../samples/export-binding/Counter.svelte | 12 ---------- .../samples/export-binding/_config.js | 15 ++++--------- .../export-binding/counter/index.svelte | 8 +++++++ .../samples/export-binding/main.svelte | 4 ++-- .../props-not-bindable-spread/_config.js | 5 +++-- .../samples/props-not-bindable/_config.js | 5 +++-- .../03-appendix/02-breaking-changes.md | 4 ++-- 12 files changed, 59 insertions(+), 60 deletions(-) create mode 100644 .changeset/beige-seas-share.md delete mode 100644 packages/svelte/tests/runtime-runes/samples/export-binding/Counter.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/export-binding/counter/index.svelte diff --git a/.changeset/beige-seas-share.md b/.changeset/beige-seas-share.md new file mode 100644 index 0000000000..3758385c55 --- /dev/null +++ b/.changeset/beige-seas-share.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +breaking: disallow binding to component exports in runes mode diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index f1de213a9b..70f981d3e6 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js @@ -255,14 +255,24 @@ export function client_component(source, analysis, options) { ); if (analysis.runes && options.dev) { - const bindable = analysis.exports.map(({ name, alias }) => b.literal(alias ?? name)); + const exports = analysis.exports.map(({ name, alias }) => b.literal(alias ?? name)); + /** @type {import('estree').Literal[]} */ + const bindable = []; for (const [name, binding] of properties) { if (binding.kind === 'bindable_prop') { bindable.push(b.literal(binding.prop_alias ?? name)); } } instance.body.unshift( - b.stmt(b.call('$.validate_prop_bindings', b.id('$$props'), b.array(bindable))) + b.stmt( + b.call( + '$.validate_prop_bindings', + b.id('$$props'), + b.array(bindable), + b.array(exports), + b.id(`${analysis.name}`) + ) + ) ); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js index 7646ae78b7..5c5b177917 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js @@ -222,7 +222,8 @@ export const javascript_visitors_runes = { if (rune === '$props') { assert.equal(declarator.id.type, 'ObjectPattern'); - const seen = state.analysis.exports.map(({ name, alias }) => alias ?? name); + /** @type {string[]} */ + const seen = []; for (const property of declarator.id.properties) { if (property.type === 'Property') { diff --git a/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js b/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js index d5402351c7..4f631b8c70 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js @@ -692,8 +692,7 @@ const javascript_visitors_runes = { } if (rune === '$props') { - // remove $bindable() from props declaration and handle rest props - let uses_rest_props = false; + // remove $bindable() from props declaration const id = walk(declarator.id, null, { AssignmentPattern(node) { if ( @@ -705,26 +704,9 @@ const javascript_visitors_runes = { : b.id('undefined'); return b.assignment_pattern(node.left, right); } - }, - RestElement(node, { path }) { - if (path.at(-1) === declarator.id) { - uses_rest_props = true; - } } }); - - const exports = /** @type {import('../../types').ComponentAnalysis} */ ( - state.analysis - ).exports.map(({ name, alias }) => b.literal(alias ?? name)); - - declarations.push( - b.declarator( - id, - uses_rest_props && exports.length > 0 - ? b.call('$.rest_props', b.id('$$props'), b.array(exports)) - : b.id('$$props') - ) - ); + declarations.push(b.declarator(id, b.id('$$props'))); continue; } diff --git a/packages/svelte/src/internal/client/validate.js b/packages/svelte/src/internal/client/validate.js index add59ebe82..b2dfee7ab1 100644 --- a/packages/svelte/src/internal/client/validate.js +++ b/packages/svelte/src/internal/client/validate.js @@ -85,16 +85,26 @@ export function loop_guard(timeout) { /** * @param {Record} $$props * @param {string[]} bindable + * @param {string[]} exports + * @param {Function & { filename: string }} component */ -export function validate_prop_bindings($$props, bindable) { +export function validate_prop_bindings($$props, bindable, exports, component) { for (const key in $$props) { - if (!bindable.includes(key)) { - var setter = get_descriptor($$props, key)?.set; + var setter = get_descriptor($$props, key)?.set; + var name = component.name; - if (setter) { + if (setter) { + if (exports.includes(key)) { throw new Error( - `Cannot use bind:${key} on this component because the property was not declared as bindable. ` + - `To mark a property as bindable, use the $bindable() rune like this: \`let { ${key} = $bindable() } = $props()\`` + `Component ${component.filename} has an export named ${key} that a consumer component is trying to access using bind:${key}, which is disallowed. ` + + `Instead, use bind:this (e.g. <${name} bind:this={component} />) ` + + `and then access the property on the bound component instance (e.g. component.${key}).` + ); + } + if (!bindable.includes(key)) { + throw new Error( + `A component is binding to property ${key} of ${name}.svelte (i.e. <${name} bind:${key} />). This is disallowed because the property was not declared as bindable inside ${component.filename}. ` + + `To mark a property as bindable, use the $bindable() rune in ${name}.svelte like this: \`let { ${key} = $bindable() } = $props()\`` ); } } diff --git a/packages/svelte/tests/runtime-runes/samples/export-binding/Counter.svelte b/packages/svelte/tests/runtime-runes/samples/export-binding/Counter.svelte deleted file mode 100644 index 94c822f31f..0000000000 --- a/packages/svelte/tests/runtime-runes/samples/export-binding/Counter.svelte +++ /dev/null @@ -1,12 +0,0 @@ - - - -{Object.keys(rest).length} - -{count} diff --git a/packages/svelte/tests/runtime-runes/samples/export-binding/_config.js b/packages/svelte/tests/runtime-runes/samples/export-binding/_config.js index aa41757bd1..0f64998059 100644 --- a/packages/svelte/tests/runtime-runes/samples/export-binding/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/export-binding/_config.js @@ -2,16 +2,9 @@ import { test } from '../../test'; export default test({ compileOptions: { - dev: true // to ensure we don't throw a false-positive "cannot bind to this" error + dev: true // to ensure we we catch the error }, - html: `0 0 `, - - async test({ assert, target }) { - const btn = target.querySelector('button'); - - btn?.click(); - await Promise.resolve(); - - assert.htmlEqual(target.innerHTML, `0 1 `); - } + error: + 'Component .../export-binding/counter/index.svelte has an export named increment that a consumer component is trying to access using bind:increment, which is disallowed. ' + + 'Instead, use bind:this (e.g. ) and then access the property on the bound component instance (e.g. component.increment).' }); diff --git a/packages/svelte/tests/runtime-runes/samples/export-binding/counter/index.svelte b/packages/svelte/tests/runtime-runes/samples/export-binding/counter/index.svelte new file mode 100644 index 0000000000..14e0de961b --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/export-binding/counter/index.svelte @@ -0,0 +1,8 @@ + + +{count} diff --git a/packages/svelte/tests/runtime-runes/samples/export-binding/main.svelte b/packages/svelte/tests/runtime-runes/samples/export-binding/main.svelte index 89f0aff9a0..4ad1684701 100644 --- a/packages/svelte/tests/runtime-runes/samples/export-binding/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/export-binding/main.svelte @@ -1,7 +1,7 @@ - \ No newline at end of file + diff --git a/packages/svelte/tests/runtime-runes/samples/props-not-bindable-spread/_config.js b/packages/svelte/tests/runtime-runes/samples/props-not-bindable-spread/_config.js index 4c3fdf5bd9..fc6b46d488 100644 --- a/packages/svelte/tests/runtime-runes/samples/props-not-bindable-spread/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/props-not-bindable-spread/_config.js @@ -5,7 +5,8 @@ export default test({ dev: true }, error: - 'Cannot use bind:count on this component because the property was not declared as bindable. ' + - 'To mark a property as bindable, use the $bindable() rune like this: `let { count = $bindable() } = $props()`', + 'A component is binding to property count of Counter.svelte (i.e. ). This is disallowed because the property was ' + + 'not declared as bindable inside .../samples/props-not-bindable-spread/Counter.svelte. To mark a property as bindable, use the $bindable() rune ' + + 'in Counter.svelte like this: `let { count = $bindable() } = $props()`', html: `0` }); diff --git a/packages/svelte/tests/runtime-runes/samples/props-not-bindable/_config.js b/packages/svelte/tests/runtime-runes/samples/props-not-bindable/_config.js index 4c3fdf5bd9..b3dcc7f23e 100644 --- a/packages/svelte/tests/runtime-runes/samples/props-not-bindable/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/props-not-bindable/_config.js @@ -5,7 +5,8 @@ export default test({ dev: true }, error: - 'Cannot use bind:count on this component because the property was not declared as bindable. ' + - 'To mark a property as bindable, use the $bindable() rune like this: `let { count = $bindable() } = $props()`', + 'A component is binding to property count of Counter.svelte (i.e. ). This is disallowed because the property was ' + + 'not declared as bindable inside .../samples/props-not-bindable/Counter.svelte. To mark a property as bindable, use the $bindable() rune ' + + 'in Counter.svelte like this: `let { count = $bindable() } = $props()`', html: `0` }); diff --git a/sites/svelte-5-preview/src/routes/docs/content/03-appendix/02-breaking-changes.md b/sites/svelte-5-preview/src/routes/docs/content/03-appendix/02-breaking-changes.md index 25547f72ab..43a5c7134c 100644 --- a/sites/svelte-5-preview/src/routes/docs/content/03-appendix/02-breaking-changes.md +++ b/sites/svelte-5-preview/src/routes/docs/content/03-appendix/02-breaking-changes.md @@ -121,9 +121,9 @@ Content inside component tags becomes a [snippet prop](/docs/snippets) called `c Some breaking changes only apply once your component is in runes mode. -### Bindings to component exports don't show up in rest props +### Bindings to component exports are not allowed -In runes mode, bindings to component exports don't show up in rest props. For example, `rest` in `let { foo, bar, ...rest } = $props();` would not contain `baz` if `baz` was defined as `export const baz = ...;` inside the component. In Svelte 4 syntax, the equivalent to `rest` would be `$$restProps`, which contains these component exports. +Exports from runes mode components cannot be bound to directly. For example, having `export const foo = ...` in component `A` and then doing `` causes an error. Use `bind:this` instead — `` — and access the export as `a.foo`. This change makes things easier to reason about, as it enforces a clear separation between props and exports. ### Bindings need to be explicitly defined using `$bindable()`