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

Order matters defining transitive dependencies using reactive statement using functions #5190

Closed
opensas opened this issue Jul 23, 2020 · 5 comments

Comments

@opensas
Copy link
Contributor

opensas commented Jul 23, 2020

Describe the bug

If I define a transitive dependency using two reactive statements, like this:

  $: sum_2 = parseInt(sum_1) + 1		// ok, no matter the order! -> first command
  $: sum_1 = parseInt(sum_0) + 1		// ok, no matter the order! -> second command

(sum_1 depends on sum_0, and sum_2 depends on sum_1)

It works ok, no matter in which order I define them

But if I do the same calling functions, the order DOES matter, and the sum_2 variable is NOT updated:

  const update_sum_1 = () => sum_1 = parseInt(sum_0) + 1
  const update_sum_2 = () => sum_2 = parseInt(sum_1) + 1

  $: update_sum_2(sum_1) // FAILS -> first command
  $: update_sum_1(sum_0) // FAILS -> second command

If I switch the reactive statements it works ok

To Reproduce
REPL with the example: https://svelte.dev/repl/eb389d5c5e4b4721af15d45e2df353f2?version=3.24.0

Expected behavior

It should update dependencies as expected regarless the order in which reactive statements are introduced in the code, like what happens with updated in the reactive statement.

Information about your Svelte project:

  • Your browser and the version: Firefox 78.0.2

  • Your operating system: Ubuntu 18.04

  • Svelte version 3.24.0

  • Whether your project uses Webpack or Rollup: rollup (or whatever is used by the repl)

Severity
It's a bug affecting the reactivity system, I guess it's rather serious

Additional context

Order matters defining transitive dependencies using reactive statement using functions

REPL with the example: https://svelte.dev/repl/eb389d5c5e4b4721af15d45e2df353f2?version=3.24.0

If I define a transitive dependency using two reactive statements, like this:

  $: sum_2 = parseInt(sum_1) + 1		// ok, no matter the order! -> first command
  $: sum_1 = parseInt(sum_0) + 1		// ok, no matter the order! -> second command

(sum_1 depends on sum_0, and sum_2 depends on sum_1)

It works ok, no matter in which order I define them

But if I do the same calling functions, the order DOES matter, and the sum_2 variable is NOT updated:

  const update_sum_1 = () => sum_1 = parseInt(sum_0) + 1
  const update_sum_2 = () => sum_2 = parseInt(sum_1) + 1

  $: update_sum_2(sum_1) // FAILS -> first command
  $: update_sum_1(sum_0) // FAILS -> second command

Switching the statements work ok:

  const update_sum_1 = () => sum_1 = parseInt(sum_0) + 1
  const update_sum_2 = () => sum_2 = parseInt(sum_1) + 1

  $: update_sum_1(sum_0) // in this order this works OK
  $: update_sum_2(sum_1) // FAILS -> first command

In fact, the following also fails

  const update_sum_1 = () => sum_1 = parseInt(sum_0) + 1

  $: sum_2 = parseInt(sum_1) + 1		// ok, no matter the order! -> first command
  $: update_sum_1(sum_0) // FAILS -> second command

This is the generated code for the working example (modifying variable from reactive statement)

function instance($$self, $$props, $$invalidate) {
	let sum_0 = 0;
	let sum_1;
	let sum_2;

	function input_input_handler() {
		sum_0 = this.value;
		$$invalidate(0, sum_0);
	}

	$$self.$$.update = () => {
		if ($$self.$$.dirty & /*sum_0*/ 1) {
			$: $$invalidate(1, sum_1 = parseInt(sum_0) + 1); // ok, no matter the order! -> second command
		}

		if ($$self.$$.dirty & /*sum_1*/ 2) {
			$: $$invalidate(2, sum_2 = parseInt(sum_1) + 1); // ok, no matter the order! -> first command
		}
	};

	return [sum_0, sum_1, sum_2, input_input_handler];
}

And this is the generated code for the failing example (calling functions from the reactive statement):

function instance($$self, $$props, $$invalidate) {
	let sum_0 = 0;
	let sum_1;
	let sum_2;
	const update_sum_1 = () => $$invalidate(1, sum_1 = parseInt(sum_0) + 1);
	const update_sum_2 = () => $$invalidate(2, sum_2 = parseInt(sum_1) + 1);

	function input_input_handler() {
		sum_0 = this.value;
		$$invalidate(0, sum_0);
	}

	$$self.$$.update = () => {
		if ($$self.$$.dirty & /*sum_1*/ 2) {
			$: update_sum_2(sum_1); // FAILS -> first command
		}

		if ($$self.$$.dirty & /*sum_0*/ 1) {
			$: update_sum_1(sum_0); // FAILS -> second command
		}
	};

	return [sum_0, sum_1, sum_2, input_input_handler];
}

