state_unsafe_mutation where it shouldn't be?

[gyzerok]

Describe the bug

Getting a state_unsafe_mutation error for the following piece of code. To me it feels like nothing is inherently wrong with it.

Surprisingly enough we are using this pattern a lot and everything worked fine. However today I've decided to force runes mode through svelte.config.js and a lot of things got crashed with this error.

Reproduction

Breaking REPL

<script>
	import { SvelteMap } from 'svelte/reactivity'

	const source = ['hey', 'ho']

	const computed = $derived.by(() => {
		const map = new SvelteMap()

		for (const item of source) {
			map.set(item, item)
		}

		return map
	})
</script>

{#each computed.values() as item}
  {item}
{/each}

Working REPL - this is how we use it in our codebase and it seems to work until you do compilerOptions: { runes: true }.

<!-- App.svelte -->

<script>
	import { Model } from './Model.svelte.js'

	const model = new Model()
</script>

{#each model.computed.values() as item}
  {item}
{/each}

// Model.svelte.js

import { SvelteMap } from 'svelte/reactivity'

export class Model {
	source = ['hey', 'ho']

	computed = $derived.by(() => {
		const map = new SvelteMap()

		for (const item of this.source) {
			map.set(item, item)
		}

		return map
	})
}

Logs

System Info

irrelevant

Severity

annoyance

[paoloricciuti]

You are setting state in a derived which is forbidden...since the error is not thrown in legacy when you don't force runes mode globally you are reading the derived in a legacy component (as there are no runes) and it doesn't throw...but, unless you are really doing something different: why are you even using a svelte set here? Or at least, why it is a derived since it's not using any state inside?

[7nik]

-		const map = new SvelteMap()
-
-		for (const item of this.source) {
-			map.set(item, item)
-		}
-
-		return map
+		return new SvelteMap(source.map(item => [item, item]))

[gyzerok]

@paoloricciuti it looks like this issue in the end highlights several potential problems, so I'll try to write them up separately, please bare with me here.

Unexpected legacy mode

We are developing our project for almost a year now and it was started as a Svelte 5 project from the beginning. It came as a surprise to me today, that our code is partly running in legacy mode.

Personally I find it counter intuitive and problematic. It's great that when you migrating an existing project, runes and legacy mode could run side by side. But for a freshly generated project I (and arguably everyone) would expect to run purely in runes mode, because legacy will be dropped in 6 and it doesn't make sense to rely on it for a fresh project.

For all these months when we were hitting bugs and I were hunting for repros never have I ever took into account that it could be connected to the legacy mode.

Do you think it'd make sense to adjust project generation such, that it'd force runes mode by default?

Setting state in derived is forbidden

That makes total sense to me... until in this specific case it does not 😄

As I understand it, deriveds are sort of a pure functions and thus must adhere to the rules of pure functions. Meaning that, as you are saying, mutating state inside should not be permitted. And I am fully onboard with that. However if we think about pure functions, mutating state created inside these functions does not make them less pure.

If we look at the diff from @7nik (thanks for your suggestion) in practice it is exactly the same code, just different syntactically. And there are a lot of cases where writing it the way I wrote it in my reproduction might be more practical.

Combined with the legacy issue described above it's hitting us hard. If we would know beforehand that we need to enforce runes, we would do so and discover this problem right away. Now it comes sort of a breaking change for us. We can't enable runes mode entirely without rewriting a lot of our code.

So here let me end with another question: do you think lifting this rule for state that was created inside the derived is sensible and could be done?

What we are trying to achieve

Yeah, I understand that my repro doesn't make much sense, but it mostly due to it being minimal as possible. Let me describe how and why we are using deriveds here in a separate comment, that one is already big one.

[dummdidumm]

I haven't looked at the repro yet but I believe Dominic had an unsafe API idea which would allow deriveds to mutate state. #14784

[gyzerok]

To give you a bit of context we are building a realtime application (not yet offline-first, but we are going there) with global state. It's a single page app and the nature of it is such that lots of places in the UI depends on various pieces of data. So it's not like we don't want to separate things it's just how it is.

Since we are a realtime app we need optimistic updates (+rollbacks) and since we want to store local models in the database we need serializable data. At the same time we are making great use of classes with runes. Lets not go deeply into why we went with classes, the relevant part here is that we use them.

So after couple of iterations on our architecture we ended up with the idea that we wrap our api models as plain objects into classes which represent our "view models".

abstract class AbstractModel<ServerModel> {
  private version: number = $state.raw(Number.MIN_SAFE_INTEGER);
  private server: ServerModel;

  constructor(server: ServerModel) {
    this.server = server;
  }

  protected get $(): ServerModel {
    this.version;
    return this.server;
  }

  rollback(): void {
    this.version += 1;
  }

  update(next: ServerModel) {
    this.server = next;
    this.version++;
  }
}

type ApiUser = {
  id: number;
  name: string;
  isVerified?: boolean;
}

class User extends AbstractModel<ApiUser> {
  get id() {
    return this.$.id
  }

  name = $derived(this.$.name)

  // this also helps us fix some inconsistencies with the API,
  // or do some stricter types for it with union types in some places
  isVerified = $derived(this.$.isVerified ?? false)
}

async function updateUserName(user: User, newName: string) {
  // writable deriveds let us optimistically update model here
  user.name = newName

  try {
    const res: { user: ApiUser } = await fetch(...);

    // here then we replace user in cache which invokes recalculation of deriveds
    // and everything is consistent
    user.update(res.user)
  }
  catch {
    // in case of error we can rollback and keep our state consistent,
    // which in turn recalculate deriveds and revert the UI to the correct state
    user.rollback()
  }
}

As you can see it gives us all the benefits we were looking for:

1. use classes
2. easily serialize server state (it's plain object) and save/read it from db
3. and writable deriveds (truly amazing feature) give us super pragmatic way to implement optimistic updates and rollbacks

Hopefully everything does make sense so far, now lets look why we might want to use SvelteSet and SvelteMap inside our dervieds.

Lets imagine that user has friends and we want also to be able to update them optimistically:

type ApiUser = {
  // ... prev fields
  friendIds: number[]
}

class User extends AbstractModel<ApiUser> {
  friends: SvelteMap<number, User> = $derived.by(() => {
    const set = new SvelteSet()

    for (const id of this.$.friendIds) {
      const user = // get user from global cache by id
      set.add(id, user)
    }

    return set
  })
}

async function addFriend(user: User, friend: User) {
  // optimistically update friends and because it is SvelteMap it will propagate to the UI
  user.friends.add(friend.id, friend)

  try {
    const res: { user: ApiUser } = await fetch(...);

    // here then we replace user in cache which invokes recalculation of deriveds
    // and everything is consistent
    user.update(res.user)
  }
  catch {
    // in case of error we can rollback and keep our state consistent,
    // which in turn recalculate deriveds and revert the UI to the correct state
    user.rollback()
  }
}

From my perspective we aren't doing anything bad here. On the contrary we are trying to utilize svelte features as much as possible to have a great DX. And we find it amazingly robust. Our code is simple and pragmatic while we are solving complex problems.

However the problem now is that if I turn the runes mode on, a lot of our models are violating unsafe_state_mutation rule and the app is entirely broken.

[gyzerok]

I haven't looked at the repro yet but I believe Dominic had an unsafe API idea which would allow deriveds to mutate state. #14784

I believe that here it's completely safe and there is no need to use something like this.

Also unfortunately that PR broke another pattern we used. Which was insert-on-read for caches. In same places our cache is written such that when you read value from it if none exists it'd insert a default one and return it straight away. And to avoid state_unsafe_mutation there we are using untrack.

While it is a bit questionable I believe that insert-on-read is also a valid thing to do and should be acceptable if you know what you are doing.