atlassian / react-sweet-state

Shared state management solution for React
https://atlassian.github.io/react-sweet-state/
MIT License
871 stars 55 forks source link

Discussion: Should an error in onUpdate trigger the container's error boundary? #220

Open obweger opened 10 months ago

obweger commented 10 months ago

Team,

just something that hit me by surprise the other day: If an error is thrown in an RSS container's onInit, the error appears to be considered a rendering error and hence bubbles up to and triggers the next error boundary. However, if this happens in onUpdate, the error doesn't appear to be considered a rendering error and hence doesn't end up triggering the error boundary.

I've now worked around this by catching the error myself in onUpdate, calling an onUpdateError handler in the container's props, and triggering the error boundary myself - but my gut feeling is that this should happen automatically.

Just food for thought and discussion - grazie!

albertogasparin commented 10 months ago

Good observation, didn't realise that. The difference is indeed that onInit is called as part of a hook initialisation while onUpdate is called by the subscription, so as part of the schedule/batching process. I'm not sure however that onInit behaviour is what we should normalise. The reason is that being able to catch it via error boundary is possible only for sync actions. Async actions will always need to be manually caught.

So my worry is that devs might not realise they need an error boundary in the first place. I feel having an out of render error is slightly more annoying to catch but less fatal. And if you use async actions you need to manually handle it regardless. But open to hear your counter arguments :)

theKashey commented 10 months ago

Errors in general should not happen, but if they do - I think it's important to break things at the right place.

Personally - I prefer when things breaks and I "see" it, not then they break silently and I have to idea why UI became glitchy

obweger commented 10 months ago

Agree with @theKashey here - my philosophy is also that if something breaks, it should break fully.

For async actions @albertogasparin, I personally think it's fine for these to not trigger the error boundary; after all, I can also trigger any async stuff in a render function and it would also not trigger the error boundary.

My 2c.