-
Notifications
You must be signed in to change notification settings - Fork 638
Fix React 19 observer pattern warnings (issue #2277) #2278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
It does look like the issue happened between React 19.1.0 and React 19.2.0. Continuing to investigate what changed. |
|
Maybe we should make this a configurable property on each node. Keep the default level of checks, and allow MST users to reduce the strictness if they know it's safe in their use case. |
|
It's also possible this behavior is already configurable? We have some docs around getting the current behavior: https://mobx-state-tree.js.org/API/#livelinessmode But I can't find how to change it at runtime. I will investigate later today. |
|
Ah, we have a function to do that globally: https://mobx-state-tree.js.org//API/#setlivelinesschecking. I wonder if it makes sense to have this available on a per-model instance, but at the very least we have an escape hatch for newer React versions. |
|
I remember Gadget hit this a lot, because there were a lot of
per-model instance seems like it could lots of bookkeeping. Not sure of the perf / memory impact of that for large applications. I'm sure it'd be fine, but hard to assess! What do you think about a callback function being another option, in addition to the warn/error/ignore, perhaps in place of? It would receive the instance, which can let the caller make a decision on how to respond? That could allow the per-model instance checking, but also other options, like "in production, don't print to console, but send a report to my error tracking service". |
Ooh yeah, I like that, kind of like https://mobx-state-tree.js.org/API/interfaces/referenceoptionsoninvalidated#oninvalidated, maybe? That said, liveliness checking warning is a good signal to developers to let them know they've done something weird (reading or writing from a node that is no longer alive). I like this default behavior. The downside is just that it's noisy in this particular use case (using an MST node as a prop in React 19.2+). The only option right now is to globally disable it. It'd be nice to opt out for certain types/specific nodes and preserve the default elsewhere. I could see a use case where someone says "Yeah, all of my |
|
Yep, I'm game for keeping the default behaviour. I could perhaps propose a tweak: one new mode that warns in development and ignores in production, and make that the default. Okay just keeping it as-is and guiding users towards how to disable it. Perhaps (maybe just in development) append a bit to the message to point them to |
What does this PR do and why?
Related to #2277.
I'm not convinced this is what we should do, because it really changes some of the strictness about reading from dead nodes. But it does seem to fix the problem in reproducers.
React 19 is doing something that triggers MobX's _interceptReads on MST node properties, which calls assertAlive() for every property access. This creates warnings for alive nodes being read during rendering, although they seem to be false positives (the observability is working as expected).
One way we could fix this is to:
This would allow passive property reads on alive nodes without warnings, and maintain existing error detection for genuinely dead nodes. It'll also preserve write protection during actions.
I added React 19 compatibility test suite that sort of mimics the behavior here. Other tests are passing.
Steps to validate locally
We need to consider if this is what we really want to do, so I'd love some feedback on what could go wrong if we relax the liveliness checks.