Skip to content

breaking: add $bindable() rune to denote bindable props #10851

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 30 commits into from
Mar 22, 2024
Merged

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Mar 20, 2024

Alternative to / closes #10804
closes #10768
closes #10711

Initially I was against let { prop = $bindable() } = $props() because it felt too weird to use a default initializer as marking a prop as bindable. But after implementing let { prop } = $props.bindable() I experienced a couple of flaws of this approach which all come down to there now being two runes which take properties from the same bag:

  • you can destructure the same value from both $props() and $props.bindable()
  • you can destructure one property from one prop, and use a ...rest parameter on the other prop, which then also includes that one property you destructured in the other one
  • you can type this wrong very easily. $props() and $props.bindable() have the same shape but there are no restrictions to actually type them that way

Fueled by the weirdness I implemented the let { prop = $bindable() } = $props() which we initially rejected as a close second - and after actually writing the code instead of just thinking about it theoretically it actually feels much nicer than $props.bindable(). The default assignment is a bit weird but I got used to it in no time. I therefore propose to go through with this variant.

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

Copy link

changeset-bot bot commented Mar 20, 2024

⚠️ No Changeset found

Latest commit: bf0a830

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@@ -490,14 +490,24 @@ let { a, b, c, ...everythingElse }: MyProps = $props();
>
> ...TypeScript [widens the type](https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwBIAHGHIgZwB4AVeAXnilQE8A+ACgEoAueagbgBQgiCAzwA3vAAe9eABYATPAC+c4qQqUp03uQwwsqAOaqOnIfCsB6a-AB6AfiA) of `x` to be `string | number`, instead of erroring.

Props cannot be mutated, unless the parent component uses `bind:`. During development, attempts to mutate props will result in an error.
By default props are treated as readonly, meaning reassignments will not propagate upwards and mutations will result in a warning. You will also get an error trying to `bind:` to them. To declare props as bindable, use [`$bindable()`](#bindable).
Copy link
Member

Choose a reason for hiding this comment

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

Are reassignments also a warning, or do they just silently not work? It's also not clear to me here that these are dev-time-only warning (which I assume they are) - and I'm not clear on which things will result in a warning versus result in an error, and whether that's a runtime error/warning (I assume so) or a compile-time error/warning (I assume not, unless you're relying on types).

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know if the refined sentences are more clear.

* tweak error message for readonly rest props

* remove property in prod

* Update packages/svelte/src/internal/client/reactivity/props.js

---------

Co-authored-by: Simon H <[email protected]>
@@ -490,14 +490,34 @@ let { a, b, c, ...everythingElse }: MyProps = $props();
>
> ...TypeScript [widens the type](https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwBIAHGHIgZwB4AVeAXnilQE8A+ACgEoAueagbgBQgiCAzwA3vAAe9eABYATPAC+c4qQqUp03uQwwsqAOaqOnIfCsB6a-AB6AfiA) of `x` to be `string | number`, instead of erroring.

Props cannot be mutated, unless the parent component uses `bind:`. During development, attempts to mutate props will result in an error.
By default props are treated as readonly, meaning reassignments will not propagate upwards and mutations will result in a warning at runtime in development mode. You will also get a runtime error when trying to `bind:` to a readonly prop in a parent component. To declare props as bindable, use [`$bindable()`](#bindable).
Copy link
Member

Choose a reason for hiding this comment

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

In pure Markdown the link appears to work, in the generated docs the ID of the heading is $props-$bindable, though, and this link is not adjusted.

All the links on the right side also generate this alert for when you navigate from the REPL to the docs:
image

@brunnerh
Copy link
Member

Isn't this missing a changeset?

@dummdidumm
Copy link
Member Author

You're right we missed that, but too late, next version is already out so we can only adjust the changelogs manually now

@KilDesu
Copy link

KilDesu commented Apr 9, 2024

Wouldn't that make bindable props optional though? How would you make them required?

@brunnerh
Copy link
Member

brunnerh commented Apr 9, 2024

It does seem to change the default inference, not sure if that could be adjusted/fixed 🤔.

Using types, properties can be explicitly marked as required/optional, though.
E.g. using JSDoc if using plain JS:

/** @type {{
 *   req: string,
 *   notRequired?: string,
 * }}*/
let {
  req = $bindable(),
  notRequired = $bindable(),
} = $props();

(Note the ? after notRequired.)

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.

Explicit bindings with $props.bindable() Svelte 5: Allow updating the value of restProps properties if they are bind to
8 participants