Skip to content

Svelte 5: disallow fallback values for bindings #9764

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

Closed
Rich-Harris opened this issue Dec 4, 2023 · 22 comments · Fixed by #9784
Closed

Svelte 5: disallow fallback values for bindings #9764

Rich-Harris opened this issue Dec 4, 2023 · 22 comments · Fixed by #9784
Assignees

Comments

@Rich-Harris
Copy link
Member

Describe the problem

More of an anti-feature request: we resolved to disallow fallback values for bindings in runes mode. They're confusing, and a source of bugginess and implementation complexity

Describe the proposed solution

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE42PwWrDMBBEf2URhTg02M1VtQ0hn9H0EMtrqkaWhLQuFKF_ryyFNKU9lD2NtDP7JrBJKvSMvwSmzzMyzg7Wsh2jT7sK_4GKMGlvFifWl9YLJy31Jw0gZ2scwfFNqhEmZ2bY1E1WdTFunk86DSkkMMM7CoIOHjydCasAwiyaODxB3Ka9trklJ1EyB6lHfjU26aO1fSiyzubYNrZPdLMZ5SRxZJzcgnF3K5Nj_lenUIZvznvANAncOmN99Rt2WIiMBqOFkuLShWoLXQ9hzbynhccO9skcYz6Xlz2Hn43W7JL3R7HX-AWtI2YRsQEAAA==

Alternatives considered

n/a

Importance

nice to have

@Malix-Labs
Copy link

Malix-Labs commented Dec 30, 2023

It seems that the "proposed solution" REPL is instead the "proposed code causing error REPL" about the error

ERROR: Cannot use fallback values with bind:

I also made a simpler one


However, on latest Win11 VScode, I only get the error too late, in the console when running the vite dev server, and not with the usual Error: 500 internal server error view
image

@Malix-Labs
Copy link

Malix-Labs commented Dec 30, 2023

Also @Rich-Harris, can you provide links for the discussions about

we resolved to disallow fallback values for bindings in runes mode. They're confusing, and a source of bugginess and implementation complexity

please ?

