leak between createStore and createMutable of object references and proxy objects

LiQuidProQuo commented 1 year ago

Describe the bug

createStore and createMutable proxy handlers are exclusively bound to an object reference.

let raw = {count:5}
let [s, setS] = createStore(raw)
let m = createMutable(raw)

both s and m share the same raw object, however, since createStore was called 1st the raw object is associated to the immutable proxy. and m is also immutable, as it returns the same proxy initially associated by createStore.

why? because in solid the reactive proxies are singletons, looked up by the object reference

if we reverse the order

let raw = {count:5}
let m = createMutable(raw)
let [s, setS] = createStore(raw)

s is now a mutable proxy.

can this be fixed? probably (not easily or without some more overhead) but at least should be documented, that createStore and createMutable should not use the same objects. as it may break expectations when working with one or the other.

Your Example Website or App

https://playground.solidjs.com/anonymous/7e274c6d-5d46-4af4-9552-1eecd17333ea

Steps to Reproduce the Bug or Issue

see

Expected behavior

above

Screenshots or Videos

No response

Platform

play

Additional context

No response

ryansolid commented 1 year ago

This is related.. sort of a duplicate of #1300. I don't see a solution to this really. Even with a Weakmap we'd be doing a lookup against some shared location when tracking reactivity. So it'd be the same problem. I suppose you could do a separate lookup for each but then they'd get out of sync. Maybe just trying to do this should throw? Even that is complicated a bit.

The problem is that because change tracking is based on mutation these have to be the same thing. And it's not as simple as deep cloning because you'd need to deep clone every set operation to be truly isolated. I don't know how many ways to say this but treating stores as they were plain objects is not going to result in good things.

LiQuidProQuo commented 1 year ago

while it is related to #1300, due to it stemming from the same implementation detail. it concerns different expectations.

createStore for creating a derived store is a powerful feature. and the derived store, does not break the immutability contract. and is not the concern of this issue

with createStore and createMutable there is a race condition that may break the following expectations.

I pass an object to createStore, I expected the returned store to be immutable
I pass an object to createMutable, I expect the returned store to be mutable that expectations breaks, when the "source" object may get accessed by different store.

as a side note

currently we can mix mutable stores in immutable stores. mix mutable and immutable stores example https://playground.solidjs.com/anonymous/b98aafff-7c7a-431b-8f93-81e5ea175760

if we simply isolate the "$PROXY" symbol with $PROXYMutable for createMutable and $PROXY for createStore then we can sort of fix the case, of the "shared object", but we lose the ability to create mixed store as shown in the mix mutable and immutable stores example above

patching createMutable wrap() function with an isolated symbol $PROXYMutable creates an isolation were a mutable store nested access, always produces the same derived type of access/proxy (That, will break the current ability to mix mutable and immutable in the same store)

patch

in this example, it shows that isolating the proxy, maintains reactivity for the object shared by 2 different type of store wrappers. and also respects the mutability contract of each.

I don't know how many ways to say this but treating stores as they were plain objects is not going to result in good things.

let me try another way.

a "store upgraded" object should be considered as an object override
once an object is "upgraded" to a store, it is no longer recommended to retain reference to the original object.
if a reference is retained, and reused by another store, ensure that it is exclusively accessed via the same type of store to avoid the race conditions problems associated with this issue:(#1396 this issue).

there is a good use case for being able to create a view only from a mutable store. but it looks like users either have to choose one or the other or come up with alternative solutions.

ryansolid commented 1 year ago

Interesting. I see. Still using the same underlying Signals this way. Hmm.. we also use $PROXY for detection. So it isn't as simple as changing the symbol. But it does sound like their could be a solution in here somewhere.

solidjs / solid