fix: make untrack behave correctly in relation to mutations

[trueadm]

For a while untrack has been a bit wonky/buggy. It's designed to prevent reads of reactive signals being tracked, but due to its buggy implementation it also causes three quite nasty unintended problems that this PR fixes:

• it allows mutations inside deriveds and the template
• it prevents us from connecting effects and other deriveds created inside from being connected to their parents, leading to memory leaks and performance problems. This is because the current implementation sets active_reaction to null, causing all mayhem in doing so when it comes to connecting things to their parents
• unsafe ensures graph consistency is kept for effects, something that was previously not true with using untrack this way

This PR fixes that, by making sure untrack only stops reactive signals from being tracked. However, there might be genuine cases where someone knows that they can mutate state inside a derived or template and know it can occur, even if it's somewhat unsafe.

So I've also exposed an unsafe API from svelte that allows these use-cases. This also allows us to fix a few tests that made use of the previous behaviour of untrack and also the store logic that needs this too (although that is internal). unsafe could also be useful to unblock those who incorrectly have been misusing untrack today to emulate the same behaviour. Furthermore, unsafe actually resolves the reactive graph for effects, by ensuring the re-schedule and correctly sync up and ensure UI consistency – something untrack never did before. This PR uses unsafe to fix all of that.

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 89780e38e8504640ed8ac9ff049f7ace085b371c

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

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

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

[Rich-Harris]

preview: https://svelte-dev-git-preview-svelte-14784-svelte.vercel.app/

this is an automated message

[github-actions[bot]]

Playground

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

[leonidaz]

@trueadm just for clarity:

• so, unsafe() purely allows mutations inside deriveds and effects? It doesn't actually prevent subscriptions during signal reads, where untrack should still be used. So you can end up with untrack callback and inside there calling unsafe. Is this correct?

• untrack is just fixed - and now it only prevents subscriptions during signal reads but does not allow mutations in the callback, unless wrapped in unsafe

• using untrack inside unsafe callback would this also be allowed?

[trueadm]

@Leonidaz unsafe is for deriveds or template expressions only. You could always mutate state inside effects.

untrack is just fixed - and now it only prevents subscriptions during signal reads but does not allow mutations in the callback, unless wrapped in unsafe

Yep

using untrack inside unsafe callback would this also be allowed?

Yes.

[gyzerok]

@trueadm since we are using untrack inside class get functions, I think this one would be affecting one of the core ideas behind how our state management is done. So just wanted to make sure I do understand how it impacts us correctly.

We are heavily using read-through cache idea, which in simplified version looks as follows.

class Cache {
  cache = new SvelteMap()

  getLazy(id: string): Model | Promise<Model> {
    const model = this.cache.get(id)

    if (!model) {
      const promise = this.fetchModel()

      untrack(() => {
        this.cache.set(id, promise)
      })

      return promise
    }

    return model
  }
}

// then in ui

