-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Reactive Map, Set, Date and URL #10263
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
Comments
This seems weird for several reasons.
This just kind of feels like a mix of redefining builtins and providing a small lodash, both of which feel bad. It's also kind of a step back to default reactivity ( |
The implementation will extend the original classes and defer to them as much as possible. There will not be any additional API on top. The import is needed because else it's not treeshakeable. As pointed out above, the proxy implementation would need to check at runtime if the thing given to it is a Map/Set/whatever and make it reactive on the fly. An explicit import doesn't have this drawback and at the same time makes it very clear that anything non-POJO isn't reactive by default (and you can use your own wrappers or the ones provided by Svelte other stuff) |
Thanks for the reply! Another oddity I didn't think of until now is that this will also mean that if you import I'm still not sure I understand why this wrapper needs to be involved in user code, or why the current use of Also related to the proxy vs import, why isn't this method used for |
Making a wrapper (with classes) is technically what I'm doing until the |
Object and arrays can be created using literals (and more often than not are) import { Array } from 'svelte/reactivity'
const myArr = new Array('a', 'b', 'c', 'd')
// instead of const myArray = $state(['a', 'b', 'c', 'd']) or, in the edge case that you happen to want to initialise the array with a single number, the syntax will get quite ugly: import { Array } from 'svelte/reactivity'
const myArr = new Array(1).fill(3)
// instead of const myArray = $state([3]) Objects will probably become even worse. Assuming the relevant static methods have been (re)implemented in the reactive version, a simple object literal would become something like this. Note that you won't have any typescript support at all in this case: // At the moment:
// const myObj = $state({ a: 1, b: 2, c: 3 })
// With wrappers:
import { Object } from 'svelte/reactivity'
const myObj = Object.fromEntries([['a', 1], ['b', 2], ['c', 3]])
// Or
// const myObj = new Object()
// myObj.a = 1
// myObj.b = 2
// myObj.c = 3 The main difference between POJOs and Set/Map/Date is that the latter don't have a literal way to write them, but they require a constructor to make instances of those classes. The way to make those reactive would just be an import statement. I don't think it would be possible to convince Javascript that the object and array literal would need to create instances of other classes than the built in Array and Object class |
The object example isn't very fair, since it would just be: import { Object } from 'svelte/reactivity'
let myObj = new Object({a: 1, b: 2, c: 3}) For arrays, it's only odd if someone wants to make an array of example one number. This is a pretty specific situation. Regardless, the point isn't so much "is wrapping I hope that makes my point clear. I don't think wrapping |
I don’t have much technical say in this matter but not gonna lie it does kinda feel like this goes against the “vibe” so far |
Would you expect something like Now, if you think of |
No, they're completely orthogonal
Sure. Now do this :) import { get_map } from './somewhere.js';
let map = $state(get_map()); |
I can understand both sides: svelte devs want a straightforward elegant interface, while the maintainers also have to wrangle the technical feasibility and need of a general approach. I found the exchange very interesting so far and after pondering all arguments, I'd like to make a proposal as well: Allow a second declarative configuration object on the $state rune to define additional reactivity$state( obj, config = { addReactivity: ["Set", "Map"] } )✅ Declarative: opt-in config ✅ Safely Tree-Shakable ✅ Expandable at will (no breaking changes & no major versions changes required) ✅ Sweet Sweet Svelteness ✅ Self-Explanatory / IDE-hints ✅ Party-Friendly 🎉 Last but not least: I'm merely a humble, new student of svelte's internal workings, so hopefully this proposal may be valid and of use. |
This still can give issues with the example Rich gave before: // somewhere.js
const myMap = new Map()
let count = 1
myMap.set('foo', count++)
myMap.set('bar', count++)
export function get_map() {
return myMap
}
export function add_to_map(key) {
myMap.set(key, count++)
} <script>
// myApp.svelte
import { get_map, add_to_map } from './somewhere.js'
const map = $state(get_map(), { addReactivity: ["Map"] })
</script>
<button onclick={() => add_to_map('button pressed')}>Size: {map.size}</button> Whenever myApp gets used, all of the sudden the existing map would need to be changed to make it reactive |
Yeah an options argument would not work as expected in all cases because of reasons like this. Also, I personally would rather import the wrapper explictily compared to adding a second options argument where I end up typing more in the end (autocomplete will give me the wrapper import for free). |
Can I make a PR for Rich's proposal? I want to get more familiar with the internals and this seems like a pretty good opportunity |
I appreciate the feedback, but can't share your reservations: Rich's example (Map dynamically added during runtime) should be adequately covered by the proposed declarative opt-in config object, as it would be proxied just like Arrays and Objects are right now. Here's how it works:
import { get_map } from './somewhere.js';
let map = $state(get_map(), { addReactivity: ["Map"] } );
Hm what for? If Re: "type more": That's rather subjective. Keep in mind that manually adding a
Well, autocomplete will also give you the properties for the (btw: I also like the config object as it's extendable; at some point we might want to add other options, eg reactivityDepth)
In your example, is myApp a component that is used multiple times through the app and you want the same state used? In that case, wouldn't you need to import myMap from a
Unlike the Summarizing (IMO)
It'd be also interesting to hear from other devs who previously stated dissatisfaction having to use new wrappers to replace standard JS built-ins on the dev side ("the vibe is off") like @Antonio-Bennett @harrisi and others who liked their posts. |
This may work when you wrote the get_map function yourself, and it returns a new empty map, but if it is exported from a library (and e.g. turns out to be a sub class or proxy of Map already), it can't just be changed by the svelte compiler without giving all sorts of issues with that library
I was extending Rich's example, pretending that somewhere.js could be part of any JavaScript library, and you don't know what it looks like. I admit that this would be a silly implementation, but if it would automatically be able to proxy Maps by using $state, it should be able to handle those cases as well (not just the much easier function In fact, I don't think this would work with POJOs (as the proxy is only made in the svelte component, not in the somewhere library), which would make the behaviour as you expect it to be even weirder. Why would the map returned by get_map get proxified by the svelte compiler (which would mean that somewhere.js would have the proxified Map instead of the built in one), whereas POJOs returned by get_object would not be proxified inside of the library function? |
It seems like you're conflating different concepts. Primarily, you might be confusing this with Harrisi's suggestion that the compiler should add the
The Svelte compiler won't change it at all, nor was it ever intended to. That's the whole point of the proposed
No, that's just incorrect and not how svelte's $state works. (I admit it also tripped me up a bit when first examining svelte 5).
Well, in your example, if you expect the state to be shared among your components (and not passed via props, et.), then yes, it must be imported from a .svelte(.js) file, as $state is a compiler instruction, not a runtime function. (The compiler actually transforms it into the proxy function.) Also, it feels like you're venturing too far into hypothetical territory here, making it difficult to align our assumptions. 📌 |
I may have been thinking too much about them being a class That said, I'm wondering how the prototype chain of a proxy works. Would Not sure if I agree about the subclasses not being proxified. As I said, a library may specify they return a Map from a function, but return a subclass of a Map instead. In that case, if you would wrap that in a $state somewhere in your own code (with the config parameter to make it reactive), you may expect it to become reactive, which won't happen because it is a subclass. |
You still think to much about classes. :) You might want to check the
ECMAScipt's Proxy is made to be transparent and undetectable, so it doesn't have a different constructor.
It's nothing to agree with or not (I'd like custom classes support), but svelte currently doesn't allow for that yet. ( #9739 - Non-Pojo Section ).
Besides the error being with the bad spec, it would be the same behaviour you would get with $state of Btw, you can easily try out both of these things in the REPL yourself. 1. Make a proxy/$state of an array/object and check instanceof (or get the prototype) 2. try to use a custom class as the value of a $state. |
I was thinking about how the svelte compiler would know whether to proxify the value within $state or not based on the given config. |
I barely have time for anything these days, so please excuse any brevity or lack thereof, as well as any potential impoliteness. I'm just trying to provide some constructive criticism :)
So, yes, I would. Svelte would know because this is how proxies work, right? You just set up the
I don't see why this wouldn't work as expected when using proxies, but I also don't think I'd come across this often. I'm thinking that if I use a library that defines some class, I'd like to just do
This really confuses me. Why would I think of " All of this also feels like a regression because Svelte 4 doesn't have this issue. You can see it working with I think that is getting to the core of the issue for me; if this proposal happens, reactivity becomes less consistent. I don't mean to sound combative or anything, and I appreciate the work you all have done. I realize I'm likely fundamentally misunderstanding things and/or just plain wrong. I'm both very tired all the time due to life and not in expert in JavaScript. I'm extremely appreciative that this discussion could even happen, thanks to the public development you all have done. |
Indeed, there lies the crucial point: Good DX/UX is all about consistency. We all applaud the team for doing an amazing job consolidating Svelte 5 so far, taking it out of "magic" territory into idiomatic candyland. But the wrappers proposal is not consistent with anything Svelte did before, nor dev expectations or what lib-users would expect in general - risking to destroy that coherent elegance. Which is unnecessary, as there's at least one better solution. ( I think we agree that the first proposed adjustment - to have the compiler insert the wrapper during build - won't fit, as outlined by Rich, it can't handle dynamically added vars during runtime ). The $state config object, which we have thoroughly discussed, still holds valid as covering all requirements and, as far as I can tell, fits best with Rich's outlined svelte tenets. So, I see no reason - unless someone comes up with a completely new and better proposal - to not go with the optional Of course, it would be much valuable to get Grandmaster Svelte's (@Rich-Harris) assessment on all of this. |
It's impossible to implement this reliably using proxies. How would the proxy know that
This certainly is a gotcha. It probably makes sense to have a check here to treat reassignment of non-POJOs as an update trigger.
This isn't true.
|
Agreed, there are some built-ins (like Date) that are blocked from proxying and would need a wrapper. So yes, extra code is needed for those cases. Still, just like with signals, the actual implementation (wrapper) doesn't need to be exposed as a new concept to learn and as an additional responsibility for the developer to manage.
Let's see whether this is true or not by comparing. As previously stated the compiler only needs this additional
As we can see, only for objects Granted, once you have declared the import you don't need to write it again for that file. Yet whether a project uses many small components or one big file, whether they use many one-type values or POJOs depends on the project / dev methodology. |
Those examples with auto-configuration feel too magic. E.g. when I see the following code: let myState = $state({ buffer: new Int32Array() }); I'll ask myself: is |
Pardon me (it's been a long discussion), there was a slight oversight in the third row of the example (corrected now). As outlined in my original proposal > "Expandable at will (without breaking changes)", The correct opt-in approach for your example gives clarity and controllability at all time, without any unexpected breaking changes (devs need to opt-in explicitly for reactive built-ins, even if a new svelte version adds them). I hope you agree that the "magic is gone" now. let myState = $state({ buffer: new Int32Array() }, { addReactivity: ["Int32Array"] } ); Thanks for bringing this up! This could actually be a good addition in DEV mode: When a user uses a built-in inside $state() but has |
As far as I understand, in general, proxies work great for non-built-in classes and objects. They still work with built-ins, it's just a bit awkward. This would certainly make the implementation a bit more unwieldy, but I don't think by much, and I don't think it will ever need to be changed. Here is an example of a
If this happened then there wouldn't be a need for the custom I also wanted to just throw it out there to try to steer the discussion - I definitely do not think a config option is a good option. I think it has way too many problems and isn't relevant to this issue. Probably best to move that discussion elsewhere (although I don't really think it's worth having at all, honestly). |
Let's look at this example: let myState = $state( {}, { addReactivity: [ "Set"] } );
const set = get_builtin_set();
myState.set = set; Here, As for me, adding a general API that may cause altering of the assigned values is weird.
Why would you do About using proxy-wrappers vs derived classes. |
There's a flaw in my Also, the idea is that the compiler would add this, it wouldn't be written out like it is in the repl. The weird I'm also not hating the idea of going back to "reassignment is reactivity" for all objects (including plain objects and arrays). Easier, consistent, and basically makes everything into a transaction, which I'd never thought of before. But this would mean not making objects and arrays special. |
I wanted to share how I've been using reactive lists and dictionaries. Maybe CRUD functions like these could be incorporated into a special constructor for declaring lists and dictionaries: export const store = $state({
store: {
someString: "",
someList: [],
someDictionary: {}
},
initialize() {
//some logic
},
set(key, value) {
this.store[key] = value;
},
push(key, value) {
this.store[key].push(value);
},
remove(key, value) {
let index = this.store[key].indexOf(value);
if (index !== -1) {
this.store[key].splice(index, 1);
}
},
get(key) {
return this.store[key];
}
}) usage import {store} from "$lib/store.svelte.js";
store.get('someString') |
My 2 cents : Svelte 5 so far looks like an improvement compared to the existing reactive solutions that are using a But for this part this feels like a step backwards (vue reactivity works with both for example) As someone who uses Map and Set a lot, I would prefer the non tree-shakeable version over both proposal discussed here. For people who don't use Map and Set, maybe distributing alternative lighter versions of svelte is an acceptable solution. Regarding clarity of which objects works with what, I believe it's fine to support the most common ones and document which ones are not supported (could even emit in the console that Finally regarding Date ... a reactive date certainly would be nice, but I've personally never felt the need for it. Compared to Collections (Set,Map) where it's quite handy to reach for their builtin methods (especially .delete) to handle component state, it doesn't seem to me that using a reactive Date to handle component local state is very useful? The docs can also cover this and say to use re-assignment for this scenario. |
I noticed that in the reactive Set and Map methods like has(key) {
var s = this.#sources.get(key);
if (s === undefined) {
// We should always track the version in case
// the Set ever gets this value in the future.
- get(this.#version);
+ get(derived(() => {
+ get(this.#version);
+ return this.#sources.has(key);
+ }));
return false;
}
get(s);
return true;
} Plus, ReactiveURLSearchParams isn't fine-grained at all. |
Describe the problem
There's a TODO in the codebase for making reactive maps and sets:
svelte/packages/svelte/src/internal/client/proxy/proxy.js
Lines 44 to 54 in 434a587
We could implement it, but there are several problems:
Date
, and possiblyWeakSet
andWeakMap
) — including future additions to the languageDescribe the proposed solution
Instead, we could just create our own wrappers:
Importance
nice to have
The text was updated successfully, but these errors were encountered: