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: cannot change nested objects if not binded #12451

Open
frederikhors opened this issue Jul 15, 2024 · 29 comments
Open

Svelte 5: cannot change nested objects if not binded #12451

frederikhors opened this issue Jul 15, 2024 · 29 comments

Comments

@frederikhors frederikhors changed the title Svelte 5: you cannot change this nested binded object Svelte 5: HUGE! Cannot change nested binded objects Jul 15, 2024
@trueadm
Copy link
Contributor

trueadm commented Jul 15, 2024

Your condition needs to be reactive, so you need to pass it in as state like shown. I wonder if we can detect this pattern in DEV and add a warning?

@frederikhors
Copy link
Author

Yeah. I think we definitely need a warning in DEV.

@trueadm
Copy link
Contributor

trueadm commented Jul 15, 2024

It seems related to #12320 too. We can probably either freeze the object if defined as an inline prop, or we can track it with a WeakMap in DEV and see where if it's used in a binding?

@frederikhors
Copy link
Author

But guys, one moment.

In App.svelte my intention is to pass some data (read-only) to List.svelte.

In List.svelte I would like to change that data (condition in this case).

Right now the only way is to create another variable inside List.svelte?

<script>
    import Filter from "./Filter.svelte";

    let { condition = {} } = $props();
    
    let new_condition_unfortunately = $state(condition)
</script>

Is there a way to avoid this new new_condition_unfortunately?

Also because I would like to reactively change new_condition_unfortunately if condition changes.

@frederikhors
Copy link
Author

It would be amazing to have something like:

let { condition = $state({}) } = $props();

Is this possible?

@frederikhors frederikhors changed the title Svelte 5: HUGE! Cannot change nested binded objects Svelte 5: cannot change nested objects if not binded Jul 15, 2024
@frederikhors
Copy link
Author

frederikhors commented Jul 15, 2024

Maybe we can do this instead:

  • App.svelte: I'm passing condition read-only and if it changes in App.svelte I need it to change in List.svelte too:
<script>
    import List from "./List.svelte";
</script>

<List condition={{name: "app"}} />
  • List.svelte: I can change condition freely in here, but I need to reflect the value in App.Svelte too if it changes
<script>
    import Filter from "./Filter.svelte";

    let {
        condition = $bindable({}),
    } = $props();
</script>

List, condition: {JSON.stringify(condition)}<br><br>

<Filter bind:condition />

Right now this doesn't work.

And I cannot use $derived neither

@frederikhors
Copy link
Author

@Rich-Harris I think we need $state() rune for $props() too:

let {
    condition = $state({})
} = $props();

@Rich-Harris
Copy link
Member

Yes, we reached the same conclusion this morning while investigating this issue. The summary:

  • prop fallback values are not made deeply reactive unless declared with $state
  • At dev time, we issue a warning if a bind: directive isn't used with something reactive. So in the bind:condition example above, we would check that the condition prop has a set accessor, and in a case like bind:x={y.z} we would check that y is a reactive state proxy, or that y.z is a setter

@frederikhors
Copy link
Author

Ok. This is the current state of affairs, do I understand correctly?

And what do you intend to do?

@trueadm
Copy link
Contributor

trueadm commented Jul 15, 2024

@frederikhors No, what Rich said is what we plan on doing next. We don't do either things yet.

@frederikhors
Copy link
Author

Ok. So you're introducing:

let {
    condition = $state({})
} = $props();

Right?

@Rich-Harris
Copy link
Member

That's the plan, yep

@trueadm
Copy link
Contributor

trueadm commented Jul 17, 2024

Ok. So you're introducing:

let {
    condition = $state({})
} = $props();

Right?

I think we're talking about two different things here. What Rich mentioned above is the same syntax, but a different behaviour from what you're asking for.

let {
    condition = $state({})
} = $props();

This would only apply deep reactivity to the fallback value when there was no prop provided. It would not make the incoming prop deeply reactive.

After working on this locally, I've actually come around to the fact that we probably should just be enforcing $bindable() here instead of using $state(). So what the correct thing to do is:

let {
    condition = $bindable({})
} = $props();