In the working example the compiler can order the statements according to dependencies (sum_1 depends on sum_0, and then sum_2 depends on sum_1). So it first checks the SECOND statement and then the FIRST. Ir reordered the statements according to the dependency graph.

In the failing example the statements appear in the order introduced in the code.

Seems like the compiler can correctly track dependencies in the reactive statement, but it can not do the same when calling a function. Some how it should recursively track every dependency on every function call and build the dependency tree from ther

Note/question: this is a different case, but I'm not sure what would be the correct behaviour.

What happens If I have something like:

$: {
  sum_2 = parseInt(sum_1) + 1
  sum_1 = parseInt(sum_0) + 1
}

Shouldn't this function run twice when sum_0 is modified? On the first run it will invalidate sum_1, and that should trigger the second run it would update sum_1. Am I right?

Right now, it just runs once, sum_2 is not updated, and then, on the next update of sum_0, sum_2 is updated with the value of sum_1 and then sum_1 is modified. sum_2 is always left behind with the value it shoud have had in the previous sum_0 update.

Shouldn't the $$self.$$.update = () => function be executed in some kind of loop, running until there is no more $$self.$.dirty elements?

@TylerRick
Copy link

I ran into a similar issue ... which I almost created a new issue for, but then found this issue, which I think is the same issue but see if you think it's the same:

https://svelte.dev/repl/4bb5f4b484154f6ca4c29e89240646b3?version=3.29.4

I was pretty surprised that I had to change the order of my reactive statements in order for it to work correctly.