I imagine the confusion could come from their misleading behavior compared to other framework (see #9764 (comment))

I personally think that instead of getting rid of fallbacks values entirely, we should apply the proposed solution in #9948.

It would both more intuitive (same functioning that other framework) and easier than having the boilerplate

$effect(() => { 
	if (propName === undefined) {
		propName = propNameDefaultValue
	}
})

for each prop (which also have the big downside that propName still wrongly have undefined as a possible type)

@harrisi
Copy link

harrisi commented Dec 30, 2023

Huh, I'm actually quite confused by this as well. The docs for Svelte 4 say that bind: is used so data can flow from child to parent, but if the child can't decide what the data should begin with, what's the point?

At the same time, the workaround is just setting the default immediately after the $props line: REPL.

Note that in App.svelte, count must be $state(), although the value given to it doesn't matter.

There's some additional weirdness here, where the child component has to set the default values if it wants to ensure correct operation, or manually check the types, as far as I can tell: REPL.

Maybe that's just a REPL issue or a bug, though. I'm not convinced of anything right now because I'm not quite sure I understand why this change was made.

@hasinoorit
Copy link

It is wired, child component can change the value but can not have default value. It will force us to add some boilerplate for some use-cases. 👎

@dm-de
Copy link

dm-de commented Jan 1, 2024

WE SHOULD have default values for props, as it is both more intuitive (same functioning that JavaScript functions parameters default value) and easier than having the boilerplate

Here is one more problem:
Today, we have generally no default values (like in function) - this values are initial (like in class constructor).
This is true for svelte 3-5. But any other big framework (react, solid-js, vue) has the ability to define real default values.
So, if you wish svelte-like "default" values, you may not get what you expect.
See: #9948

I suggest to add a smarter ability to set default values.
Today, you must use this:

//Svelte 4
$: if (value === undefined) value = 123

//Svelte 5
$effect(() => {  if (value === undefined) value = 123  })

Because, Svelte is a compiler - it should add above code automatically, if we write this:
let { value=123 } = $props()

@mjadobson
Copy link

Or alternatively, to set when null or undefined:

let { propName } = $props();

propName ??= propNameDefaultValue

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE42PUWvDIBSF_8pFBm1ZSdpXFxNGf8ayh8YYZmdU9KYwxP8-o1032B6G-HDuPffwnUAmqYQn9CUQfZ4FoeTZWrIn-GFX4a9CoUjam8XxddJ47qTFttcAcrbGIZzepBphcmaGTVVnVZXDzVOv00MlEMxwERyBwYPHM4rtLu2a-p6WRMkZpB7pzVynRWPbUGRXcbNojE1t24Q0m1FOUoyEoltE3N8b5Jz_dSho4QsurnjWGeszXo_rv-26jiVjJqBwPBwg_iowLIhGg9FcSf7OwnYHrIXwHVIKwCODYzqOMRNks6cQfnriml3y_uj6Gj8BYcUIebkBAAA=

@dm-de
Copy link

dm-de commented Jan 1, 2024

Interesting, but this is a non-reactive class-constructor-like default value
(this is not how it works in react, solid-js, vue)

edit: code updated:

LINK

@mjadobson
Copy link

Interesting, but this is a non-reactive class-constructor-like default value (this is not how it works in react, solid-js, vue)

edit: code updated:

LINK

I believe it works the same way as solidjs; you need to use an effect to respond to future prop changes.

Without effect:
https://playground.solidjs.com/anonymous/4fe70729-39a6-4f66-b3c4-40c57c75e413

With effect:
https://playground.solidjs.com/anonymous/b4abc484-b2a9-483c-9c8b-76d2ca63ee68

@dm-de
Copy link

dm-de commented Jan 1, 2024

in Solid, you use:
const merged = mergeProps({ greeting: "Hi", name: "John" }, props);

in Vue, you use:
const props = defineProps({ name: {type: String, default: 'Anonymous'} })

in React, you use:
const {fontSize = "20px"} = props;

@dummdidumm
Copy link
Member

Solid and React don't have a concept for two way bindings, so the comparison is flawed

@Thiagolino8
Copy link

Solid and React don't have a concept for two way bindings, so the comparison is flawed

But Vue does have it, so the comparison is valid

@Azarattum
Copy link
Contributor

Can’t we just ignore the default value when a prop is bound, but allow it otherwise? You might not know in advance whether a particular prop will be bound when writing a component. I’d say, it isn’t even component’s responsibility to know whether somebody bounds it’s props.

@Malix-Labs
Copy link

Malix-Labs commented Jan 19, 2024

As the time pass, I'm more and more against the idea to disallow fallback values for binding.
It's very much not a pleasure nor intuitive to work around that.

@Malix-Labs
Copy link

Can this be escalated?
It's one of the least desirable changes I've seen for now.
Props are really not a pleasure to deal with currently.

@dummdidumm
Copy link
Member

We merged a relaxation of the fallback values: The runtime error is only thrown if you bind undefined to a property with a fallback value. Does this cover everyone's use case? If not, what are you trying to do that no longer works / what are you doing that needs to rely on the old "propagate fallback value upwards if undefined is bound" behavior?

@Azarattum
Copy link
Contributor

This sounds reasonable. Is it available in the repl already?

@Azarattum
Copy link
Contributor

Although I'd expect that it would bound literally "undefined" to the property (ignoring fallback completely)

@Malix-Labs
Copy link

Malix-Labs commented Feb 15, 2024

@dummdidumm I think that the #9948 proposal is more desirable, and would theoretically be incompatible with that relaxation.

As I stated in that issue, the "initial" value behavior outlined in its description is perhaps the direct cause of confusion about props (what has caused Rich to disallow fallback values for binding in the first place).

@dummdidumm
Copy link
Member

These two are not directly related - but judging from your response, you would want undefined to be the value that is passed through in case it's bound (i.e. ignoring the fallback value completely) so that #9948 could then be implemented in a consistent manner (fallback values always apply for normal props, never apply for bindings)?

@Malix-Labs
Copy link

Malix-Labs commented Feb 15, 2024

@dummdidumm

These two are not directly related

That's true indeed,
However, I think that as it would make it consistent, it would remove the confusion that perhaps caused #9764 in the first place

you would want undefined to be the value that is passed through in case it's bound (i.e. ignoring the fallback value completely) so that #9948 could then be implemented in a consistent manner (fallback values always apply for normal props, never apply for bindings)?

Exactly

Moreover, that's the standard most of the big names in frontend framework has adopted, such as React, Vue, and Solid (see #9948 (comment))

@Malix-Labs
Copy link

Update

#10797 has been merged

@ryanatkn
Copy link
Contributor

ryanatkn commented Jun 16, 2024

We merged a relaxation of the fallback values: The runtime error is only thrown if you bind undefined to a property with a fallback value. Does this cover everyone's use case? If not, what are you trying to do that no longer works / what are you doing that needs to rely on the old "propagate fallback value upwards if undefined is bound" behavior?

I have a simple case that’s unsupported, but I see a straightforward if verbose workaround. I want to be able to define a fallback in a child’s bindable prop, and then use the child without providing an initial value. This would allow parents to receive the fallback in the binding after the child initializes without being forced to override it.

  • here’s a REPL with the error when a fallback is defined and the parent initializes to undefined
  • this REPL shows a workaround using $effect in the child side-by-side with the allowed form

Shortened from the REPL, demonstrating the limitation described in the quote above:

<script>
	// Parent.svelte
	// This throws the error:
	// props_invalid_value - Cannot do `bind:value={undefined}` when `value` has a fallback value
	let value = $state(); // same as `$state(undefined)`
</script>
<Child bind:value />

<script>
	// Child.svelte
	let {value = $bindable('initial')} = $props();
</script>

Allowing the parent to pass undefined to a bindable with a fallback would make the fallbacks of = $bindable(0) and and = 0 behave consistently, and it matches my intuition that undefined values fall back to the default, but it’s currently disallowed to initialize the parent’s local value to undefined when bound to a bindable with a fallback.

In other words, children can't provide bindable initial defaults to their parents without a workaround. This case makes them not the "real" default values described in #9948.

The workaround I can see:

// Child.svelte
let {value = $bindable()} = $props();
if (value === undefined) value = 'initial'; // needed so `value` is correct during the child's initialization
$effect.pre(() => {
	if (value === undefined) value = 'initial';
});

But it would be nice to write:

// Child.svelte
let {value = $bindable('initial')} = $props();

Behaving the same as this, but bindable, so parents can pass undefined and get the fallback:

// Child.svelte
let {value = 'initial'} = $props();

A different partial workaround would be to export the fallback value from the child's module context and use it in the parents' initial values, but that value couldn't use the child's local state and it duplicates the declarations.

I have several of these cases in my projects. One concrete example is a hue input - the input component needs to choose an arbitrary starting point like 180 degrees, and I want parents to be able to use this default for consistency across the app without needing to define their own initial values, and bind:value is a nice convenience that mirrors HTML element APIs.

update: After rewriting some code because of this limitation, I think I'm glad I was nudged away from $bindable. Using explicit callbacks/event handlers makes the data flow clearer and I probably fixed some subtle bugs in more complex components. It also removes the temptation to use $effect to react to bound values changing. The downsides include the additional code and the APIs don't mirror DOM elements as much. I'd still like the ability and think we'll see more requests for this, but it could be added post-5.0 because it's just relaxing a restriction. Are there arguments against it that don't also apply to normal elements, besides implementation complexity?

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 a pull request may close this issue.

10 participants