{#await modelCache.getLazy(id) then model}
  <div>{model.name}</div>
{/await}

From your description I assume we need to swap untrack with unsafe. Would you mind elaborating a bit more what that would change for us? Will there be some unexpected side-effect that we need to consider when updating?

[leonidaz]

@trueadm since we are using untrack inside class get functions, I think this one would be affecting one of the core ideas behind how our state management is done. So just wanted to make sure I do understand how it impacts us correctly.

We are heavily using read-through cache idea, which in simplified version looks as follows.

class Cache {
  cache = new SvelteMap()

  getLazy(id: string): Model | Promise<Model> {
    const model = this.cache.get(id)

    if (!model) {
      const promise = this.fetchModel()

      untrack(() => {
        this.cache.set(id, promise)
      })

      return promise
    }

    return model
  }
}

From your description I assume we need to swap untrack with unsafe. Would you mind elaborating a bit more what that would change for us? Will there be some unexpected side-effect that we need to consider when updating?

this shouldn't affect this unless this instance was called from the template effect or inside a derived. basically, here you don't want getLazy to subscribe to the signal which is the correct usage of untrack.

The bug with untrack was that it allowed mutations inside or if called from derived or template effect - this is a super discouraged practice but some people were using it as hack, which now they'd have to also use unsafe for mutations. It doesn't look like your code would be affected judging by this example.

[gyzerok]

@Leonidaz I've updated original example. In our case .getLazy is used in UI code (think you call it template effect) and inside deriveds

[leonidaz]

Playground

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

created a repl with this pr

I don't see it being affected as there'd be a mutation warning / error from svelte.

I'm not sure how exactly this works since technically it is called from the template effect. maybe because of await or how SvelteMap works.

But basically, if you apply this pr to your code base, you may start getting mutation errors, if you don't, then you're ok.

[trueadm]

@gyzerok I think you should be able to safely switch to unsafe there and it should work as it does now. You'd have to confirm first, but you can try this PR out today by using https://pkg.pr.new/svelte@14784 as your Svelte dependency.

[leonidaz]

created a repl with this pr

I don't see it being affected as there'd be a mutation warning / error from svelte.

I'm not sure how exactly this works since technically it is called from the template effect. maybe because of await or how SvelteMap works.

But basically, if you apply this pr to your code base, you may start getting mutation errors, if you don't, then you're ok.

The repl was not in runes mode, gets me a lot.... discovered it by accident by defining a state at the top. this is super annoying to have it switch automatically even if you use SvelteMap or untrack, etc.

@gyzerok so with the runes mode 🤦‍♂️ indeed you get the unsafe mutation warning

So, yeah, if this lands, you're going to have to use unsafe like @trueadm mentioned. Hopefully, it would be an easy change.

Here's a repl with using unsafe to allow mutations from template effects.

I think since you're directly calling getLazy with id in the template, In your case you want to track the state inside getLazy so it's reactive when the map value changes. So, just changing untrack to unsafe should be good. But there may definitely be cases where you might want to use both to avoid extra subscriptions.

Despite having to make some adjustments, I'm glad that that this functionality would be split up as the purpose untrack is ambiguous otherwise.

[gyzerok]

@trueadm I've tried to install version from this PR and swapped untrack with unsafe in my code. Now I am getting a lot of Uncaught Error: https://svelte.dev/e/state_unsafe_local_read. And error stacks there are leading to really random places, which are just regular signals or deriveds, so I can't even tell what usages of unsafe or untrack are causing the errors.

[trueadm]

@gyzerok Looks like you'll need an untrack in there too with the unsafe if you're reading state you've created locally in a derived.

[dummdidumm]

Since people already rely on untrack having this weird side effect, we can't "fix" the behavior as it would break some code bases. I think we need to introduce unsafe on top without fixing untrack (maybe there's a way to detect whether or not it's used as a unsafe replacement and we can warn at dev time about it?), and fix untrack in Svelte 6.

This also needs elaborate documentation because it's probably hard to understand when you use which.

More generally, would there be a way to automatically detect that something is an unsafe mutation and put it in the respective category, without the user having to think about it?

[trueadm]

More generally, would there be a way to automatically detect that something is an unsafe mutation and put it in the respective category, without the user having to think about it?

Unsafe mutations will cause effects to invoke multiple times – which is not something the user would expect unless something like unsafe was used. Furthermore, checking for it each time is extra overhead, so avoiding it for the case where we don't have unsafe.

[leonidaz]

Since people already rely on untrack having this weird side effect, we can't "fix" the behavior as it would break some code bases. I think we need to introduce unsafe on top without fixing untrack (maybe there's a way to detect whether or not it's used as a unsafe replacement and we can warn at dev time about it?), and fix untrack in Svelte 6.

I think that the number of people / code bases using untrack in deriveds or template effects to specifically mutate state is pretty minimal and as I recall this practice was highly discouraged and without untrack there were warnings. So, essentially, people have been misusing / abusing untrack's buggy behavior as an undocumented feature - it's clearly not its intended use, even semantically. I think by far, most people use $effect for any mutations.

But even if this small number of projects have been implemented this way, it's trivial to fix them by adding unsafe as a wrapper around mutations. Maybe, when installing a new version (e.g. npm install, npm update), a warning can be printed in postinstall to notify people that if they're mutating state in deriveds or in template effects to wrap these operations in unsafe.

I think the benefits of solving the issues outlined by @trueadm for most of the codebases that were properly using untrack far outweigh the drawback of quick updates for a tiny minority of projects:

• it prevents us from connecting effects and other deriveds created inside from being connected to their parents, leading to memory leaks and performance problems. This is because the current implementation sets active_reaction to null, causing all mayhem in doing so when it comes to connecting things to their parents
• unsafe ensures graph consistency is kept for effects, something that was previously not true with using untrack this way

[paoloricciuti]

I think the key here is that this was undocumented so not an intended feature...so imho this could be a fix rather than a breaking...but it's definitely subtle.

[Ocean-OS]

Since people already rely on untrack having this weird side effect, we can't "fix" the behavior as it would break some code bases. I think we need to introduce unsafe on top without fixing untrack (maybe there's a way to detect whether or not it's used as a unsafe replacement and we can warn at dev time about it?), and fix untrack in Svelte 6.

Since there is now a variable that is changed through untrack (instead of just clearing the active reaction), we could just check if untrack is currently running when $.set is called and warn/recommend using unsafe.

[trueadm]

Since there is now a variable that is changed through untrack (instead of just clearing the active reaction), we could just check if untrack is currently running when $.set is called and warn/recommend using unsafe.

But untrack is where you mutate something in an effect without wanting to track the read as a dependency? i.e.

untrack(() => {
  count++;
});

[Ocean-OS]

But untrack is where you mutate something in an effect without wanting to track the read as a dependency? i.e.

untrack(() => {
  count++;
});

Yes, in the example you gave, it would compile to $.update(count), which uses $.set under the hood. If $.set were to be called in untrack, it should warn, since unsafe should be used for that.

[trueadm]

Yes, in the example you gave, it would compile to $.update(count), which uses $.set under the hood. If $.set were to be called in untrack, it should warn, since unsafe should be used for that.

unsafe is for mutating state inside a derived, not an effect. It's fine to mutate state inside effects. So untrack is correct here as count++ reads count and then increments it.

[paoloricciuti]

It's fine to mutate state inside effects.

Don't let them know 😱 It's absolutely unsafe to mutate state in effect and you should never do it or you'll be fired 😄 😄 😄

Joking aside...what id we also check if the active_reaction is an effect and only warn if it's not?

[trueadm]

Joking aside...what id we also check if the active_reaction is an effect and only warn if it's not?

You can also mutate state when active_reaction is null and we already throw an error when it's a derived – or if it's a block effect. I don't get what the warning would do.

[gyzerok]

Pitching in, since we are one of these people using untrack in an unexpected way.

I'd agree with Simon that changing it's behavior even though undocumented is a breaking change for any current project. It'll effectively stop our ability to update to newer versions until we figure out how to adapt to newer reality. This means no bugfixes, performance fixes or memory leak fixes in the mean time for us.

On the other hand speaking about us - we are closely following every release and are willing to update asap. The problem is, I still do not really understand how this change will impact our codebase.

The main (probably only) place where we are using untrack inside deriveds is get-or-insert operations for SvelteMap. In my opinion this is a sensible operation and a lot of languages have it included in their std Map implementations. So I'd advocate to treat is as something that should be possible in Svelte reactive system without hacks.

In the spirit of Svelte I am also wondering if it's possible to internally change untrack behavior such, that it's working somehow differently in derived and consumers do not need to have any gotchas about it?

[trueadm]

We decided that this is most definitely going to be a breaking change so it will have to wait for Svelte 6. In the mean time, I've tried to alleviate some of the issues mentioned in https://github.com/sveltejs/svelte/pull/15065.

[gyzerok]

@trueadm I am wondering if it would still make sense to expose unsafe so people like us who are using untrack inside deriveds can transition and get the benefits of not using untrack for this cases. What do you think?

[gyzerok]

If you decide to do this I would happily test unsafe in our codebase before it gets merged

[leonidaz]

@trueadm I am wondering if it would still make sense to expose unsafe so people like us who are using untrack inside deriveds can transition and get the benefits of not using untrack for this cases. What do you think?

yeah, I think if a compilation version, or feature flags, could be introduced so we can start using some of the v6 changes ahead of time, it would be amazing. Not sure how practically this is possible but unsafe cannot be really exposed without changing how untrack works.

[leonidaz]

would be really great to have this, even behind an experimental flag. Currently, doing some updates in deriveds doesn't work.

e.g. repl with $inspect

• click on increment linked, then on increment count, error derived_references_self is thrown. commenting out $inspect(linked) fixes it.

repl without $inspect

• same steps as above click on increment linked, then on increment count, then again on increment linked, the incrementing doesn't work, unless line (15) in the instance code is commented out: linked.current++;

With this pr, no error and incrementing works as expected although $inspect on the derived only prints it the on init repl