That's because I can't think of any reason that you'd want to mutate a prop without it being bound. In fact, before we (probably wrongly) made fallback values that are not marked as bindable deeply reactive, you would get a nice warning about mutating objects this way. So I think the correct action is to probably revert that PR that makes non bindable props deeply reactive.

Additionally, we can add logic in where if you pass in a non reactive object to a prop that has been marked as $bindable in DEV, but the object isn't reactive or has setters, then we warn you that this was probably a mistake, and that you should make the object you're passing in reactive.

As to the original issue:

In App.svelte my intention is to pass some data (read-only) to List.svelte.

In List.svelte I would like to change that data (condition in this case).

Using $state() on the read-only prop wouldn't actually make sense here, as $state() being passed the object would then further mutate that object with changes. Instead you probably want an effect that updates when the read-only prop changes, and updates a local state variable with that value instead. Essentially, keeping the original prop untouched, but updating your local state that can then be passed through and bound to.

@frederikhors
Copy link
Author

frederikhors commented Jul 17, 2024

@trueadm so are you saying that I should have a new state with a relative effect every time and for every prop like:

let {
    condition = {},
    filters = {},
    order = {},
} = $props();

let localCondition = $state(condition);
let localFilters = $state(filters);
let localOrder = $state(order);

$effect(() => {
    condition = // something here;
    filters = // something here;
    order = // something here;
});

... instead of having something like:

let {
    condition = $pleaseMakeThisReactiveAndLocalToThisComponent({})
    filters = $pleaseMakeThisReactiveAndLocalToThisComponent({})
    order = $pleaseMakeThisReactiveAndLocalToThisComponent({})
} = $props();

?

@Rich-Harris
Copy link
Member

Why are you mutating condition, filters and order inside the component? You shouldn't ever be mutating non-bindable props. If you're not mutating them (only reassigning them temporarily, until the prop updates) then you don't need an effect.

That's because I can't think of any reason that you'd want to mutate a prop without it being bound

Ha, excellent (and with hindsight, obvious) point.

Yes, we should absolutely revert #11804 (for non-bindable prop fallbacks, I mean — the original issue still stands). It uses static analysis to infer the desired behaviour, rather than just optimising things behind the scenes, which is a very Svelte 4 (in the negative sense) way of doing things.

@frederikhors
Copy link
Author

Why are you mutating condition, filters and order inside the component?

Let's say I have a Player page and in that page I have a list of player Skills.

I'm passing those props from the Player page to the SkillList component.

In the SkillList I can change those props accordingly (for example I can change the filters "show only the soft skills").

