Circular references in Stores trigger loss of proxying if non-trackable objects are referenced

[QWu4xYV]

Update

This issue originally suggested that circular references in stores triggered loss of proxying, but after further investigation by @maciek50322 , it turns out that the issue is when mixing non-trackable objects with trackable ones (as in the playground examples below). I think this is still a bug, as we may want to store Stores inside of plain objects with circular references.

Original issue:

Describe the bug

(From Discord support channel here with more investigation details. Was told to file here as a bug)

Solid Playground:

• createMutable: https://playground.solidjs.com/anonymous/8f3ae147-b722-4b67-8592-044670933b61
• createStore: https://playground.solidjs.com/anonymous/8f07d9c2-54da-43e9-8e08-88393f446bca

Notice in the playground that we can add as many dogs who don't know their trainers, but only one dog which knows its trainer (which has a circular reference on the dogs they are training)

As someone new to Solid, my understanding of the investigation from Discord is that when adding an object to a list in a store which creates a circular reference, the proxy object seems to be lost in unwrap, leading the list to update with the circular reference the first time, but then never be updatable afterwards.

Your Example Website or App

https://playground.solidjs.com/anonymous/8f3ae147-b722-4b67-8592-044670933b61

Steps to Reproduce the Bug or Issue

1. Click on "Add Dog who does not know their trainer" as much as you want
2. Observe it update successfully
3. Click on "Add Dog who knows their trainer" as much as you want
4. Observe it only update the first time

Expected behavior

As a user I expected to be able to add as many circular references as I want.

Screenshots or Videos

No response

Platform

• OS: macO
• Browser: Chrome
• Version: 120.0.6099.216

Additional context

No response

[takeramagan]

in proxyTraps,
set(target, property, value) {
batch(() => setProperty(target, property, unwrap(value)));
return true;
},

if I remove the unwrap here, the bug is fixed.
Will there be any other side effects if done this way @ryansolid

[fabiospampinato]

You don't want to remove that unwrap because you don't want to directly set proxy instances on the proxied object directly, if you can easily avoid it, the problem is how the unwrap function itself works.

[maciek50322]

This happens because changing person1.dogs, causes changed properties to unwrap, so when pushing new dog with trainer: person1 , the dogs[x].trainer is being unwrapped. If unwrapper meets proxy, it just takes its original value. Because dogs[x].trainer isn't proxy, unwrapping goes deeper, to dogs[x].trainer.dogs. Now this is a proxy, so unwrapper reassigns unwrapped dogs[x].trainer.dogs - and because of circular reference, this is same as reassigning unwrapped person1.dogs, thus overriding proxy with unwrapped proxy - no more tracking.

This wouldn't happen if dogs[x].trainer was a proxy (unwrapping wouldn't go deeper), so actually if your person1 was a proxy, so instead of:

const makePerson = (name: string) => {
  const dogs = createMutable<Array<Dog>>([]);
  return {
    name,
    dogs
  };
};

this would work:

const makePerson = (name: string) => {
  return createMutable<Person>({
    name,
    dogs: []
  });
};

But also I think your code could work if unwrapping skipped original proxy, so if proxy trap was something like this:

set(target, property, value, receiver) {
  batch(() => setProperty(target, property, unwrap(value, new Set([receiver]))));
  return true;
}

(this should affect only circularly referenced proxies)

[maciek50322]

So actually I checked, and my suggestion to change proxy trap wouldn't work.
To make it work the proxy would need to keep track of top-level mutable to exclude it from unwrapping.
I am not sure if this would be intended, because then for the "corrected" example:

const makePerson = (name: string) => {
  return createMutable<Person>({
    name,
    dogs: []
  });
};

when adding new dog with owner, the proxy would be assigned to original value and then unwrapping wouldn't handle proxy within proxy.

So my suggestion is to avoid controlling mutable only from within non-trackable object as it can become non-trackable itself.
That means it's best if you have direct access to mutable or from other e.g. mutable

const mutable = createMutable({}); 
const onClick = () => mutable.prop = ... // this is fine

const mutableWrapper = { mutable: createMutable({}) };
const onClick = () => mutableWrapper.mutable.prop = ... // not using mutable directly, avoid this

const mutableWrapper = createMutable({ mutable: createMutable({}) }) 
const onClick = () => mutableWrapper.mutable.prop = ... // this is also fine

[QWu4xYV]

@maciek50322 , thanks so much for digging into this! So the problem isn't with circular references, it's with circular references outside of stores...

(I modified my original playground from discord that used Store instead of Mutable, https://playground.solidjs.com/anonymous/8f07d9c2-54da-43e9-8e08-88393f446bca, to make everything a Store and that works now, which seems to confirm it: https://playground.solidjs.com/anonymous/4d5e0561-8504-4093-be33-b80a08ab02f0)

While this is fine as a workaround, I'm assuming this would still be considered a bug, as I may want to use plain objects instead of stores in some cases. I'll change the issue description accordingly.