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

inconsistent behavior when updating reactive declared variable #4933

Closed
tanhauhau opened this issue May 30, 2020 · 25 comments
Closed

inconsistent behavior when updating reactive declared variable #4933

tanhauhau opened this issue May 30, 2020 · 25 comments
Labels
Milestone

Comments

@tanhauhau
Copy link
Member

tanhauhau commented May 30, 2020

Describe the bug
I am not sure what is the expected behavior when updating reactive declared variable, but here are the inconsistencies that I have found.

First of all, here is what I meant by updating reactive declared variable

<script>
   let a = 1;
   $: b = a * 2;

   function update() {
      b = 42;
   }
</script>
a: {a} b: {b}
<button on:click={update}>Update b</button>
<input bind:value={a} />

REPL

The intended behavior for the code snippet above is to

  • reactively update b when a changes
  • allows b temporarily go "out-of-sync" of a when calling update, setting b to 42
    • in this case, b is not always a * 2
  • however, if a changes again, b will be updated back to a * 2, instead of staying at 42

I used the word "intended" behavior, because that is the behavior im looking for, but I may not be expressing it correctly in Svelte. It may not be the expected behavior of the code.

In the example above, the REPL behaves as intended, however, it will break if all of the following conditions are met:

Condition 1: any of the dependencies of the reactive declarations is mutated, reassigned, or exported
in this case

  • if a is exported (changing the example to export let a = 1, or
  • if a is mutated / reassigned, eg: <input bind:value={a} /> or having function foo() { a = 5 };

Condition 2: the dependencies of the reactive declarations that is mutated, reassigned or exported is not a primitive
this is because of the behavior of $$invalidate, Svelte uses safe_not_equal to decide whether the updated value is the same as the current value during $$invalidate. comparing objects with safe_not_equal will always return true, because Svelte allows user to mutate the object / array directly, therefore should always $$invalidate them.

  • try changing the example to a = { v: 1 } and $: b = a.v * 2

Condition 3: using bind: or value = value when updating the reactive declared variable
this is kind of the edge case that wasn't handled properly in https://github.com/sveltejs/svelte/blob/master/src/compiler/compile/render_dom/Renderer.ts#L154

  • try changing the example to function update() { b = 42; b = b; } or adding <input bind:value={b} />

The behavior of the bug was introduced in #2444.

The intention of the issue #2444 was to propagate the changes of the reactive declared variables back to its dependencies. in the context of this example, would meant, updating b should update a as well. updating a will update b back again in the reactive declaration.

It works if you always want b to be the value deriving from a. However in the example above, we want the value of b to be temporarily out of sync of a.

I dont know what is the expected behavior of the Svelte should be, but having inconsistencies when all the "subtle" conditions were met is unfriendly. it requires the user to have much deeper understanding of the nuances of the language.

Related issues that are symptom to this inconsistencies:

@dimfeld
Copy link
Contributor

dimfeld commented Jul 22, 2020

As an example of condition 3 I just encountered, this statement:

providerMarkers = providerMarkers; // trigger reactivity

at the end of a fairly complex reactive block generates this entire thing:

(((((((((((((((((((((((((((((((((((((((((($$invalidate(117, providerMarkers), $$invalidate(107, map)), $$invalidate(119, hasLine)), $$invalidate(7, providers)), $$invalidate(115, activeProviders)), $$invalidate(101, showConnections)), $$invalidate(24, focalProvider)), $$invalidate(118, lines)), $$invalidate(100, highlightConnectionProviders)), $$invalidate(58, report)), $$invalidate(37, activeCompetitorCountFilters)), $$invalidate(36, activeReferrerCountFilters)), $$invalidate(17, showReferrerTypes)), $$invalidate(96, _b)), $$invalidate(97, _c)), $$invalidate(5, selectedProfiles)), $$invalidate(2, showAllConnections)), $$invalidate(116, activeProviderIds)), $$invalidate(99, showListProvidersSelected)), $$invalidate(98, hoverProvider)), $$invalidate(27, selectedProvider)), $$invalidate(123, providerListIds)), $$invalidate(102, showConnectionsSet)), $$invalidate(14, competitorCountFilters)), $$invalidate(13, referrerCountFilters)), $$invalidate(104, lastReferrerConfigName)), $$invalidate(25, usedReferrerConfigName)), $$invalidate(23, currentProviderList)), $$invalidate(31, worklist)), $$invalidate(3, viewingProviderInfo)), $$invalidate(42, topVolumeProviders)), $$invalidate(95, _a)), $$invalidate(129, initialWorklistObject)), $$invalidate(103, viewHoverProvider)), $$invalidate(35, activeListProfilesSet)), $$invalidate(22, listSortField)), $$invalidate(29, worklistObjects)), $$invalidate(124, tmConfig)), $$invalidate(21, activeListProfiles)), $$invalidate(30, $providerLists)), $$invalidate(39, activeProfilesInfo)), $$invalidate(122, oldActiveProfilesInfo)), $$invalidate(127, activeProfiles)), $$invalidate(26, $profilesStore); // Trigger reactivity.

So it invalidates everything that the block references and ends up rerunning pretty much every reactive block in the component. If I change providerMarkers = providerMarkers to providerMarkers = new Map(providerMarkers), then the output is just this:

$$invalidate(117, providerMarkers = new Map(providerMarkers)); // Trigger reactivity.

That's what I would expect. It's not a big deal in my case to recreate providerMarkers each time but this is definitely unintuitive and can possibly cause issues when dealing with imperatively controlled libraries such as Leaflet.

I ended up using the above workaround and also added extra guards on the reactive block that clears and recreates the Leaflet map when new data arrives.

Here's a simple REPL that demonstrates just this particular issue: https://svelte.dev/repl/3c532069bd8c4d1bb0cee46807843b63?version=3.24.0

@TehShrike
Copy link
Member

A reproduction of undesirable invalidation when mutating a reactive declaration: https://svelte.dev/repl/785c7c419caa49dea1b8414e9c6916f2?version=3.42.1

@micahswab
Copy link

Another simple reproduction of this issue when using an export, #each and bind: https://svelte.dev/repl/837df5671cc2442fbac4ebf7d4a07ffc?version=3.42.4

@frederikhors
Copy link

Can #6507 be related? Can you help me understand if this is the case?

@Prinzhorn
Copy link
Contributor

My mental model has always been that a reactive declaration like $: b = a * 2 defines b in it's entirety ("Here's my recipe for b and all the ingredients, please Svelte make b always up to date"). And outside of this declaration b is immutable. I never had the use-case to touch b because that might cause my brain to melt when I manually need to think about what b could be at any given moment. If it's immutable and completely defined by the reactive declaration then it is easy to look at the code (yay code quality).

I personally think the compiler should prevent any mutation of these reactively declared variables. That would get rid of all these edge cases at the root. If you need this behavior, you can always achieve it, see below.

For me there is a distinct difference between these two scripts:

let a = 1;
$: b = a * 2;
let a = 1;
let b;
$: {
    b = a * 2
};

The first example defines a "recipe" for how to create b and b is completely defined by that declaration. Outside of that it is immutable, data flows only into a single sink.

The second example declares a variable b and then uses a reactive statement to update it. But it also allows you to do with b whatever you want. If someone wants to go that route (definitely not me), they are free to do so at their own risk of ensuring consistency.

TLDR: Introduce a compiler option that makes reactive declaration in the first example immutable. Remove the option in v4 and make it the default. Anyone in the same boat as me?

@ghost
Copy link

ghost commented Sep 13, 2021

What would be a workaround for this issue at the moment? Is it even possible?

I made another example here https://svelte.dev/repl/96e9f667343a492faee44a0fa8e0b36c?version=3.42.5

@benmccann
Copy link
Member

benmccann commented Sep 18, 2021

When Rich introduced Svelte, he used spreadsheets like Excel as a model to explain reactivity. In Excel, you can only define a cell's value with a single formula. You can't have multiple things updating a cell's value. And I think that makes sense because I don't know how things would behave otherwise. So if we think about reactivity in those terms, then @Prinzhorn's suggestion that you not be able to mutate a reactive variable outside of its declaration makes a lot of sense. My initial reaction is that I don't think that would be limiting because you can express almost anything in Excel and it's a tool a lot of people were able to easily learn to use.

I'm not sure by looking at the description of this issue that I totally understood at first how difficult some of these cases might be to model and think about. Going off the issue description, I'd probably just say that the value should end up as 42. But some of these cases can get more confusing. E.g. looking at #6720 you have to start doing more mental gymnastics to explain how things would work

@ghost
Copy link

ghost commented Sep 18, 2021

@benmccann My case is that I have a component which takes an object as a prop. As the user changes the object's values, they can either Save or Cancel. So in the component I have two variables: value and valueUnsaved. It's similar to the example on my comment above. To avoid mutating the original object directly, I assign valueUnsaved as a deep clone of value. If value is changed outside of the component, valueUnsaved should be updated. But unfortunately I couldn't get this to work.

@frederikhors
Copy link

@benmccann My case is that I have a component which takes an object as a prop. As the user changes the object's values, they can either Save or Cancel. So in the component I have two variables: value and valueUnsaved. It's similar to the example on my comment above. To avoid mutating the original object directly, I assign valueUnsaved as a deep clone of value. If value is changed outside of the component, valueUnsaved should be updated. But unfortunately I couldn't get this to work.

Same here, and I think this is not correct. The more I think about it, the more I am convinced it's a wrong mental model.

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Sep 19, 2021

My case is that I have a component which takes an object as a prop. As the user changes the object's values, they can either Save or Cancel. So in the component I have two variables: value and valueUnsaved. It's similar to the example on my comment above. To avoid mutating the original object directly, I assign valueUnsaved as a deep clone of value. If value is changed outside of the component, valueUnsaved should be updated. But unfortunately I couldn't get this to work.

I'm doing exactly the same in my app and just looked at how I implemented it. Turns out I'm using a store (since all these things are held in stores that are transparently synced with the server and across multiple Electron windows using BroadcastChannel). Here's the relevant code copied (and redacted) from my component with the original comment from my code base (MIT licensed for the sake of this comment):

(the feature allows customizing the "Copy as..." context menu in the app, the copyAsTemplates holds all the context menu entries. All my stores also have a defaultValue property and reset method)

<script>
  import equal from 'fast-deep-equal';
  import { onMount } from 'svelte';
  import { copyAsTemplates } from '~/js/stores.js';
  import deepClone from '~/js/lib/deepClone.js';

  // Without cloning this will point to the same object and they'll always be the same.
  // We'd basically change the object in the store without changing the store.
  let tmpCopyAsTemplates = deepClone($copyAsTemplates);

  onMount(() => {
    // We cannot make the above statement reactive because we touch tmpCopyAsTemplates.
    // But we need to make sure to sync this when a different window changes copyAsTemplates.
    return copyAsTemplates.subscribe((value) => {
      tmpCopyAsTemplates = deepClone(value);
    });
  });

  $: isUntouched = equal(tmpCopyAsTemplates, $copyAsTemplates);
  $: isDefault = equal(tmpCopyAsTemplates, copyAsTemplates.defaultValue);

  function handleSaveClick() {
    $copyAsTemplates = deepClone(tmpCopyAsTemplates);
  }

  function handleCancelClick() {
    tmpCopyAsTemplates = deepClone($copyAsTemplates);
  }

  function handleResetClick() {
    copyAsTemplates.reset();
  }
</script>

<!--Redacted component stuff for actual editing-->

<button disabled={isUntouched} on:click={handleSaveClick}> Save </button>
<button disabled={isUntouched} on:click={handleCancelClick}> Cancel </button>
<button disabled={isDefault} on:click={handleResetClick}> Reset to default </button>

I've said it multiples times and I say it again: in Svelte stores solve all your problems and I love them so much. In cases like this, when you run into limitations of reactivity, you can always resort to an imperative subscribe call (I usually avoid imperative code in Svelte like the plague).

@ghost
Copy link

ghost commented Sep 19, 2021

That's a great solution! But wouldn't you need the prop to be a store so you can subscribe to updates? Or maybe the component could create a store and synchronize it's value using a reactive statement?

@Prinzhorn
Copy link
Contributor

@joaopaulobdac that depends on your architecture, I guess your parent component could wrap the value in a store and pass it down without bind. At least that's a workaround for now, there might be some change coming following the discussion in #6730

@TylerRick
Copy link

I ran into I think this same confusing (IMHO incorrect) behavior today.

Would this be an example of Condition 1 (any of the dependencies of the reactive declarations is mutated, reassigned, or exported)?:

https://svelte.dev/repl/ff6e69e975df44f3821cc4ed956881f8 (created as new issue #7129)

@jmroon
Copy link

jmroon commented Jan 17, 2022

Ran into the same thing as well. Thankfully, I'm already using stores and can sidestep the issue with manual subscriptions, but I'm a bit leery of overusing reactive statements now as it seems like some nasty bugs could pop up unintended.

Here was my REPL, for what it's worth.
https://svelte.dev/repl/6ffc787bb6fd42f1a2590b98ff8838cd?version=3.46.2

You can kind of sidestep the issue without stores by calling a separate function in your reactive statement, and assing to your left hand side variables in there. Seems as though the compiler won't generate any additional invalidates in that case.

@7nik
Copy link

7nik commented Feb 9, 2022

Here is my simplified case https://svelte.dev/repl/715f7a4749264a0e88aac699a9641dc4?version=3.46.4
In reality, I made a visualization of algorithms work on a graph that users can enter. Apparently, I need to clean up data before the next run, but the reactivity was messing up with the first visualization step.

@QuickMick
Copy link

i ran into the same issue, i think.
here is a simplified example repo: https://github.com/QuickMick/svelte-computed-with-object-bug ( i also made a bug report, because i have not seen this issue here - #7282)
it seems, that also some events (in this case, the "input" event of the textarea) forces the reactive statement to reevaluate, even if there are no changes.

@madacol
Copy link

madacol commented Mar 18, 2022

To prevent a reactive assignment to climb up the dependency tree

$: data = obj.data

becomes

let data;
const getData = obj => obj.data
$: getData(obj)

// now you can set `data` externally without invalidating `obj`

Honestly I've spent all day and I still don't fully understand how reactive declarations are supposed to work. It's very unintuitive. And I feel like I can't get by with current behavior that climbs up the dependency tree

@madacol
Copy link

madacol commented Mar 18, 2022

Within my own ignorance, I feel like there should be 2 separate ways of "reactive declarations", depending on how the invalidate moves the dependency tree:

  1. one-way (downward)
  2. two-way (downward/upward)

Something like $: variable = ... for one-way, and $bind: variable = ... for two-way

Similar to how binding comes in 2 flavors too, one-way binding <input {value}/>, and two-way binding <input bind:value/>.

@cshaa
Copy link

cshaa commented Sep 20, 2022

I've come across this bug in the enterprise app I'm working on, and it's a serious pain in the ass for us. We need to fetch data from the backend every time a prop updates, but this bug invalidates the prop again and again leading to an infinite loop.

Pseudocode showing our use case:

export let url;
$: content = url;
$: fetchData(url);

function fetchData(url) {
  fetch(url).then(c => content = c); // invalidates url => runs in an infinite loop
}

Actual code from our application in the REPL.

I don't see any simple walkaround, and I couldn't find any version of Svelte where this works properly.


EDIT: I did find a walkaround, inspired by madacol's comment. The key is not letting Svelte know that the dependent variable relates to the prop in any way, wrapping the relation in a function. The pseudocode from before would look like this:

export let url;
let content;
$: fetchContent(url);

function fetchContent(url) {
  content = url;
  fetch(url).then(c => content = c); // only invalidates content, as Svelte doesn't know it's a dependency of url
}

Actual code in the REPL.

I still maintain that this is a bug, as the behavior is very unintuitive and I don't see any good reason for Svelte behaving in this way.

@hartwm
Copy link

hartwm commented Oct 5, 2022

This is kinda crazy. I just spent hours thinking I was doing something dumb. So you push data to store from API and then you want to filter that data locally but it just doesn't work.

@fkling
Copy link

fkling commented Jan 16, 2023

I've also encountered this surprising behavior and dug a little bit into it. I'm probably repeating a lot of things that have already been said.

I think regardless of whether assigning to a reactive variable should be allowed or not, the behavior overall should still be correct and consistent in itself. Take the following combined example:

<script>
	export let user;
	$: name = user.name
</script>

<input bind:value={name} />
<input value={name} on:change={event => name = event.target.value}>

The reactive variable name is updated via binding and via "manual" assignment. In general I would expect both of these behaviors to be equivalent, but the generated code for both event handlers is different:

let name;
let { user } = $$props;

function input0_input_handler() {
  name = this.value;
  ($$invalidate(0, name), $$invalidate(1, user));
}

const change_handler = event => $$invalidate(0, name = event.target.value);

When using bind:value we also invalidate the dependencies of the reactive variable, but we don't do that in the assignment case...

I now understand that invalidating dependencies was introduced by #2444, but I think the logic is not granular enough. In JavaScript, assigning to a variable can never have an impact on the values/variables from which this variable was initialized. I think the issue becomes even clearer in the following example:

<script>
	export let user;
	let name
	$: if ($someStore) {
		name = user.name
	}
</script>

<input bind:value={name} />

which generates

let $someStore;
component_subscribe($$self, someStore, $$value => $$invalidate(2, $someStore = $$value));
let { user } = $$props;
let name;

function input_input_handler() {
  name = this.value;
  (($$invalidate(0, name), $$invalidate(2, $someStore)), $$invalidate(1, user));
}

$someStore is only "indirectly" used for computing the value of name, yet it's invalidated too. I think that's conceptually wrong, regardless of whether assigning to a reactive variable should be allows or not.

What makes the matter worse (and which has been mentioned already) is that sometimes invalidating the dependencies will have an effect (if the dependency is an object value) or not (if the dependency is a primitive). This behavior is not obvious to someone who just uses bind:value.

To me a possible solution seems to be that when binding to a reactive variable, dependencies should be invalidate iff the binding target is not a variable and the dependency is directly used in computing the variable's value (to prevent the if example above).

I started to look into the implementation a bit. The "problem" is that the logic for generating the $$invalidate calls (in invalidate.ts doesn't know know anything about the nature of the passed in name. E.g. if we bind to an object property like bind:value={user.name} then this logic gets passed user and at this point the information that a property is bound is not available.

@tanhauhau tanhauhau added this to the 4.x milestone Feb 24, 2023
@willnationsdev
Copy link

I agree with @fkling here. I was struggling with these issues significantly while starting to actually use Svelte for a real project. I'm concerned that the unpredictability of the reactive behavior is going to be a major turn-off for others on my team who are less experienced with programming (designers that dip into code when they have to).

Regardless of whether you divide up reactive declarations to account for one-way vs. two-way binding via syntax changes, before that, there needs to be a pristine, consistent, and intuitive clarity by which Svelte users can engage with reactive declarations/statements overall.

@NfNitLoop
Copy link

NfNitLoop commented Jul 3, 2023

I'm running into similar issues in #8895.

I was struggling with these issues significantly while starting to actually use Svelte for a real project.

I've been using Svelte on/off for a few years and this continues to bite me in the ass, so it's not just newbies. The behavior can be as you expect until you modify some other part of your component, and then you have a regression that you need to debug, and debugging it is time-consuming.

I'm concerned that the unpredictability of the reactive behavior is going to be a major turn-off for others on my team who are less experienced with programming (designers that dip into code when they have to).

Unpredictability is a "major turn-off" to seasoned developers too! 😊

there needs to be a pristine, consistent, and intuitive clarity by which Svelte users can engage with reactive declarations/statements overall.

👆💯

The docs/tutorial talk about the fact that components will only get updated and reactivity will only fire when the reference of an object changes -- interior mutability doesn't count. But then there are undocumented exceptions like this that try to be "conservative" and do updates when objects references haven't changed, leading to unexpected behavior.

I <3 Svelte's developer experience on the whole, but I've now spent enough time investigating issues like this that I'm investigating alternatives. 😞

IMO, this bug can be boiled down to what @tanhauhau said:

I don't know what is the expected behavior of the Svelte should be

Documenting the intended behavior would be a great first step here. Simplifying it would be better. 🤞

@Not-Jayden
Copy link
Contributor

Just wanted to share that this might be solved in Svelte 5, using $state() and $effect() runes to achieve reactivity instead of the svelte 4 reactive declaration.

Svelte 4 reproduction (can't update b): https://svelte.dev/repl/f5cfadae61e14fe989d0afe7496ebe1e?version=3.23.0
Svelte 5 reproduction (works as expected): https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE3WRwWrDMAyGX0WYHtIxGlZ68ppA-w47LTvYjgJmiWMSuTCM3312nJAetpul__ul38izTvc4M_7pmREDMs5u1rJXRj82FfMDe8JYz6ObVOpcZzVpS3VjGuqRQEAFh5kEYeEfonfI4S0c3zdZ7rI4LTq8wDnrB-w6VFQUR6hq8KnVUDI8kQuY58VH54wiPRpwto0jb9G52lKMbf_lHLLtD8t9t6RNl7wgF3JzXcv9k0Zw8GugADIWMhHSEcWho-Gq1-q78muiUMPH8gJxLTNU_4_fQ73S8pnWxjqCdIKqYcYNEqeGgdSm5UuOag9URj6eZxhb3WlsGacptr_CL6ayI9nZAQAA

@dummdidumm
Copy link
Member

This comes down to $: standing for two things and it's not easy to distinguish:

  • side effects that happen in reaction to something
  • derived state that keeps values in sync

Svelte 5 fixes this by separating these into two distinct runes, $effect and $derived. That way it's much clearer what's going on and such edge cases are avoided entirely.
In this case, you would have a $state variable and either update that through the binding of from an $effect, resulting in the desired behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests