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

Svelte5: Deep reactivity is unintuitive with reactive Maps #11346

Open
ottomated opened this issue Apr 27, 2024 · 16 comments
Open

Svelte5: Deep reactivity is unintuitive with reactive Maps #11346

ottomated opened this issue Apr 27, 2024 · 16 comments

Comments

@ottomated
Copy link

Describe the bug

Intuitively, I would expect this to be reactive:

import { Map } from 'svelte/reactivity';
const s = $state(new Map([[0, { nested: 0 }]]));

// somewhere:
s.get(0).nested++; // should be reactive

However, reactive Maps (and Sets) don't proxy their values by default. This breaks the intuition that values passed to the $state rune are made deeply reactive. I think even if this is out of scope for the reactivity helpers, it should be clearly documented.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACm2SzW6DMAyAX8WKKgEqgq7SLkCZdtyhe4GxQ0bNlAlClLidqijvvvDTrXQ95GDnsz_LiWWNaNGw7M0yyTtkGXtWisWMzmoIzAlbQh-b_qjrIVOYWgtFZSUrEp3qNYGFPVfgoNF9B8FUkmrkNYmToHOQD-xwWiToP76wpgfYwcoQJwwtdFxlYB24KF9S23-UxO9BFkYz7PEVNo1nQ5_blWBHF82WxBcl3HexcOLtETPYgMuvke2IGKQw4EG84C6CiupeGgLh-3jwRRJqTy2FN8pkbLNe58vbyfY52aIbyMXwuNlEc6SRjlrCJKlb5PpXLCZmXlcli_TvTWShylc0hId5h36zi8meJqsrUnXh9-P-7014DftP0PUH0Qg8sIy0z767H5Uo1qc_AgAA

Logs

No response

System Info

n/a

Severity

annoyance

@paoloricciuti
Copy link
Contributor

I had the same thought but I think they want to make it explicit. However you don't need to pass new Map to state, you can just create the new Map and if you want some object to be reactive inside it you should create it with $state

@sheijne
Copy link

sheijne commented Apr 28, 2024

As mentioned writing $state(new Map()) does not make sense. $state intentionally does not make things like Map reactive. That's what svelte/reactivity is for.

I would, however, expect it to work without wrapping it in a $state:

const map = new Map([['foo', { deep: 0 }]]);

map.set('bar', { deep: 0 });

// I would expect both of these to be reactive:
map.get('foo').deep++;
map.get('bar').deep++;

Though technically this is possible:

const entries = $state([['foo', { deep: 0 }]]);
const map = new Map(entries);

let bar = $state({ deep: 0 });
map.set('bar', bar);

// Both of these are reactive:
map.get('foo').deep++;
map.get('bar').deep++;

That feels like a painful amount of boilerplate to achieve the same, and it seems prone to error as you could end up with some values being reactive and some not reactive:

const map = new Map();

let foo = $state({ deep: 0 });
map.set('foo', foo);

let bar = { deep: 0 };
map.set('bar', bar);

map.get('foo').deep++;
map.get('bar').deep++; // Oops, I did a dumb dumb, and now this is not reactive. Happy debugging!

All this makes me wonder if this works as designed, or if it could/should be improved.

@paoloricciuti
Copy link
Contributor

As mentioned writing $state(new Map()) does not make sense. $state intentionally does not make things like Map reactive. That's what svelte/reactivity is for.

I would, however, expect it to work without wrapping it in a $state:

const map = new Map([['foo', { deep: 0 }]]);

map.set('bar', { deep: 0 });

// I would expect both of these to be reactive:
map.get('foo').deep++;
map.get('bar').deep++;

Though technically this is possible:

const entries = $state([['foo', { deep: 0 }]]);
const map = new Map(entries);

let bar = $state({ deep: 0 });
map.set('bar', bar);

// Both of these are reactive:
map.get('foo').deep++;
map.get('bar').deep++;

That feels like a painful amount of boilerplate to achieve the same, and it seems prone to error as you could end up with some values being reactive and some not reactive:

const map = new Map();

let foo = $state({ deep: 0 });
map.set('foo', foo);

let bar = { deep: 0 };
map.set('bar', bar);

map.get('foo').deep++;
map.get('bar').deep++; // Oops, I did a dumb dumb, and now this is not reactive. Happy debugging!

All this makes me wonder if this works as designed, or if it could/should be improved.

They were originally designed with deep reactivity and changed back to be explicit because Dominik thought not having a $state anywhere was too magic.

I kinda agree with you that it would be nice to have it.

@sheijne
Copy link

sheijne commented Apr 28, 2024

