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

fix: on teardown, use the last known value for the signal before the set #15469

Merged
merged 12 commits into from
Mar 11, 2025

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Mar 7, 2025

PSA: we are changing how effect teardowns work!

Today, if you read some state inside a teardown function, you get the latest state:

$effect(() => {
  const prev = value;
  return () => {
    // could be `true` but is usually `false`, depending on
    // whether the teardown is running because the effect
    // is about to re-run (because `value` changed) or
    // because it's being destroyed (because the
    // component unmounted, for example)
    console.log(prev === value); 
  };
});

This is counter-intuitive to many people, especially those coming from React, since this is perhaps the one place where stale closures don't make things more confusing:

const [value, setValue] = useState(...);

useEffect(() => {
  const prev = value;
  return () => {
    console.log(prev === value);
  };
});

On one level, this is just how signals work. The same behaviour occurs with other signal-based frameworks like Solid.

But on another, it's irksome behaviour, and can easily lead to frustrating bugs — if you have something like this...

{#if chat}
  <Chat {chat} />
{/if}

...and Chat.svelte reads chat inside an effect teardown function...

<script>
  import { open, close } from '$lib/chat';

  let { chat } = $props();

  $effect(() => {
    open(chat.id);
    return () => close(chat.id);
  });
</script>

...then chat will be undefined by the time it runs. You can work around it by storing a reference inside the effect, but that's weird and annoying. It's so annoying in fact that it inspired a widely-shared post called Svelte 5 is not JavaScript. We're always grateful for this sort of detailed feedback!

So here's the plan: when a state change occurs that could lead to an effect re-running, we capture the previous value. If that state is read inside a teardown function that runs as a consequence of the state change, we provide the previous value instead of the current value. As you can see from the PR, this is relatively straightforward and carries very little overhead.

This is technically a breaking change! But the strong consensus among maintainers is that it qualifies as a bugfix, and as such doesn't require a semver major release. If this decision is likely to cause you headaches, now is the time to let us know.

Thank you for your attention! — Rich

Original PR message follows:


An alternative to #15400. Fixes #14725.

As Rich suggested yesterday, we can store the old value in a Map and then use that during the teardown phase. Additionally, we also need to do some work around the logic in how props.js works which we can extract from #15400 in a simpler form as we don't need to rest of the logic.

Copy link

changeset-bot bot commented Mar 7, 2025

🦋 Changeset detected

Latest commit: d870436

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Mar 7, 2025

Playground

pnpm add https://pkg.pr.new/svelte@15469

trueadm and others added 3 commits March 7, 2025 15:25
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
@Conduitry Conduitry changed the title fix: on teardown, use the last known value for the signal before the se fix: on teardown, use the last known value for the signal before the set Mar 7, 2025
@bfanger
Copy link
Contributor

bfanger commented Mar 8, 2025

  1. Will there be a way to access the current/next value in the cleanup?
  2. Will there be a way the access the previous value during effect?
  3. Will the "last known" value be scoped to the $effect?

Proposal for 1 & 2, using a similar api to untracked(), which allows passing the next & previous function to other functions.

let { chat } = $props();

$effect((previous) => {
  previous(() => chat.id); // value of chat.id since the last $effect run
  return (next) => {
    next(() => chat.id); // value the chat.id is now
  };
});

Example for 3:

  let changesALot = $state(true);
  let changesSometimes = $state(false);

  $effect(() => {
    if (changesALot && changesSometimes) {
      console.log("changesALot and changesSometimes are both true");
    }
    return () => {
      changesSometimes; // I expect this to be the current value, unless the changesSometimes participated in the cause to run the effect.
    };
  });

@Rich-Harris
Copy link
Member

1. Will there be a way to access the current/next value in the cleanup?

If we need to add an API for getting the current value, we certainly could (latest(() => value) or whatever). It's not something we plan to do unless a need is demonstrated though

2. Will there be a way the access the previous value during effect?

Same as today:

<script>
  let count = $state(0);
  let prev;

  $effect(() => {
    console.log(prev, (prev = count));
  });
</script>

<button onclick={() => count += 1}>{count}</button>

3. Will the "last known" value be scoped to the $effect?

The guarantee is that the value will be the same in the teardown function as in the effect itself. That's all. So if changesALot updates, the teardown function will see the current value of changesSometimes. If changesSometimes changes — in other words, that change is the reason for the effect to re-run — it will see the prior value.

@Rich-Harris Rich-Harris merged commit 110d420 into main Mar 11, 2025
11 checks passed
@Rich-Harris Rich-Harris deleted the old-value-teardown branch March 11, 2025 18:48
@github-actions github-actions bot mentioned this pull request Mar 11, 2025
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

Successfully merging this pull request may close these issues.

Prop value can be different than guaranted by TypeScript
3 participants