Skip to content

breaking: disallow binding to component exports in runes mode #11238

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 7 commits into from
Apr 23, 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/beige-seas-share.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

breaking: disallow binding to component exports in runes mode
Original file line number Diff line number Diff line change
Expand Up @@ -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}`)
)
)
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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;
}

Expand Down
22 changes: 16 additions & 6 deletions packages/svelte/src/internal/client/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,26 @@ export function loop_guard(timeout) {
/**
* @param {Record<string, any>} $$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()\``
);
}
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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 <button>increment</button>`,

async test({ assert, target }) {
const btn = target.querySelector('button');

btn?.click();
await Promise.resolve();

assert.htmlEqual(target.innerHTML, `0 1 <button>increment</button>`);
}
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. <Counter bind:this={component} />) and then access the property on the bound component instance (e.g. component.increment).'
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<script>
let count = $state(0);
export function increment() {
count++;
}
</script>

{count}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<script>
import Counter from './Counter.svelte';
import Counter from './counter/index.svelte';
let increment;
</script>

<Counter bind:increment={increment} />
<button onclick={increment}>increment</button>
<button onclick={increment}>increment</button>
Original file line number Diff line number Diff line change
Expand Up @@ -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. <Counter bind:count />). 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`
});
Original file line number Diff line number Diff line change
Expand Up @@ -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. <Counter bind:count />). 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`
});
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<A bind:foo />` causes an error. Use `bind:this` instead — `<A bind:this={a} />` — 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()`

Expand Down