Ah, I see, that makes sense. I do understand the reasoning, and I can certainly agree with it to some extend. It's how I feel about the entirety of svelte/reactivity actually. Personally I think it's weird to have primitives for $state and $derived, and having to import a magical Date/Map classes from svelte/reactivity (though it's nice to get tree-shakeable implementations of the primitives and such!), but maybe that's another discussion.

Back on topic, I fear we'll end up seeing a lot of this in the wild:

import { Map } from 'svelte/reactivity';

class ReactiveMap extends Map {
    constructor(entries) {
        let e = $state(entries);
        super(e);
    }
    set(key, value) {
        let v = $state(value);
        super.set(key, v);
    }
}

export { ReactiveMap as Map };

It does the trick, but it leans toward writing JavaScript pre 2015, where everyone was writing the same utility functions because the base library was lacking.

Do you have a reference to any discussions about deep reactivity in maps/sets? I'd love to read about the reasoning, and I was looking in #10263, but some quick searches left me empty handed.

@ottomated
Copy link
Author

They were originally designed with deep reactivity and changed back to be explicit because Dominik thought not having a $state anywhere was too magic.

Would it be possible and desirable to enable deep reactivity if the Map is wrapped in a $state rune? Or even to automatically replace normal Maps, Sets, Dates, etc. with their reactive versions if they're put inside a $state rune?

@harrisi
Copy link

harrisi commented Apr 29, 2024

Would it be possible and desirable to enable deep reactivity if the Map is wrapped in a $state rune?

It may be possible, but would break the current logical semantics of how classes work, since when you try doing it with SomeOtherClass, the behavior is basically the opposite.

Or even to automatically replace normal Maps, Sets, Dates, etc. with their reactive versions if they're put inside a $state rune?

No, #10263 (comment).

@ottomated
Copy link
Author

It may be possible, but would break the current logical semantics of how classes work, since when you try doing it with SomeOtherClass, the behavior is basically the opposite.

eh, I disagree - you're explicitly importing Map from svelte/reactivity here, my mental model views it as a builtin versus a class

@sheijne
Copy link

sheijne commented Apr 29, 2024

@ottomated I believe that the point is that if you would do this:

class MyClass {
  #something = {}

  setSomething(value) {
    this.#something = value;
  }

  getSomething() {
    return this.#something;
  }
}

const myClass = $state(new MyClass());

It would not result in a "reactive class", which would cause a lot of confusion among developers if $state(new Map()) does result in a reactive class.

This is the subtle difference between magic and voodoo.

@Rich-Harris
Copy link
Member

Suppose we were to change things so that values were automatically replaced with reactive proxies — what would a good opt-out mechanism look like?

@ottomated
Copy link
Author

Suppose we were to change things so that values were automatically replaced with reactive proxies — what would a good opt-out mechanism look like?

$state.frozen(new Map()) is my intuition, but then my intuition was also that $state(new Map()) would be required.

After using svelte 5 for a few projects, I've built the habit of wrapping anything that needs to be reactive in a rune of some kind. It feels a bit more magic than magical that svelte/reactivity hides that away. Maybe the syntax could be

import { $map } from 'svelte/reactivity';

const map = $map([['key', 'value']]);
//    ^? Map<string, string>

// opt-out
const shallowMap = $map.frozen();

But then you would need to do $state({ map: $map() }) which would definitely need at least some lint rules to feel right.

An opt-out mechanism could not be something like new ReactiveMap([], { deep: false}) though. I don't think you can deviate from the base class's public API at all.

@Rich-Harris
Copy link
Member

Yeah that doesn't seem like an improvement frankly. For one thing what is a frozen map? It implies you can't set or delete etc.

let bar = $state({ deep: 0 });
map.set('bar', bar);

This doesn't seem like a painful amount of boilerplate to me, it seems like a reasonable way to opt in to the desired behaviour.

I don't think you can deviate from the base class's public API at all.

And this is basically what makes me uncomfortable about the proposal to make values reactive by default. This...

let foo = { deep: 0 };
map.set('foo', foo);

console.log(map.get('foo') === foo); // false!

...violates my expectations a bit too much.

@ottomated
Copy link
Author

ottomated commented May 1, 2024

For one thing what is a frozen map? It implies you can't set or delete etc.

Sure, frozen is a bad word for it - could be $map.shallow, or default to shallow and add $map.deep. More important than the semantics is the underlying idea of using rune-like names for svelte/reactivity exports, would like to hear your thoughts on that or if the API is already too set in stone.

This doesn't seem like a painful amount of boilerplate to me, it seems like a reasonable way to opt in to the desired behaviour.

I don't think it's a boilerplate issue, I think it's an expected behavior issue. $state being proxied by default makes this really hard.

// this works
const obj = $state({ });
obj.foo = { deep: 0 };

// this doesn't - but it feels the same!
const map = $state(new Map());
map.set('foo', { deep: 0 });

And this is basically what makes me uncomfortable about the proposal to make values reactive by default.

I would call that a different thing than changing the public API. I would also argue that this is much weirder:

const map = $state({});
let foo = { deep: 0 };
map.foo = foo;
	
console.log(map.foo === foo); // false!

@Rich-Harris
Copy link
Member

More important than the semantics is the underlying idea of using rune-like names for svelte/reactivity exports, would like to hear your thoughts on that or if the API is already too set in stone.

Names beginning with $ are prohibited, because if you can do this...

import { $map } from 'somewhere';

...then you can also do this, which is just a recipe for silliness:

import { $state } from 'somewhere';

We could say 'you can have names beginning with $ except for existing runes', but then we'd be forgoing the ability to add new runes without breaking changes, which would be a grave error.

The alternative is to make $map and $set etc actual runes, but that would also be a big mistake. For one thing, it dilutes the 'rune' concept to mean gestures vaguely towards 'reactivity' rather than 'an instruction to the compiler'. For another, it sucks from a day-to-day usability standpoint if you start typing $s or $d and the first autocomplete suggestions are $set and $date rather than the far more commonly-used $state and $derived.

Finally, we want to avoid the impression that there's something special about these classes — Map and Set and Date and URL are things that you could build in userland. We're making that unnecessary, because we're a batteries-included framework, but the point is that you can use Svelte's primitives to make your own reactive classes. Using runic notation would undermine that message.

I would also argue that this is much weirder

It's not weirder. At most, it's equivalently weird, but I'd argue that big-blob-of-reactive-state is a more appropriate place for that sort of weirdness than something that, aside from causing effects to rerun on updates, is designed to exactly replicate built-in APIs.

@ottomated
Copy link
Author

ottomated commented May 1, 2024

Names beginning with $ are prohibited

This makes sense.

The alternative is to make $map and $set etc actual runes, but that would also be a big mistake.

This was my thought too.

Finally, we want to avoid the impression that there's something special about these classes — Map and Set and Date and URL are things that you could build in userland. We're making that unnecessary, because we're a batteries-included framework, but the point is that you can use Svelte's primitives to make your own reactive classes. Using runic notation would undermine that message.

I'm trying to reconcile my thoughts this, because at heart I agree with this as well. Here's the difference, I think:

A. Some people using Svelte 5 will only use deep reactive state, because that's what they're used to in Svelte 4. When those people see svelte/reactivity, they expect it to be deeply reactive by default. They don't care that they can make their own reactive classes, and probably aren't seeing Map, Set, etc. as classes, but rather as builtins. In fact, default javascript Maps are "deeply reactive" in Svelte 4 if you use map = map.

B. Once people build an intuition for $state in custom classes, this behavior makes perfect sense. I would even put myself in group A when I submitted this issue, and reading the responses has converted me to group B. However, even if the documentation on this is expanded, I expect many svelte 5 adopters to be in group A.

Edit: another note on this - I looked at the source code several times while trying to understand this at first and it doesn't avoid that impression - it uses internal methods rather than runes.

It's not weird_er_. At most, it's equivalently weird, but I'd argue that big-blob-of-reactive-state is a more appropriate place for that sort of weirdness than something that, aside from causing effects to rerun on updates, is designed to exactly replicate built-in APIs.

In the hierarchy of potential unintuitiveness, I think getters and setters on an object you defined yourself is higher than method calls on a class you imported, if that makes sense?

Side note: I wonder if it would be possible to patch the AST i.e. s.foo === foo -> $state.snapshot(s.foo) === foo and maintain equality this way.

@Rich-Harris
Copy link
Member

I looked at the source code several times while trying to understand this at first and it doesn't avoid that impression - it uses internal methods rather than runes

It does, just because it's one less moving part if we don't need to run the Svelte compiler on its own source code in order to run tests etc. It would be slightly more cumbersome to implement in userland since we don't expose source, but still totally doable.

Side note: I wonder if it would be possible to patch the AST

The thought has definitely occurred! It would likely help in cases like #11403. Reasons not to do it:

  • performance overhead, unless it's a dev-only thing that says 'you probably meant to use snapshot'
  • uncanny valley, if === works differently in .svelte.js vs .js (e.g. you're passing values to a library function that isn't aware of this)
  • to preserve the illusion you'd need to patch things like [].indexOf and [].includes, and likely many other things. it'd be a moving target and there'd always be ways to break it

Because of those, it's almost certainly better to just educate Svelte developers that foo.bar = bar; foo.bar === bar won't necessarily evaluate to true

@ottomated
Copy link
Author

It does, just because it's one less moving part if we don't need to run the Svelte compiler on its own source code in order to run tests etc.

Yeah, I understand why, just a note.

Because of those, it's almost certainly better to just educate Svelte developers that foo.bar = bar; foo.bar === bar won't necessarily evaluate to true

Yes, and that knowledge would extend to svelte/reactivity automatically.


I honestly think the best solution to all of this is something like this.

import { map, set, url } from 'svelte/reactivity';

const m = map.deep();
const shallow_set = set();
const u = url('http://example.com');

It solves the opt-out / opt-in issue cleanly, and it also solves a different footgun I ran into: if you refactor a chunk of code into a new component that includes a ReactiveMap constructor but forget to import ReactiveMap, you'll silently lose reactivity.

In addition (I assume this is already on the roadmap), the documentation should include more of these examples, there should be more lint warnings around this, etc.

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

No branches or pull requests

5 participants