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

[Svelte 5] Mutating non-bound props lead to undefined behavior ($binds rune proposal) #9997

Closed
GauBen opened this issue Dec 24, 2023 · 6 comments

Comments

@GauBen
Copy link
Contributor

GauBen commented Dec 24, 2023

Describe the bug

Given a parent:

<Child {value} />

And a child that can mutate its value prop:

<script>
	let {value} = $props()
	$inspect(value)
</script>

<input bind:value />

Updating the value in the child will skip $inspect but will properly update value (see reproduction)

The documentation reads

Props cannot be mutated, unless the parent component uses bind:. During development, attempts to mutate props will result in an error.

The behavior described above is not raising an error during development. However, I believe this should be enforced at compile time. Mutations have always created undefined behaviors in Svelte (mutations are always possible but not reactive) and it should be possible to solve it in Svelte 5 by forcing either immutability or bindings:

<script>
	const {size} = $props() // Cannot use `let` to declare props
	let {value} = $binds() // New rune idea
</script>

<input bind:value {size} />

Writing <Child {value} {size} /> would cause a "value must be bound" error

In the case of objects, I guess we could step a bit away from usual js where props are always mutable:

const { obj } = $props()
// This would work in js but should not compile in svelte
obj.prop = 1
// Compile error: cannot update `obj` as it is a constant prop

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACo2QT2uEMBDFv8o0LKjgn3tQYdtTocfemh7cOFsDMQlJtCzid2-M7Zaye-glMHlv3sz8FnIWEh2hbwtR3YiEkqMxJCf-YrbCzSg9htrpyfLtp3bcCuNbppgXo9HWw7Myk4ez1SMkZRWrcm9MNpdED3MnJ4QGDs53HtMkyZiqq98oVZv2RfNOQjRQqEUMPQnV0725auvKtNH5NAjZB88-eIn6ejUEy0NRMPU6CAefQko44dDNCFxbi9zLC40pj3pSPfA_WXfmFUV4AoFR9-IssCfU2zAuvwKLnf9FtsH4WTjgMFYbl2abchDKmbBeGtUbPjc8tIpfzZJm0LThOOW0xFLqj5SRb5GRfEefrdXdK97XL6KIi9ECAgAA

Logs

No response

System Info

svelte 5-rc.26

Severity

annoyance

Edit: merry Christmas! 🎅

@GauBen GauBen changed the title [Svelte 5] Mutating non-bound props lead to undefined behavior [Svelte 5] Mutating non-bound props lead to undefined behavior ($binds run proposal) Dec 24, 2023
@GauBen
Copy link
Contributor Author

GauBen commented Dec 24, 2023

Regarding #9995, this proposal would make the solution explicit but not very elegant

// Parent
<Middle bind:ref color="red" />

// Middle component
<script>
	const {...props} = $props()
	let {ref} = $binds()
</script>

<Button bind:ref {...props} />

// Button
<script>
	const {color} = $props()
	let {ref} = $binds()
</script>

<div bind:this={ref} style:color />

@dm-de
Copy link

dm-de commented Dec 25, 2023

value must be bound

This is very bad... You need to understand why.
And by the way. Svelte 5 have missing default prop value for bind!!!

In Svelte, a parent bind:value can do 2 things:
A) control child prop with parent prop - this is normal for any prop - even if you don't bind it and use only {value}
B) parent can "listen" to child value (two-way-binding) - so value move up from child to parent

With this, you have 4 constellations:

1) What, if I don't like to listen or control prop?
-> I should NOT use bind:value or {bind}. Simple do not pass any value
<Child />
I this case - child prop must use a default value for prop
EXAMPLE

2) What, if only want to control child prop from parent prop?
-> I should use {value} and not bind:value
In this case, child value change does not reflected to parent
EXAMPLE

3) What, if only want to "listen" (read only) child prop from parent prop?
-> I should use bind:value and not {value}
In this case, child value reflect to parent
EXAMPLE

4) What, if I need to listen and to control?
-> I should use bind:value and not {value}
Changes are reflected from child to parent and opposite.
EXAMPLE

And... here is a problem today with Svelte 5 bind:value and default prop:
EXAMPLE

@GauBen GauBen changed the title [Svelte 5] Mutating non-bound props lead to undefined behavior ($binds run proposal) [Svelte 5] Mutating non-bound props lead to undefined behavior ($binds rune proposal) Dec 25, 2023
@GauBen
Copy link
Contributor Author

GauBen commented Dec 25, 2023

Thanks for your input @dm-de, I'm glad you're taking the time to make a detailed response, although I'm not sure to understand everything. I honestly know that the $binds() rune proposal is bad, but I'm drafting an idea hoping Svelte maintainers will have a better one to improve the current state of $props(), bind: and all. I'm fully prepared for a response like this one below, and I wouldn't even be mad, it's too damn funny.

image

Anyway, Rich Harris made a long stance on props mutability reactivity and potential bugs in #9739.

If two unrelated components both have references to some shared state, then mutations in one might cause unexpected updates in the other. [...] Our plan to prevent this category of errors is to make reactive objects readonly in child components.

