svelte icon indicating copy to clipboard operation
svelte copied to clipboard

Svelte 5: Tell if a bindable prop is actually passed using bind

Open jamesst20 opened this issue 9 months ago • 12 comments

Describe the problem

Svelte 5 warns when non-binded props are mutated which is a very good thing to avoid unexpected side effect.

However, there is no way to tell if a prop is actually binded or not, or at least if there is it doesn't appear to be documented.

Shallow/deep copy could be avoided in some scenarios where performance matter if it's not even used.

Describe the proposed solution

Here is a real life possible scenario where you might just want to call a callback with the new values without mutating the actual passed value.


<!-- MyComponent.svelte -->
<script>
let { values = $bindable(), onChange } = $props();

const addValue = (newValue) => {
  if (isBindedProp(values)) {
    values ||= [];
    values.push(newValue);
    onChange?.(values);
  } else {
    onChange?.([...(values || []), newValue]);
  }  
};
</script>

....

<!-- Somewhere -->
<MyComponent bind:values={...}>

<!-- Somewhere else -->
<MyComponent values={...} onChange{...}>

Importance

must have.

Svelte 4 librairies should be able to tell that as well to preserve compatibility without switching to runes

jamesst20 avatar May 17 '24 16:05 jamesst20

What's the advantage of doing this instead of just doing


<!-- MyComponent.svelte -->
<script>
let { values = $bindable(), onChange } = $props();

const addValue = (newValue) => {
    values ||= [];
    values.push(newValue);
    onChange?.(values);
};
</script>

....

<!-- Somewhere -->
<MyComponent bind:values={...}>

<!-- Somewhere else -->
<MyComponent values={...} onChange{...}>

this way you also avoid creating new memory by spreading.

paoloricciuti avatar May 17 '24 16:05 paoloricciuti

What's the advantage of doing this instead of just doing

<!-- MyComponent.svelte -->
<script>
let { values = $bindable(), onChange } = $props();

const addValue = (newValue) => {
    values ||= [];
    values.push(newValue);
    onChange?.(values);
};
</script>

....

<!-- Somewhere -->
<MyComponent bind:values={...}>

<!-- Somewhere else -->
<MyComponent values={...} onChange{...}>

this way you also avoid creating new memory by spreading.

You can test it out, it triggers a warning and it has side effect. It shouldn't update values because I didn't use bind. To get rid of this error, I would be forced to create a "memory spreading" as you call it to avoid that side effect

REPL

"%c[svelte] ownership_invalid_mutation\n%cMyComponent.svelte 
mutated a value owned by App.svelte. This is strongly discouraged. Consider 
passing values to child components with `bind:`, or use a callback instead"

jamesst20 avatar May 17 '24 17:05 jamesst20

How about something like:

let { prop = $bindable.required() }

Maybe a different name like $bindable.must, or even, $bound and $bound.optional. This would be a pretty massive breaking change, but the way I see it optional bindings shouldn't be allowed at all unless there is a way for the child to know when the value is bound to or not (which adds unnecessary complexity). If there is state to be shared from the child to the parent, I believe there are 2 options for the parent:

  1. the parent does not provide the prop at all. The child is independent and manages its own state.
  2. the parent binds to the prop, gaining "ownership" of the value (the child still controls/owns it, but from a logic standpoint it's as if the parent provides it and changes it based on callbacks from the child).

Child components will want to implement one or both of these behaviours:

  • be fully-controlled by a parent
  • manage their own state

If only one of the behaviours is needed, either a callback function (events) or bindable prop with no default is used. If you need both, use a bindable prop with a default value.

The problem is, as a child component, if you have a bindable prop, you will mutate it, otherwise it wouldn't be bindable. I believe the warning doesn't really make sense and might not catch potential issues fast enough (parent doesn't bind the prop but the child rarely mutates it so the warning never pops up). I think there are a few solutions to this:

  1. Add a way to make bindings required
  2. Make all bindings required (it's okay to omit the prop completely if it has a default value) - breaking
  3. Add a way for child components to tell which props are bound to (difficult to use by component authors - should they always check before mutating props?)

Whatever you thoughts on everything I said here are, my only question is: what is the use case for allowing prop={var} (without bind:) on $bindable props? If there is a valid use case, how should the warning in the original question be fixed?

rChaoz avatar May 21 '24 07:05 rChaoz

How about something like:

let { prop = $bindable.required() }

Maybe a different name like $bindable.must, or even, $bound and $bound.optional. This would be a pretty massive breaking change, but the way I see it optional bindings shouldn't be allowed at all unless there is a way for the child to know when the value is bound to or not (which adds unnecessary complexity). If there is state to be shared from the child to the parent, I believe there are 2 options for the parent:

  1. the parent does not provide the prop at all. The child is independent and manages its own state.
  2. the parent binds to the prop, gaining "ownership" of the value (the child still controls/owns it, but from a logic standpoint it's as if the parent provides it and changes it based on callbacks from the child).

Child components will want to implement one or both of these behaviours:

  • be fully-controlled by a parent
  • manage their own state

If only one of the behaviours is needed, either a callback function (events) or bindable prop with no default is used. If you need both, use a bindable prop with a default value.

The problem is, as a child component, if you have a bindable prop, you will mutate it, otherwise it wouldn't be bindable. I believe the warning doesn't really make sense and might not catch potential issues fast enough (parent doesn't bind the prop but the child rarely mutates it so the warning never pops up). I think there are a few solutions to this:

  1. Add a way to make bindings required
  2. Make all bindings required (it's okay to omit the prop completely if it has a default value) - breaking
  3. Add a way for child components to tell which props are bound to (difficult to use by component authors - should they always check before mutating props?)

Whatever you thoughts on everything I said here are, my only question is: what is the use case for allowing prop={var} (without bind:) on $bindable props? If there is a valid use case, how should the warning in the original question be fixed?

Hi,

These are indeed very good suggestions! However, I'm on the fence of disagreement that a bindable prop should always be binded. I believe the very best case scenario should be

  • $bindable() -> optional binding by default
  • $bindable.must() -> mandatory binding (I leave the syntax choice to Svelte maintainers)
  • $isBinded(prop) -> Tell if a prop is binded or not (I leave the syntax choice to Svelte maintainers)

The reason is that I believe it should be possible to have a component that can either manage its own state or manage an external state with bind.

Let's take an exemple of a "Svelte Select Library" named "MySelect"

// Component mutates and always display the current selection based on value prop
<MySelect bind:value={value} options={...}>

// Component uses its own state to manage the value. When the form is submitted, it will be submitted like a normal form input.
<MySelect name="user[firstName]" options={...}>

// Component uses its own state to manage the value. A callback onChange is used to do something with the selected value
<MySelect value={initialValue} options={...} onChange={/* do something */}>

@Rich-Harris I would love to know what you think about this (I hope it's ok to mention you)

jamesst20 avatar May 21 '24 14:05 jamesst20