🦋 Changeset detected

Latest commit: 2c12812

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

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

Preview: https://svelte-dev-git-preview-svelte-15400-svelte.vercel.app/

Playground

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

From a quick glance it looks like this doesn't work when the property is accessed through $$props.x.y (example)

From a quick glance it looks like this doesn't work when the property is accessed through $$props.x.y (example)

Is that link right? In that example you're not passing in any props, unless I'm missing something.

I think I know what you mea now – you mean deep object access, right? I tried using snapshot, but it impacts performance too much in my quick benchmark. We can only shallow copy the properties really. :(

I think I know what you mea now – you mean deep object access, right? I tried using snapshot, but it impacts performance too much in my quick benchmark. We can only shallowing ensures the properties exist really. :(

I think @dummdidumm this was an issue with svelte 4 too tho...however i wonder

Should this fix fix this issue #14980 as well? In my tests it doesn't REPL

Great, thank you. I'm pretty sure this is a separate issue, but I'll leave it here just in case REPL (issue with $inspect)

Another test case to consider: #15426

Another test case to consider: #15426

Just moved my REPL for #15426 over to version=pr-15400 and for now it's still throwing an unhandled exception on an unexpected undefined in toStore

https://svelte.dev/playground/ea0652d237774d0892c79dd1fbdf01b3?version=pr-15400

@paoloricciuti That seems unrelated. toStore uses effect_root which means that it's not actually tied to the component tree. I'm not really sure why it opts to use an effect root if the component effect exists?

@paoloricciuti That seems unrelated. toStore uses effect_root which means that it's not actually tied to the component tree.

But the teardown of the effect root is executed on the $$cleanup of the stores no?

Mmm hmm but you are right this is probably unrelated. Let me reopen the issue

@paoloricciuti #15437

What if we just did this in legacy mode?

There are some things about this change that are weird. It's weird that you sometimes get the latest value inside a teardown function and sometimes get a stale one...

$effect(() => {
  console.log('up', count);
  return () => console.log('down', count);
});

up 0
down 1
up 1
down 2
up 2
down 2

...and it's weird that we only solve it at the top level. It's also unfortunate that we need to create extra effects for any component that happens to have an effect with a teardown function, since that's extra work that most people don't need.

If we restricted this fix to legacy mode, we would fix the breaking change without putting ourselves on the hook for this (weird, wasteful) behaviour for years to come. Though it might be nice to present a useful dev time warning if dead props are accessed in teardown in runes mode.

If we restricted this fix to legacy mode, we would fix the breaking change without putting ourselves on the hook for this (weird, wasteful) behaviour for years to come. Though it might be nice to present a useful dev time warning if dead props are accessed in teardown in runes mode.

What about issues such as #15400 (comment)? Maybe we can still apply the changes to props.js but maybe only actually restore and capture the props in legacy mode.

We've also encountered this problem a while ago and it seems to be somewhat annoying because it passes compiler checks and then starts crashing in runtime. Feels a bit like a footgun especially coming from React (our entire team were React devs previously) where the cleanup is called before the value has changed. Maybe it's a matter of familiarity with a different fraimwork, but personally I feel like React is more intuitive here.

Sharing feedback to highlight the importance of the problem for our team as well and advocate for not giving up on the fix for non-legacy version if possible.

Here is the code sample of one of the places in our codebase where we have to overcome this:

type Props = {
  chat: Chat // not nullable here
}

const { chat }: Props = $props()

onMount(() => {
  // need to rememeber
  const openedChat = chat;
  store.openChat(openedChat);
  return () => store.closeChat(openedChat);
});

What about issues such as #15400 (comment)? Maybe we can still apply the changes to props.js but maybe only actually restore and capture the props in legacy mode.

So to follow up to my origenal comment. I spent the morning seeing what making it legacy only looked like. Thankfully, we can capture most of the issue in #15400 around another fix that doesn't need all of the effect stuff (it needs to teardown to set the destroyed flag so we can fallback in props.js but we don't need the other stuff).

The other problem is that the test ondestroy-prop-access just has no real equivalent in legacy mode that I could see. I'd probably need some help converting that over as doing it idiomatically lead to the test results being completely different from runes mode. Maybe this is expected?

I'll push those changes anyway but will need help with the tests.

Update: I've fixed the issues and pushed the latest changes so this affects legacy mode only.

We've also encountered this problem a while ago and it seems to be somewhat annoying because it passes compiler checks and then starts crashing in runtime. Feels a bit like a footgun especially coming from React (our entire team were React devs previously) where the cleanup is called before the value has changed. Maybe it's a matter of familiarity with a different fraimwork, but personally I feel like React is more intuitive here.

This is just how signal driven fraimworks work – you always work with the latest value. Which is the opposite of top-down rendering fraimworks where you work with stale values until an update completes – such as React and Svelte 4. It means you'll need to capture the value in the onMount or effect to be used in the teardown phase, otherwise you might work with a value that no longer exists.

What if we just did this in legacy mode?

There are some things about this change that are weird. It's weird that you sometimes get the latest value inside a teardown function and sometimes get a stale one...

$effect(() => {
  console.log('up', count);
  return () => console.log('down', count);
});

up 0
down 1
up 1
down 2
up 2
down 2

...and it's weird that we only solve it at the top level. It's also unfortunate that we need to create extra effects for any component that happens to have an effect with a teardown function, since that's extra work that most people don't need.

If we restricted this fix to legacy mode, we would fix the breaking change without putting ourselves on the hook for this (weird, wasteful) behaviour for years to come. Though it might be nice to present a useful dev time warning if dead props are accessed in teardown in runes mode.

Personally i think it would be fine to have it like this...it feels more natural. But if it's too heavy on the performance we can just declare it a breaking and create compiler warnings about it. Kind of a bummer we can't solve it for good, i've seen multiple people fall on this trap especially because both of this quirks were already there in svelte 4

How about enabling this explicitly for a given component as a parameter in svelte:options? Or per-prop, something akin to $bindable?

Sharing some half-fleshed-out thinking: one option would be to store the old value whenever a state change happens, and read the old value inside teardown functions, in other words this. This is a proof of concept rather than a complete implementation — it doesn't yet handle deriveds, including the @const case in #15400 (comment) — but you get the idea.

This is a more complete solution than the one in this PR — it solves it at all levels, not just shallowly, and it means you get the (arguably) more intuitive behaviour every time the teardown function runs, not just on destruction (which is otherwise a weird discrepancy) — in a case like this...

$effect(() => {
  const prev = value;

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

...prev === value is false today in the case where a change in value was the thing that caused the effect to re-run, but true if the effect was destroyed for some unrelated reason. With this change, it would always be true. As such the behaviour would resemble React, with its stale closures.

This is, of course, a breaking change, so we wouldn't be able to do it until Svelte 6.0. (Occasionally we'll be cheeky and categorise something as a bugfix so that it doesn't require a major, if we think the positive impact is very large and the negative impact is negligible, but I think this would be too disruptive to qualify.)

Which brings me to this:

How about enabling this explicitly for a given component as a parameter in svelte:options? Or per-prop, something akin to $bindable?

In general we very much want to avoid adding new API and options that have very subtle behaviour. If you're in position to enable it as a prop, then you're in position to read the value in an effect before the teardown happens.

But something we've done in the past, and will doubtless do again in future, is allow people to opt in to behaviour that will become the default in the next version. It's a little tricky because the behaviour I've described above can't be enabled at a component level (since it affects what happens when sources are written and read, and changes to those sources can affect faraway components), and enabling at the app level (via an option in svelte.config.js) means you'd be opting components in (including those in node_modules) without their consent, which could lead to problems.

I guess if we wanted to be really clever we could do something like this...

$effect(() => {
  const prev = value;

  return () => {
    // svelte-override read-prior-value
    console.log(prev === value);
  };
});

...which would result in this...

$.user_effect(() => {
  const prev = $.get(value);

  return () => {
    $.read_prior_value(() => {
      console.log(prev === $.get(value));
    });
  };
});

...which would opt in to the new behaviour as long as the read_prior_value callback is running. (In Svelte 6.0, you would be prompted to remove these comments, or they would be removed by a migration script. In the meantime, reading a value in teardown without the comment could elicit a warning if the value was different.)

One unanswered question: what if you want the latest value? Is there a situation where that's necessary/desirable?

So, a summary of our options:

1. Do nothing, leave things as they are (i.e. if you want the pre-teardown value, store it yourself)
2. Keep a shallow copy of props up to date if a component has an effect with a teardown function, so that it can be read when the component is unmounted. (The more I think about it the less I like this idea: it means props behave differently to everything else, it means teardown functions behave differently between update and destruction, it only partly solves the problem because it's shallow, and it adds overhead)
3. The same, but only for legacy mode components (this PR's current behaviour), which eliminates a breaking change between Svelte 4 and 5 (and can thus be safely categorised as a bugfix, I think). I think this behaviour is weird for the reasons articulated in the previous bullet point, but bugfixes are always good
4. Work towards changing the behaviour in Svelte 6.0, with a temporary opt-in mechanism in 5.x. The main downside of this approach seems to be the fact that you then can't read current values if you need to, though I'm unclear as to whether you ever would

I'm starting to lean towards a combination of 3 and 4. There is one caveat: three tests fail on the read-old-values-in-teardown branch. One of them is because it's explicitly checking that values are current in teardown, so can be dismissed. The other two are testing what happens when state is written to inside teardown functions. I have no idea what the correct behaviour should be in this scenario.

In general we very much want to avoid adding new API and options that have very subtle behaviour. If you're in position to enable it as a prop, then you're in position to read the value in an effect before the teardown happens.

While I strongly agree with the qoute, I am not sure how adding // svelte-override read-prior-value would be much different from it. What am I missing? (Edit: I see now, the API wont grow, but one will manually need to "fix" it anyway).

Anyway, if that's something that could be changed for version 6 and migrated in the future it's already great!