oby: Infinite recursion with arrays

@fabiospampinato

I’ve been running into a lot of difficult-to-debug issues around infinite loops involving arrays and circular references. Here’s a sandbox with a sample of what I’m talking about:

https://codesandbox.io/s/voby-demo-store-counter-forked-r225g4?file=/src/index.tsx (click the + a couple times and the app will hang)

This has been a bit difficult in my app because Solid handles these circular references in arrays just fine and I have data of a similar shape cropping up in numerous places in my app. So this looks to me like a bug, but what do you think? Are these cases something that voby/oby should handle? Or does there just need to be a big warning sign in the readme?

About this issue

  • Original URL
  • State: closed
  • Created 9 months ago
  • Comments: 19 (19 by maintainers)

Most upvoted comments

I am currently still using it. The batching/tick behavior is pretty essential to the current iteration of my app. Though I’ve recently had some insights that might let me take over the tick behavior that would let me use alternate renderers. But I’m not in a rush to switch.

If I throw inside the effect I see the error in the console. Maybe you had an error boundary or something?

Screen Shot 2023-10-06 at 13 18 41

Re something ought to be done

What you’ve outlined seems pretty reasonable. I think a big warning in the readme/docs would be very handy. This issue has cost me a lot of time because the nature of the bug is so weird. It’s seemingly recursive but it never blows the stack. And I think there might be something in the oby code swallowing errors because when I was playing around with it locally, I threw errors from the oby code when the it went through too many iterations of get from the store and the recursion would stop but I’d never see the errors in the console. So that might also be something to hunt down now that there is a smallish repro. I may have had something else in the app catching those errors. I’m not sure.

Re migration from Solid: maybe that’s something for a v1 release

I totally get that. Solid’s stores have bitten me repeatedly. Especially as they’ve changed several times.

Re performance: there’s a WeakMap to cache proxies

That makes sense.

Regarding the 1 level unwrapping

I was only thinking about this for when an array gets set on a store field. Any other unwrapping would be constant-time. But I agree that it is surprising and could lead users to think that the unwrapping is going to be deep when it isn’t. At least with no unwrapping of array items (status quo), it’s very clear that there’s no unwrapping beyond what you see.

Thanks for all your time on this. Oby’s batching is chef’s kiss by the way. batch and tick are a great combo.

Regarding the 1 level unwrapping: depending on how you look at it it feels like a worst of both worlds, or a best of both worlds, compromise. From my point of view unproxying at 1 level is both potentially way slower than the constant time unproxying, and also doesn’t solve the problem in general. From another point of view I guess it could make more code work out of the box with no user-level changes. I don’t really like it as a solution.

I sort of expect everything that works on a plain object to also work on a proxy. If that’s not the case, I think there should be warning signs.

I guess that’s a little surprising, and there should be warning signs, but I guess they should be on MDN or something? Like a proxied object is not triple-equal to the unproxied object. structuredClone throws for proxies. postMessage also throws. It’s not Oby’s place to document this stuff probably, though some warnings can be added to the readme if they are useful of course.

I thought it already unproxied set values, just not with array items? Have I got that wrong?

Yes it already unproxies set values, but not deeply. In your code the proxy is found “deep” inside the array, so that doesn’t get unproxied.

That was my first thought, But then I’m not sure how that would work. You’d have to modify or make copies of arrays when doing an unwrap (if it has proxy items) and that seems really surprising. But it seems like you’d have that same issue with any deep unwrap, which seems like a problem.

Yeah that’s another problem with deep unwrapping, especially if done by default. You deep unwrap twice and the two objects don’t match one another. That’s how Solid works by the way.

manually unwrapping stuff before setting it

This is simplest but means losing reactivity when reaching through a nested array, which seems less than ideal.

Losing reactivity how? This option should JustWork™️, it “just” requires some understanding of this problem and manual error-prone code that unwraps values being set.

I’m not sure. It’s a tricky problem. My understanding is that if array methods were proxied, then unwrapping could still be constant time without losing the nested reactivity. Is that correct?

I’m not sure what you mean, it seems orthogonal to the problem at hand 🤔 array methods are already proxied.