Of course, if you want to allow the child to update a value given as prop then you can do so with bind:. (This is sort of what happens in Svelte 4 today, except that it's an illusion — in Svelte 4, the child can mutate the object but the parent just won't 'notice', resulting in differing views of reality. This is very bad.)

There is, at this time of writing (next.26), a discrepancy between scalars (this is the bug described in the first message) and objects. While objects are made readonly thanks to proxying, scalars are mutable but somehow not reactive.

The documentation reads this inaccurate statement:

Props cannot be mutated, unless the parent component uses bind:. During development, attempts to mutate props will result in an error.

This is only true for objects, not scalars.

You made 4 great demos, but some of them are problematic:

  1. Perfectly fine, that's how props with default values work
  2. This one is problematic: you couldn't do that with objects. You are updating a copy of the prop, not the prop itself, and it somehow breaks reactivity
  3. All good
  4. Fine too, not much different from 3

I'm very divided regarding 2. I don't like the idea of having no way for a component to tell its parent that a value can be mutated and needs to be bound:

<script>
  // There is no way for a component to enforce `bind:` on the parent
  let { iWillChange, iWont } = $props()

  // Updating iWillChange will never be an issue, but updating iWillChange.key
  // will throw if the parent did not bind the prop
</script>

Proposal:

<script>
  let { iWillChange } = $binds()
  const { iWont } = $props()
  // No ambiguity at all!
</script>

What if you want to provide a value but don't want to keep track of it? Well, you'll be forced to bind: it so you will need to make a copy (see demo above, I wrote the bug and the copy in the same repl)

<script>
  // In +page.svelte
  const {data} = $props()

  // Make an editable copy
  let name = $state(data.name)
</script>

<!-- <input {value} /> wouldn't be possible anymore, I'm pretty sure no maintainers will allow it -->
<input bind:value={name} />

It might also be worth enforcing const ... = $derived(...), as it is not possible to update derived values.

image

I guess I want $props() to be like $derived() for parent states.

This would make a nice symmetry:

Mutable Immutable
Inside a component $state $derived
Parent to child $binds $props

@dm-de
Copy link

dm-de commented Dec 25, 2023

I don't fully understood this before all the time.
But with all proxies and so on... and because, we can not set default value for bind prop in $props...

I think in Svelte 5, this are not two states, this is ONE SINGLE state shared between parent and child:

PARENT:

<script>
    let value = $state(123)
</script>
<Child bind:value />

CHILD:

<script>
    let {value} = $props()
</script>

So, if i change child value, i change parent value at same time without data transfer.
Right?

If all this is correct, then I can not bind derived value (it is immutable).
And I tested this, and this is true!

If $props accept all types of props (bind or not),
then it is possible to do this:

let {...rest} = $props()

FULL EXAMPLE

Here is only one question:
Is it possible and is it useful to make all props immutable, except bound-values?

Parent:
<Child bind:foo {bar} />

Child:
let {foo, bar} = $props()

where foo is mutable
and bar is immutable
automatically

And what, if I use child differently at same time:

<Child bind:foo {bar} />
<Child {foo} {bar} />

Something bad can happen! Because Child expect a mutable "foo".

So... We have 2 options to fix that problem!

  1. Leave Svelte 5 system as it is. All child props are always mutable!

  2. Change Svelte 5 system:
    -add new $binds() and make only this values mutable
    -make $props values always immutable

Personally, i prefer solution 1 - current Svelte system.
It's not worth it to change.

@GauBen
Copy link
Contributor Author

GauBen commented Dec 25, 2023

So, if i change child value, i change parent value at same time without data transfer.

Not really, IIRC Svelte 5 uses signals, which behave roughly the same as the writable store from svelte 4. You can imagine that parent is compiled to something like this:

<script>
    import {writable} from 'svelte/store'
    let value = writable(123) // $state() gets compiled to writable()
</script>
<Child bind:value />

$state implementation

This means that there is indeed data transfer between the parent and the child, made completely invisible by the compiler.

If all this is correct, then I can not bind derived value (it is immutable).

This is not really related to $props, $derived are one-way by design. You can't do that either

<script>
    let name = $state('foo')
    let double = $derived(name + name)
</script>

<input bind:value={double} /> <!-- Can't do this, double is immutable -->

Something bad can happen! Because Child expect a mutable "foo".

100%, that's why I'd like a way for a component to announce which props it might mutate

So... We have 2 options to fix that problem!

Maybe more!

  1. Is not really doable, as Rich Harris explains in Proxied state #9739, this leads to buggy behaviors. That's why they settled for readonly objects
  2. This makes things explicit at the expense of making things more tedious to write, which is not very svelty
  3. Let's wait a few days for a maintainer to chime in and suggest a better idea (🤞)

I completely get why you prefer option 1, but I shot my foot today because of how things are implemented

@dummdidumm
Copy link
Member

Closing in favor of #10768 - it will describe what is bindable, though not what is required to be bound; that one we probably won't add.

@dummdidumm dummdidumm closed this as not planned Won't fix, can't repro, duplicate, stale Mar 12, 2024
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

No branches or pull requests

3 participants