sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.04k stars 4.08k forks source link

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

Open frederikhors opened 1 month ago

frederikhors commented 1 month ago

Describe the bug

If you change the text in the input the condition doesn't change.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACp2RS2uFMBCF_0oY7kJBdG9V6KaLS2kXXTZd-IhlQJOQjJdeQv57mWvRUvpeBDKTOZzzTQKMOCkP5WMA3c4KSri2FjKgs-XCn9RECjLwZnE9dyrfO7TUSC0JZ2sciVv0JEZnZiEhL7jKV52EK6mrYlfo6jLbGz0godF1COxaCgmttRJiFEUDGcxmwBHVACW5RcVsC8fyP6a7wYmU2_Ot9fuEPDwpEoEvkrZ0ohYhZtyMohYH64z1SfoRiSNlO1IpwvHh_i735FA_43hOtqc0Vp1r-FxW8RasQz2Uu-f3_Kvmtxv4AurAlm03qST9kW41_BcfaruQ4KC1BFIvJGGFPbXTouqw6XKG-_znn-IrrT-lT6ICAAA=

Severity

blocking all usage of svelte

trueadm commented 1 month ago

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 commented 1 month ago

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

trueadm commented 1 month ago

It seems related to https://github.com/sveltejs/svelte/issues/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?

Prinzhorn commented 1 month ago

It seems related to #12320 too

Definitely, because removing bind:condition makes it reactive, which is completely counterintuitive (why would the default prop - a vanilla object - be reactive?)

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA52QT0vEMBDFv0oYPLRQ2nttF7x4ENGDR-Ohf1IZaJOQTBeXkO9uZyutiLjqIZCZeS_zfgkw4Kg8lM8BdDMpKOHGWsiATpYLf1QjqaX2ZnYddyrfObR0kFoSTtY4EvfoSQzOTEJCXnCVrz4J11JXxe7Q1VlbHJYXJ9PjgKqHktysYrYFYMkfE9zionF7hrX-nILFoyIR-CKpM7pHQqNFLa5a1H3TjioJMc14HrlrnbE-Sb8ScLpMbP5ShLunx4fck0P9isMp2UZprFp34HMm_8jIy8p9_c9fsXp--xkX-S7TrQv_xYfaziQ4aC2B1BtJWGGPzTirOmy-nOHit-Qv8R2mKxatkQIAAA==

$bindable does not matter either:

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA52QS2uFMBCF_0oY7kJBdG9V6KaLS2kXXTZd-IhlQJOQjJdeJP-9zrVoKX0vAnMmczLnyww9DspD_jiDrkcFOVxbCwnQ2bLwJzWQWrQ3k2u5U_jWoaVKakk4WuNI3KIn0TszCglpxipdfRKupC6y3aGLy2xWLS-OpsMeVQc5uUmFZAvAI39McIPLjNszrPp9Ch4eFImZC0mt0R0SGi1KMYeEm2EpD9YZ66P4Y2yOlIjNlIv5-HB_l3pyqJ-xP0fbVRyKxlV8LrhvwRrUXb7v_J5_9fz2B76AOvDKuhlUFP9Ity78Fx9qO5HgoKUEUi8kYYU91cOkynnzpQwXPiV_Cq8OhbKhhgIAAA==

frederikhors commented 1 month ago

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 commented 1 month ago

It would be amazing to have something like:

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

Is this possible?

frederikhors commented 1 month ago

Maybe we can do this instead:

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

<List condition={{name: "app"}} />
<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 commented 1 month ago

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

let {
    condition = $state({})
} = $props();
Rich-Harris commented 1 month ago

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

frederikhors commented 1 month ago

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

And what do you intend to do?

trueadm commented 1 month ago

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

frederikhors commented 1 month ago

Ok. So you're introducing:

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

Right?

Rich-Harris commented 1 month ago

That's the plan, yep

trueadm commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 https://github.com/sveltejs/svelte/pull/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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

https://github.com/sveltejs/kit/discussions/12392 https://github.com/sveltejs/kit/discussions/12460 https://github.com/sveltejs/svelte/discussions/12405 https://github.com/sveltejs/svelte/discussions/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 commented 1 month ago

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

CaptainCodeman commented 1 month ago

It seemed like a continuation of all your other ones.

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?

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE52QT2vDMAzFv4oROyQQknuaBHbZoYztsOO8Q_44Q5DYxlbKivF3H2pKQ8eg6w4GS_aT3u8FGHFSHsr3ALqdFZTwaC1kQEfLhT-oiRRk4M3ieu5UvndoqZFaEs7WOBLP6EmMzsxCQl5wla86CTv-NikSvdEDEhotavHgqSWVBF5YCgmttRJiKnVVbMN1dRrboR7KTVw0kMFsBhxRDVCSW1TMLtZZcaf3J5xIuc39Wl_5PxMEvki6AmF3bTepJMQ04_fIXeuM9Um6-wHE7rItiFKE_dvrS-7Jof7E8ZhcntJYda7hcwri7PGuKFbNX8O4yXebbl34Lz7UdiHBRmsJpL5Iwgp7aKdF1eGiyxku_kr-Eb8Bh_rH-ssCAAA=

frederikhors commented 1 month ago

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 https://github.com/sveltejs/svelte/issues/12451#issuecomment-2234063760.

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

CaptainCodeman commented 1 month ago

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

I think Discord would be better for this.

frederikhors commented 1 month ago

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 commented 1 month ago

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.