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

Bindings don't work inside {#each reactiveThing as thing} #2444

Closed
Rich-Harris opened this issue Apr 20, 2019 · 6 comments · Fixed by #2694
Closed

Bindings don't work inside {#each reactiveThing as thing} #2444

Rich-Harris opened this issue Apr 20, 2019 · 6 comments · Fixed by #2694
Labels

Comments

@Rich-Harris
Copy link
Member

In a case like this...

{#each filtered as todo}
	<label class:done={todo.done}>
		<input type="checkbox" bind:checked={todo.done}>
		{todo.text}
	</label>
{/each}

<p>completed {todos.filter(t => t.done).length} of {todos.length}</p>

...where filtered is derived from todos, toggling an individual input doesn't affect anything else that depends on todos. Demo here.

That's because this code gets generated:

function input_change_handler_1({ todo, each_value, todo_index }) {
	each_value[todo_index].done = this.checked;
	$$invalidate('filtered', filtered);
}

We should instead invalidate the ultimate dependencies of filtered rather than filtered itself:

function input_change_handler_1({ todo, each_value, todo_index }) {
	each_value[todo_index].done = this.checked;
-	$$invalidate('filtered', filtered);
+	$$invalidate('todos', todos);
+	$$invalidate('hideDone', hideDone);
}

(While hideDone doesn't need to be invalidated here, I'm not certain we can reliably determine that statically. Needs more thought.)

@Rich-Harris Rich-Harris added this to the 3.x milestone Apr 20, 2019
mrobakowski added a commit to mrobakowski/svelte that referenced this issue Apr 28, 2019
mrobakowski added a commit to mrobakowski/svelte that referenced this issue Apr 28, 2019
mrobakowski added a commit to mrobakowski/svelte that referenced this issue Apr 28, 2019
@mrobakowski
Copy link

Hi, I had a shot at this and managed to get the compiler to generate 3 invalidates - one for each dependency and one for the reactive declaration. Some tests didn't pass if I emitted invalidates only for the dependencies. Here's the commit: mrobakowski@8e470fa

@mrobakowski
Copy link

Also, I believe I found another bug while trying to reduce the example https://svelte.dev/repl?version=3.1.0&gist=c4b48ccfb869a6915a3def99b3fbbd2d. With my commit name is properly invalidated, but the text isn't updated (possibly because compiler doesn't consider name as ever written to?).

Rich-Harris added a commit that referenced this issue May 6, 2019
invalidate dependencies of reactive declarations
@Rich-Harris
Copy link
Member Author

Thank you! I adapted your solution in #2694. Reopening this (autoclosed) issue so that we can address the second bug you found — you're right, the compiler thinks name is never assigned to or mutated, so it generates update code like this...

p(changed, ctx) {
  if ((changed.foo) && t5_value !== (t5_value = ctx.foo.name.w)) {
    set_data(t5, t5_value);
  }
}

...which means name can never update.

@Panya
Copy link
Contributor

Panya commented Jun 20, 2019

It was fixed in 3.2.1 and above.

@mrobakowski
Copy link

@Panya the original issue was fixed, but https://svelte.dev/repl?version=3.1.0&gist=c4b48ccfb869a6915a3def99b3fbbd2d still doesn't behave like it should.

@Conduitry
Copy link
Member

In

<script>
	let name = { w: 'world' };
	$: foo = { name }
</script>

<svelte:body on:click={() => foo.name.w += "!"}/>
<h1>Hello {name.w}!</h1>
<h2>{foo.name.w}</h2>

I really don't think that we should expect the compiler to consider modifications to foo.name to be modifications to name. It might be obvious in this specific case, but in general this won't be something that's possible at compile time. One of our guiding principles for reactivity has been to have simple rules the compiler follows to know what invalidates what, rather than having wildly more complicated rules that cover additional cases, and where it's never clear whether something that's not reactive is a bug or is working as intended.

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