Cyclic data structures currently not supported in an existing store about store HOT 9 CLOSED

This sounds like a problem I would have stumbled myself on very soon, thanks for reporting it, I'll get it fixed probably within a couple of days.

If you'd like to help find the issue I think this is most probably an issue with proxy-watcher, but I don't quite understand how that happens right now.

from store.

There might be a fundamental problem with repeated structures, if foo.deep.bar and qux.deep.bar are the same thing, but you only edit foo.deep.bar[0], how do we know that qux.deep.bar has been edited as well? I think this could be tricky to detect, perhaps impossible under some scenarios.

from store.

I think I've just stumbled on this myself, and I've been thinking more about it. I think there are basically 2 options here:

1. Throw an error when repeated structures are encountered rather than keep the process spinning forever.

   • This would be a bit of a bummer, however it feels a bit like dealing with a non-normalized database where data is duplicated all over the place, maybe instead of copying/referencing over a substructure it'd be cleaner (and probably less error-prone once set-up properly) to instead store an ID that can be used to retrieve the original structure.

2. Support repeated structures seamlessly.

   • This would appear to be the obvious best choice intuitively, and I think it should be doable, however in order to do that we would have to deeply proxy the passed object immediately, otherwise we might not know if/where any substructures are repeated, and this may have a considerable performance cost.

@florianf which option would you like? 🤔 I'd personally be in favor of the second option if the performance impact won't be significant, which I'm afraid it'll probably be.

from store.

Hi,
I tried to dig into the source, but I'm not fluent in TypeScript and JS Proxies, so my understanding of the issue is only superficial.

Regarding your example foo.deep.bar === qux.deep.bar, so assuming they are referencing the same object, shouldn't those properties only be wrapped once in a Proxy? Or is the problem, that nested objects are "lazily" proxied, when first accessed or changed?

Option 2. would be clearly the "correct" one, but if it has a major performance impact it should at least be possible to opt-out of this behaviour. Maybe this can be detected automatically?

from store.

I took a quick look at the freezer.js immutable lib and it seems it traverses the complete tree and marks every node as "frozen":

https://github.com/arqex/freezer/blob/master/src/frozen.js#L26

This appears to be the only solution to me. I don't know how expensive the proxy creation is, maybe proxies could only be created for nodes that have more than one reference?

from store.

Regarding your example foo.deep.bar === qux.deep.bar, so assuming they are referencing the same object, shouldn't those properties only be wrapped once in a Proxy? Or is the problem, that nested objects are "lazily" proxied, when first accessed or changed?

In my example those would be the same object, and it would be wrapped only once (lazily), the issue is: the library needs to know which paths changed, so that it can efficiently check if your components should re-render, now let's say that you edit the object via path A, but then you retrieve the object via path B in your selector, the library would see that none of the changed paths have been accessed and it won't re-render your component, this would be a bug. In order to avoid this bug the library would have to know all possible paths that could be used to access an object, that means it needs to discover them as soon as possible and so it can't proxy things lazily, which I think would be an important issue for performance.

but if it has a major performance impact it should at least be possible to opt-out of this behaviour. Maybe this can be detected automatically?

I guess the library during development could detect this and throw an error, or it could optionally support this use case in an opt-in fashion. I haven't actually measured yet what the performance implications of this would be, may they are not that bad.

This appears to be the only solution to me.

If the object is deeply frozen then it can't be mutated, I think maybe that could be a different approach for a state management library, I don't think it could solve our problem in this library though.

from store.

now let's say that you edit the object via path A, but then you retrieve the object via path B in your selector, the library would see that none of the changed paths have been accessed and it won't re-render your component

O.k. now I understand. Btw. cool feature, that wasn't clear to me ;-)

If the object is deeply frozen then it can't be mutated, I think maybe that could be a different approach for a state management library, I don't think it could solve our problem in this library though.

That comment of mine was misleading, sorry.
What I meant: It seems it's necessary to traverse the whole object tree on creation (not lazily) to find duplicate references to objects. Then it should be possible to detect all those paths (f.ex. save the parent(s) in a hidden field to every node like freezer.js does) and to trigger re-renders for all matching selectors.

from store.

What I meant: It seems it's necessary to traverse the whole object tree on creation (not lazily) to find duplicate references to objects. Then it should be possible to detect all those paths (f.ex. save the parent(s) in a hidden field to every node like freezer.js does) and to trigger re-renders for all matching selectors.

Precisely.

Alternatively we could compare the things that could have been changed and accessed with each other for equality (if they are not primitives), but I don't think this would scale well either.

I'm not sure there are any other approaches, other than fundamentally changing how the library works.

Anyway I'll have a think about this, currently I'm thinking that pushing for non-duplicated nested structures would be a good thing, and it would be a shame if when adopting these kind of structures one would have to pay a performance cost for the discouraged case 🤔 On the other end though it would be a bummer if the library didn't really support nested duplicated structures, at the very least it should thrown an error rather than crashing the browser. Maybe I can add an option for this or something, we'll see.

from store.

I've finally addressed this issue in the latest version.

Basically now all the internal functions used for cloning and comparing objects work with circular structures too, however when a duplicate reference is found, i.e. when the library finds two different paths that point to the same object at the same time, an error is thrown, because this kind of structures can't be supported properly without sacrificing performance by eagerly deep proxying the entire object, and even that might not be enough to implement support for this this robustly.

from store.