I thought the point of reactive statements was that it lets you define variables declaritively (so that the dependency tree is all resolved for you and you don't even have to think about what order of operations would be needed to reactively calculate a value based on another value) rather than imperatively.

The strange thing is that this only happens when you extract the code that was in a reactive statement (and working just fine there) into a function. I made sure to pass the dependencies into the function ($: resetIfNeeded(value)), so I would have expected it to be reactive in exactly the same way and behave exactly the same after extracting the function as before.

@Conduitry, you mentioned in #2956 (comment):

if you do want to use separate functions and have your dependencies still counted, you can make all of the relevant dependencies be arguments to those functions, so the compiler can see them in the reactive block.

Shouldn't that apply in this case too? What am I missing?

Why does this:

	const resetIfNeeded = (val) => {
		if (val !== 'err' && error) {
			// This doesn't trigger the reactive statement above. Why not?
			error = undefined
			console.log('resetting error:', error)
		}
	}
	$: resetIfNeeded(value)

work any differently from this?:

	$: if (value !== 'err' && error) {
		error = undefined
		console.log('resetting error:', error)
	}

Or to put it another way, why does the order not seem to matter when it's in a "bare" reactive statement, but does matter when the same code is moved to a function?

Can you confirm whether this is a bug?

@TylerRick
Copy link

@opensas I just re-read your issue description in more detail. Wow, very good and thorough explanation (and nice minimal repro)!! There's not much that I could add to that...

I guess the bottom line is that we have to be more careful about ordering reactive statements if any of them call functions. (At least until this gets fixed.)

But it can be hard to notice all the places where you may have an implicit ordering dependency. Sometimes you don't notice it until after it has introduced a bug caused by a variable not reactively updating like you thought it was going to / was supposed to.

Workarounds?

I wonder if there are any workarounds one could use to "trick" Svelte into seeing the same dependencies for a function call as it would see if you inlined the function call. (In case you don't want to manually reorder your reactive statements, or you don't know in exactly which order they should be in, and you want to make sure it is "safe" (predictable/maintainable/not going to bite you later when you refactor things and move your code around) by explicitly listing your dependencies.)

I tried a few more things (in this fork of your REPL) and found one workaround that seems to work (pretty obvious now, but wasn't at first):

Since we know that update_sum_1 updates sum_1, we can give the compiler an explicit "hint" about that, like this:

	$: { update_sum_2(sum_1) }
	$: { update_sum_1(sum_0); sum_1 = sum_1 }

(or if you want to be neatly symmetrical and consistent—although not technically needed—this works too 😄 :

	$: { update_sum_2(sum_1); sum_2 = sum_2 }
	$: { update_sum_1(sum_0); sum_1 = sum_1 }

)

Even though calling update_sum_1 correctly invalidates sum_1:

	const update_sum_1 = () => $$invalidate(1, sum_1 = parseInt(sum_0) + 1);

, that actually has no useful effect in the $$.update because (without the automatic reordering that the non-function does for us) it invalidates sum_1 too late to do any good — after sum_2 has already been "updated" from a stale sum_1 value.

In a way, this workaround is analogous to the infamous assignment workaround we all know and love (to trigger reactivity after an in-place mutation of an object or array). But on the other hand, it's not the same, because in this case the "workaround assignment" is more separated (by more lines of code, and possibly even in another file?) from the "real update". A developer should not have to look at the implementation of every function, and explicitly reassign any assignments made inside the function, when the compiler is perfectly capable of doing that for us.... 😄

It also seems similar to how "read" dependencies referenced from functions are hidden to the Svelte compiler (#2956) (REPL):

	let count = 5;
	$: value = f();
	
	function f()
	{
		return count;
	}

and you have to do this (REPL) to trick Svelte into seeing those "hidden" variable references:

	let count = 0;
	$: value = f(count);
	
	function f(_dont_need_to_use_this_arg_) {
		return count;
	}

Even though the argument itself is unused, by referencing (reading from) count directly from the $:, it forces the compiler to add count to its list of "read" dependencies (the $$.dirty conditions that are checked to see if this statement should get re-run) for this $: statement.

Perhaps a "purer" way to explicitly list the dependencies would be as an array (kind of like the list of dependencies passed to a useEffect hook in React), like this:

	$: { value = f(); [count] }
	
	function f() {
		return count;
	}

This way the reader doesn't mistakenly assume that the parameter will/needs to actually be used in the function (since we can simply use the top-level variable directly from the closure instead).

So these two are kind of analogous:

	$: { value = f(); [count] }
	$: { update_sum_1(sum_0); [sum_1 = sum_1] }

My proposed workaround is to "write" dependencies (the variables that need to get invalidated as a result of the assignment) what the #2956 workaround is to "read" dependencies.

(We can even wrap it in brackets to make it clearer that it is just a hint (redundant) and not a part of the main logic that you're supposed to read.)

Must we really repeat ourselves like that?

Actual solution

Like you said:

Seems like the compiler can correctly track dependencies in the reactive statement, but it cannot [or at least does not currently] do the same when calling a function. Somehow it should recursively track every dependency on every function call and build the dependency tree from there.

and/or:

Shouldn't the $$self.$$.update = () => function be executed in some kind of loop, running until there is no more $$self.$.dirty elements?

@opensas
Copy link
Contributor Author

opensas commented Oct 27, 2020

Great work @TylerRick, the explicit 'hint' seems to be a fine workaround, and honestly, in spite being a workaround, it doesn't look so ugly.

I think this issue should really be taken into account. Specially when the logic for tracking transitive dependencies is already in place, being used in the case of transitive reactive statements. Same logic should be aplied when dealing with transtive reacive statements calling functions.

I'm pretty much surprised that so few people stumbled upon it, it's not so difficult to fall into it, and it's really hard to find out about it.

Unfortunately I'm not so comfortable with svelte source code to fix it myself, I tried to do my best to document and explain as much as I could understand about it.

I hope svelte devs find some time (I know they do a marvellous job creating this great piece of technology for free, in their spare time!) to finnaly solve this. It's a shame to have this flaw in the reactivity system, wich I think is one of svelte's strongest selling points.

@Conduitry
Copy link
Member

There would be far too many edge cases in trying to trace dependencies through functions that are called in reactive blocks. It's much better to have a set of simple, dumber rules than to have a complicated but still leaky abstraction. If we don't have a simple rule, how do we decide which things are bugs? It may not be documented yet outside of comments in the issue tracker, but the current behavior is the intended one. See also #5848 (comment)

More practically, we have been all along encouraging people to use function calls to specifically hide certain reactive dependencies from the compiler, and so any change to this would be breaking for them.

@opensas
Copy link
Contributor Author

opensas commented Sep 21, 2023

this issue seems to be fixed in the Svelte 5 preview repl

see: repl with Svelte 4

image

versus

repl with Svelte 5

image

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

4 participants