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

Props default values should behave like default arguments of functions #4442

Closed
mindrones opened this issue Feb 21, 2020 · 9 comments
Closed

Comments

@mindrones
Copy link
Member

mindrones commented Feb 21, 2020

Describe the bug

When a prop has a default value, if you instantiate the component without that prop and then update that prop with a value and then update the component again without that prop, the prop does not take the default value anymore.

This has been discussed in #support today around 7pm.
These issues have been mentioned: #1467, #3251
Another similar issue: #3063

Logs

N/A

To Reproduce

This is intuitive but it doesn't work as I expect it: https://svelte.dev/repl/f7121f1c22c7467985b2a761dc310aea?version=3.18.2.

This is how you get it to work but it's counter intuitive to me: https://svelte.dev/repl/8ab3b1414ee54d00b24dbd66dafed5b3?version=3.18.2

Expected behavior

I think props default values should behave like default arguments of a function: if the passed prop is undefined the component should assign the default value no matter what.

Stacktraces

N/A

Information about your Svelte project:

N/A

Severity

I would say annoying/counter intuitive, especially because the doc says [1]:

You can specify a default value, which will be used if the component's consumer doesn't specify a prop.

so this can be pretty puzzling at first.

[1] https://svelte.dev/docs#1_export_creates_a_component_prop

Additional context

N/A

@Conduitry Conduitry added the awaiting submitter needs a reproduction, or clarification label Feb 23, 2020
@Conduitry
Copy link
Member

Moral issues aside of whether this is a bug fix or a breaking change, there is also the practical issue of components having no API for resetting or removing a component's prop. All there is is $set() which you pass an object to, which overwrites the current props. When you use a spread, if there are any props in the old value but not in the new value, the compiler explicitly passes undefineds for these keys in the next call to $set().

Changing this behavior would involving some new way to tell components to remember the initial values of each of their props (additional question: what would happen when the initial value is a mutable object that was since mutated?) and provide an API to set arbitrary props back to this initial value. Because of that, I'm strongly leaning towards this being something to be documented rather than something to be changed.

@Conduitry Conduitry added docs and removed awaiting submitter needs a reproduction, or clarification labels Feb 23, 2020
jesseskinner added a commit to jesseskinner/svelte that referenced this issue Feb 25, 2020
In regards to sveltejs#4442, this adds wording to explain that props are set to undefined when they are removed by the consumer.
@mindrones
Copy link
Member Author

mindrones commented Feb 26, 2020

involving some new way to tell components to remember the initial values of each of their props [...] and provide an API to set arbitrary props back to this initial value.

But wouldn't this be normal business for a "default value logic"?

additional question: what would happen when the initial value is a mutable object that was since mutated?

Continuing with the analogy between a component and a function, this is what you can expect from a function:

> c = 1 
1
> const sum = (a, b = c) => a + b
undefined
> sum(1)
2
> c = 3
3
> sum(1)
4
> c = {value: 1}
{ value: 1 }
> const sum2 = (a, b = c) => a + b.value
undefined
> sum2(1)
2
> c = {value: 10}
{ value: 10 }
> sum2(1)
11

so I would find perfectly normal to see the component using the new value when being passed an undefined prop.

At least it would be consistent with common sense (at least to me :)
Using undefined from the second time on means that we don't have a "default value" feature in place, which seems to me more useful and easier to understand than the current behaviour.

@mindrones
Copy link
Member Author

mindrones commented Feb 27, 2020

Just to add a bit of context, I'm now doing:

export let colorDefaultFill;
export let colorSea;
export let colorStroke;
export let colorStrokeSelected;
...

$: colorDefaultFill = colorDefaultFill || 'white';
$: colorSea = colorSea || 'white';
$: colorStroke = colorStroke || 'grey';
$: colorStrokeSelected = colorStrokeSelected || 'black';
...

for a lot of variables, because in practise you might instantiate a chart in a dashboard and just keep updating props as the user interacts with things. This practically leads to props becoming undefined very quickly, which defeats the purpose of having a "default" value notation. [1]

This adds a lot of visual overhead and I find it's counterintuitive (as in, I keep using the "default" value notation just to then use the above way when I encounter the said behaviour).

[1] An example of how this can be puzzling (the default fill being white):

  • we instantiate the map: fill = white, all fine;
  • we update to a blue background: fill = blue now, all fine;
  • we don't pass a fill anymore in a new update: the map background is now black (the default value for fill in svg);
  • but, by reading the component code it would seem that it should be white.

@jesseskinner
Copy link
Contributor

Would it help to set expectations to refer to this as an "initial value" rather than a "default value"?

@mindrones
Copy link
Member Author

Yeps, for the current behaviour I think "initial value" would be more appropriate.

jesseskinner added a commit to jesseskinner/svelte that referenced this issue Feb 27, 2020
@jesseskinner
Copy link
Contributor

@mindrones I've updated the wording accordingly.

@jesseskinner
Copy link
Contributor

jesseskinner commented Feb 27, 2020

I wonder if we should put something in the docs specifically about how to achieve a default value analogous to default function parameters, eg.:

export let prop = undefined;
$: prop = prop === undefined ? 'default' : prop; 

@mindrones
Copy link
Member Author

mindrones commented Feb 27, 2020

@jesseskinner please let's move this discussion to the PR. This issue is not strictly about about the documentation as here I'm proposing to change this behaviour.

EDIT: answered here

jesseskinner added a commit to jesseskinner/svelte that referenced this issue Feb 27, 2020
In regards to sveltejs#4442, this adds wording to explain that props are set to undefined when they are removed by the consumer.
jesseskinner added a commit to jesseskinner/svelte that referenced this issue Feb 27, 2020
@Conduitry
Copy link
Member

I realize that this is almost a circular argument, but now that the current behavior has been documented, doing what's outlined in this issue would be a breaking change, not a bugfix.

I also don't think that we want to give each component the burden of maintaining the default values of all of the props. Adding an extra API for resetting props to their default values (and then using this from the parent component in spreads) doesn't sound great, and changing the behavior of $set so it treats undefineds differently than it does now sounds even less great. Closing.

saabi added a commit to nestauk/svizzle that referenced this issue Dec 9, 2022
closes #604

- spacebar scrolling disabled when focused
- added theme props for focus outline
- added ariaDescribedBy and ariaLabel

- added `padding` and `textAlign` props

- renamed from `Switch`
- added keyboard support
- added theme prop `focusOutline`
- added theme prop `knobColor`
- No longer using radio buttons

- added keyboard support
- replaced message by `MessageView` component
- added `selectedKeyBackgroundColor` and `selectedKeyTextColor` theme props
- added `focusedKeyColorText` theme props
- added `hoverColorBar` and `hoverColorText` props
- added `refRectColor`, `refRectStrokeColor` and `refTextFill` theme props
- `valueAccessor` has reactive default value sveltejs/svelte#4442
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