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]

🦋 Changeset detected

Latest commit: 89780e3

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]

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?

[gyzerok]

It's called unsafe and this explanation leaves me wondering how to ensure that my usages of it are safe in practice. Maybe the idea is that if you use it, you should already understand it. However maybe there could be a bit lengthier explanation?

[Ocean-OS]

There'd probably be a more detailed description in the online documentation, perhaps in the $derived page.

[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.