Skip to content
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

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

Merged
merged 7 commits into from
Apr 23, 2024

Conversation

dummdidumm
Copy link
Member

Svelte 4 allowed you to have export const foo = .. in component A and then do <A bind:foo />. 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 <A bind:this={a} /> and then do a.foo - makes things easier to reason about.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Svelte 4 allowed you to have `export const foo = ..` in component A and then do `<A bind:foo />`. 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 `<A bind:this={a} />` and then do `a.foo` - makes things easier to reason about.
Copy link

changeset-bot bot commented Apr 19, 2024

🦋 Changeset detected

Latest commit: 8d5acf6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member

Does this also result in a type error on the parent component doing bind:increment or whatever?

@dummdidumm
Copy link
Member Author

No, that's part of language tools which I didn't get around to yet

Comment on lines 97 to 98
`Cannot use bind:${key} on this component because it is a component export, and you can only bind to properties in runes mode. ` +
`Use bind:this instead and then access the property on the bound component instance.`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also applies to the existing error message for non-bindable props, but: the message is a bit cryptic because it refers to 'this component' with no further context. 'you can only bind to properties in runes mode' is also slightly cryptic because the parent component is the one doing the binding, so it sounds like it matters which mode the parent is in rather than the child.

Ideally we would specify both parent and child component. Something like

Cannot use bind:foo (in Parent.svelte) to bind to an export from Child.svelte. Instead, use bind:this={component} and access the property as component.foo

and

Cannot use bind:foo (in Parent.svelte) to bind to a non-bindable prop. To make it bindable, use the $bindable rune in Child.svelte: let { foo = $bindable } = $props()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tweaked the messages, let me know if it's clear enough now or if we need to find a way to bring the consumer component name in there.

…breaking-changes.md

Co-authored-by: Rich Harris <rich.harris@vercel.com>
b.id('$$props'),
b.array(bindable),
b.array(exports),
b.id(`${analysis.name}.filename`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh actually you know what? we can just pass the function itself through here and use component.name instead of reverse engineering it from filename. that way it's more likely to match the user's intentions — foo/index.svelte becomes <Foo> rather than <index>

making that change now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name will be App_1 if there's a variable with the name App inside it, so it's not guaranteed to look correct in all cases (but probably fine for 99% of all cases)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants