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

rerun reactive declarations if the reactive declared variable is mutated #5045

Draft
wants to merge 2 commits into
base: svelte-4
Choose a base branch
from

Conversation

tanhauhau
Copy link
Member

Fixes #2444

The demo in #2444 was fixed in v3.2.1 but broken again since v3.16.0

The test case added in #2694 didn't handle a particular case in the demo where there's another declaration that should be invalidated too:

<script>
   let foo;
   $: var_a = some_fn(foo);
   $: var_b = some_fn(foo);
</script>
{#each var_b as b}
   <input bind:value={b} />
{/each}
{var_a.length} {var_b.length}

after #2694 mutating var_b, would $$invalidate var_b & foo.

but because foo wasn't mutated anywhere, var_a is marked as fixed_reactive_declarations instead, and $$invalidate foo will not produce a new var_a.

This fix is to mark foo as mutated if var_b or var_a is mutated.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@Conduitry Conduitry marked this pull request as draft June 23, 2020 13:57
@Conduitry
Copy link
Member

The test failure here looks to be legitimate.

@stale
Copy link

stale bot commented Jun 26, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 27, 2021
@vercel
Copy link

vercel bot commented Mar 16, 2023

@dummdidumm is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@dummdidumm
Copy link
Member

dummdidumm commented Mar 16, 2023

I'm not sure how I feel about this change.

  • Con: It could result in a lot of false positive reruns
  • Con: The tests currently fail, unsure how reliably we can get this to work with introducing bugs
  • Pro: Intuitively it makes sense to me to rerun in this case

In general this looks like one of those general shortcomings / things we haven't quite thought through holistically for bindings. Personally I'd hold off from merging this and wait until Svelte 5 where we should have a deep conversation about the future direction of bindings.

@dummdidumm dummdidumm added this to the one day milestone Mar 16, 2023
@tanhauhau
Copy link
Member Author

@dummdidumm yes you are right, i think we can hold off or even close this one for now.

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

Successfully merging this pull request may close these issues.

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