Then if I change those props in Player page (for example if the Player I'm viewing changes and the props reset) the SkillList component props change too.

This is a very frequent use-case.

And there is more: in this case I don't want to use a bindable object in the Player page for many reasons, one of them: for example I have a listCondition object that is $derived from something else and I cannot bind to it...

@MotionlessTrain
Copy link
Contributor

In the SkillList I can change those props accordingly (for example I can change the filters "show only the soft skills").

Then if I change those props in Player page (for example if the Player I'm viewing changes and the props reset) the SkillList component props change too.

If you solve this by reassigning the prop variables, when "show only the soft skills" is enabled, the next time the player get different skills, that filter would get undone, so this pattern wouldn't work very well anyway

A better way would be something like this, which would not involve reassigning props:

let {
    condition = {},
    filters = {},
    order = {},
} = $props();

let appliedFilter = $state(x => x) // is reassigned with a different filter, based on the selected settings, e.g. one which would only keep soft skills

let localCondition = $derived(appliedFilter(condition)); // or something like that. Not sure which of those things the filter would apply to
let localFilters = $derived(appliedFilter(filters));
let localOrder = $derived(appliedFilter(order));

Having one variable being updated in two places, with both not being synced in some way would cause issues in the long run

@CaptainCodeman
Copy link

CaptainCodeman commented Jul 17, 2024

I think the existing $bindable is sufficient and the real issue behind this is with the code for the $effect

Many variations of this same question have been posted on Discord and the svelte repos. I think the problem is that the pseudo code to test if it works isn't representative of the actual code that would work if it was completed:

i.e. this is being used to test if it re-runs when props change

$effect(() => {
  console.log('$effect, target:', target);
  // I need to do something for each params entry here
});

But it doesn't reference them. If the code was there that actually did something with the props (I understand the intention is to "persist" them) then it would re-run, because they are referenced in the $effect. As a simple example, changing it to use JSON.stringify in the REPL makes it re-run.

sveltejs/kit#12392
sveltejs/kit#12460
#12405
#12407
https://discord.com/channels/457912077277855764/1262395840031358978/1262395840031358978
https://discord.com/channels/457912077277855764/1260602115051491470/1260602115051491470
https://discord.com/channels/457912077277855764/1260988941281005660/1260988941281005660
https://discord.com/channels/457912077277855764/1253833044238794803/1253833044238794803
https://discord.com/channels/457912077277855764/1254063452813660221/1254063452813660221
https://discord.com/channels/457912077277855764/1254443024377708564/1254443024377708564
https://discord.com/channels/457912077277855764/1234512637778198651/1234512637778198651

I think many of the discussions have fizzled out because of lack of clarification or they end up with requests to "write the code for me"

@frederikhors
Copy link
Author

@CaptainCodeman this issue does not concern the ones you linked.

@frederikhors
Copy link
Author

Anyway, make it bindable and it works - I'm not sure what $bindable would be meant to do if bound to a const for example?

Please, read #12451 (comment).

And there is more: in this case I don't want to use a bindable object in the Player page for many reasons, one of them: for example I have a listCondition object that is $derived from something else and I cannot bind to it...

@CaptainCodeman
Copy link

CaptainCodeman commented Jul 17, 2024

It sounds like you need to clone or snapshot something somewhere along the line.

Your description of what you want is a little hard to follow - it's unclear if you're talking about changing player data or filters for the player data. It may be an xy-problem - I think you would be better creating a clearer example of just what you want to achieve, not what you think the solution should be and a question of how to solve some final piece.

Svelte is capable of implementing all the UX patterns you'll ever come across, but it's difficult to help when we're only given a small piece to solve and it just doesn't work the way you want it to.

@frederikhors
Copy link
Author

@CaptainCodeman this issue has a lot of examples (with reproduction) of what I'm describing as a frequent usecase.

This is another one

In this case as you can see the only way to use condition in List.svelte is to create another local variable:

let localCondition = $state(condition);

I would like to avoid that for plenty of reasons.

@CaptainCodeman
Copy link

You're still focused on one particular line of code, I'm saying to take a step back and properly describe what you're trying to achieve to know if that is even required - sometimes you end up trying to make something work that won't and it's due to previous choices.

@frederikhors
Copy link
Author

@CaptainCodeman I've taken a lot of steps back and I still can't figure it out, can you enlighten me please?

@CaptainCodeman
Copy link

"take a step back and properly describe what you're trying to achieve".

I think Discord would be better for this.

@frederikhors
Copy link
Author

Ok, again, what I'm proposing to Rich and Svelte amazing team is a way to avoid to declare local variables if I can use that prop like a $state() one (knowing that the prop on the parent is not binded and one-way only).

If Rich and the Svelte team tell me it's wrong I'll use a local variable for every prop that comes along and I'll bite the bullet as long as it suits me and as long as Svelte is so attractive more than all the other frameworks.

You're trying to convince me that it's not worth it and that my way of thinking is wrong. Ok, thanks for this, maybe that's the case, I still have to figure it out. But please, let me hear from them now.

@CaptainCodeman
Copy link

Something that only saves a single line of code is probably going to be considered low priority unless it would provide some very frequently used benefit which doesn't appear to be the case here.

My comments were about the general theme of what you've made multiple support requests for - I think your approach to asking the question is wrong which is why you don't feel you've had an answer and keep asking variations on the same thing. I'm just saying you'll get better help if you describe what you're actually trying to achieve, not just what you think the final jigsaw piece will be - sometimes the last piece won't fit if other pieces have been put in wrong.

And to get help you have to provide the information needed for people to help you. I've asked for it, other people have in discord, often you just repeat the same thing you've already asked or link to a previous version of the same question, then just re-post the same question to start again.

I'll try to help if you create a topic on Discord and can provide the information needed.

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

6 participants