Bug: Dev Mode breaks applications: calling getters in props objects causes side-effects

[yGuy]

In dev mode, there is a feature that inspects the properties of all props passed to a component, recursively. This happens using a simple for (key in obj) loop and thus traverses all enumerable properties, recursively, including getters. This feature cannot be disabled without disabling dev mode and it will only trigger in slightly advanced setups and thus is hard to debug. Iterating all enumerable keys for getters in an object is a problem, because these getters are also evaluated using simple unconditional obj[key] syntax, causing possible side-effects to happen for non-trivial getters. Although frowned-upon in many cases, it is valid for getters to have side-effects that could change the state of the objects or throw exceptions. Specifically in objects that are unrelated to the React components. However, in dev mode, this causes the application to break or to behave differently in non-dev mode with hard to diagnose bugs.

React version: 19.2.0

(this is a regression)

Steps To Reproduce

1. Run the following demo in dev mode

import {useEffect, useState} from 'react'
import {createRoot} from "react-dom/client";

function getData() {
    const result = {}
    result._someInnocentUnusedProp = {}
    Object.defineProperty(result._someInnocentUnusedProp, "nestedUnusedCalculatedProp", {
        get: () => {
            console.log('get nestedUnusedCalculatedProp called - why?')
            throw new Error("I should not call getters unconditionally")
        }, enumerable: true
    })
    return result._someInnocentUnusedProp
}

function DataRenderer() { return null }

function Child(props) {
    return <div>child: {props.count}<DataRenderer data={getData()}></DataRenderer></div>;
}

function App() {
    const [count, setCount] = useState(0);

    // just to cause a re-render
    useEffect(() => {
        if (count > 5) return
        setCount((c) => c + 1);
    }, [count]);

    return <Child count={count}/>;
}

createRoot(document.getElementById("root")).render(<App/>);

2. You will get a stacktrace like the following:

 Uncaught Error: I should not call getters unconditionally
    at Object.get [as nestedUnusedCalculatedProp] (main.jsx:10:19)
    at addObjectDiffToProperties (react-dom-client.development.js:3968:21)
    at addObjectDiffToProperties (react-dom-client.development.js:4016:23)
    at logComponentRender (react-dom-client.development.js:4130:22)
    at commitPassiveMountOnFiber (react-dom-client.development.js:15469:13)
    at recursivelyTraversePassiveMountEffects (react-dom-client.development.js:15439:11)
    at commitPassiveMountOnFiber (react-dom-client.development.js:15718:11)
    at recursivelyTraversePassiveMountEffects (react-dom-client.development.js:15439:11)
    at commitPassiveMountOnFiber (react-dom-client.development.js:15476:11)
    at recursivelyTraversePassiveMountEffects (react-dom-client.development.js:15439:11)

Link to code example:

Code Sandbox

(Sometimes you need to reload the page a couple of times for the issue to appear. The timing is not deterministic, which makes the issue especially hard to diagnose.)

The current behavior

React Dev mode evaluates getters recursively from time to time, possibly causing side effects.

The expected behavior

Rect dev mode should not evaluate all getters in props objects unconditionally.

My thoughts

The problematic code is here, which evaluates all enumerable properties, even from prototypes:

https://github.com/facebook/react/blob/3a495ae72264c46b4a4355904c6b4958b0a2f9b2/packages/shared/ReactPerformanceTrackProperties.js#L295

The workaround here (other than not using dev mode) is to make the properties non-enumerable, however that is not always feasible.

My suggested fix would be to not evaluate getters for the logging purposes. They are likely to return new objects, perform a lot of work and thus slow down the execution, may change the state of the application, or may even throw. Only ever compare fields to avoid changing the state or executing code at all costs.

Using Object.keys() would improve the situation for properties defined on classes, but ideally "getter"s should not be called at all, no matter whether they are defined as own properties or prototype properties.

The current state requires complicated workarounds and causes hard to debug issues in complex applications. As it only affects dev-mode, I would love to see this improved in a bugfix so that people can upgrade to 19.2

[yGuy]

Thanks @Amitjoiya for the PR - indeed, this looks like it should solve the issue without reducing the idea of the usefulness of the original feature. LGTM!

Let's hope someone will see it and at least set the status to confirmed.

[jraoult]

Thanks, @yGuy, for investigating that. I'm glad I found this issue because I couldn't determine the root cause. We use MobX with the computedRequiresReaction option. Calling getters ("observable" or "computed" in MobX terms) outside an "observed" context is logged as a warning. In our case, it can reach up to 2000+ warning messages, which freezes the dev tools when the console is open. The workaround in dev mode is to disable MobX's configuration, but it sadly defeats the whole purpose. I doubt many people use this, but I leave it here for reference.

I'm not sure about the details, but MobX uses proxies internally, so I don't think the fix in the linked PR would address the issue.

I also found this PR: https://github.com/facebook/react/pull/34742, which seems to reduce the volume of log traces in my case, potentially, but I'm not sure it would help in yours.

[yGuy]

@jraoult - the PR would only make it worse, I guess. At least in the sense that the problem will be even harder to reproduce. In my case, the logging wasn't actually the problematic thing. It never logged anything. The problem is that for the purpose of potentially logging some diagnostics, completely unrelated user-code gets executed and in the case of side-effects can break your application in all kinds of ways.

The person who implemented this code in that way surely assumed that every object that is passed to React is a pure JSON-like object without behavior - no Proxies, no getters, no objects, no classes, just plain data. However, that assumption is clearly too optimistic. I guess we should flag this as a security issue because you can inject arbitrary code that will be executed by the dev tools ;-) this way. That might get more attention than this.

[eps1lon]

This is a performance critical path so it needs to be fine tuned for performance. We're already omitting props in Server Components if they have an underscore. Would that help this case if we'd skip those when diffing as well?

Diffing is really just best effort so it seems fine to ignore underscore props which are mostly considered private.

[yGuy]

@eps1lon thanks for chiming in.

Sorry, but no, the underscore approach would not help. As this is often about objects that developers may not have control over. Third party library objects that just happen to be part of the "props" object, but which are never used in the process of rendering.

Also: Is this really performance critical?

The code in question is not improving performance: the whole point of the code is to spend extra time for logging purposes! It's about detecting possible issues that might then be loggged. It's not trying to be performant or improve performance at all, but it is supposed to help DX at dev mode time. And that part does not even happen deterministically - so much for DX. It enumerates possibly thousands of props of objects recursively that are completely unrelated to a potential problem. If anything, the current state slows down the process by even enumerating functions in a prototype chain and checking whether they have changed! And if the code was caring about performance, it should not be calling code (which it has not control over and no business with) at all. Computed properties/getters often perform more work than just returning a value. The logging code executes this code, which is definitely way slower than checking whether this is a getter and then skipping it, which would be the safer and quicker option.

[jraoult]

@eps1lon, if this is about a warning for a missing memo opportunity, would there be a way to disable this "hint" altogether? I don't need it, and the costs are not worth the benefit in my case.

It won't be easy to find a solution that works for everyone here. One solution for my case is to make properties non-enumerable on MobX objects, but that is not realistic since I need enumerability in some scenarios.

[eps1lon]

It's not trying to be performant or improve performance at all, but it is supposed to help DX at dev mode time.

The logic needs to be performant though.

[yGuy]

It's not trying to be performant or improve performance at all, but it is supposed to help DX at dev mode time.

The logic needs to be performant though.

Are you saying that the proposed PR has a performance problem? Because I don't really see it. In fact, Object.keys() or Object.entries or the like should be faster. In my case, it will massively improve performance, as it will do for @jraoult, because it does not enumerate elements it should not be enumerating anymore.

And the current code around Line 405 (see below), looks like it would be very expensive for the case where there are functions in a prototype (quite likely), so not performing that check on the prototype chain (that rarely ever changes in the real world) will improve the performance if implemented properly, too:

https://github.com/facebook/react/blob/3a495ae72264c46b4a4355904c6b4958b0a2f9b2/packages/shared/ReactPerformanceTrackProperties.js#L405

But I don't like where the discussion is going, to be honest. IMHO this is unwanted behavior that is very hard to workaround - a.k.a a bug. And it only happens at dev mode time where performance should take second seat (I mean: components are unmounted and mounted twice for the sake of a better DX, already). Even if the performance would degrade (I would humbly estimate below 1% for the majority of cases and a performance improvements by orders of magnitude in other cases, when the getter calculates values), I think fixing the problem would be more important than slightly changing the performance characteristics of this piece of code.

[yGuy]

Bumping the "discussion", here.

So, the recommended way of dealing with this feature is to change the way objects are created that are passed as props?

As far as I can tell, this new requirement was never there since React 1.0 and is not documented, anywhere.

If you believe that this is good default behavior and do not consider this a bug, the documentation should at least be updated to help people adjust and prepare their code bases accordingly - maybe add a linter warning that warns you when you pass objects as props that contain more properties than required:

Something along the lines of this:

Be careful what is passed as "props": Props may not contain objects that start with an underscore and if your props happen to contain references to other objects (even if they are not used in the regular React rendering and processing), make sure that these are added explicitly as non-enumerable properties. This also holds true for objects in props that use prototypal inheritance, and recursively nested properties - used or unused. In various situations, React Dev mode will perform checks on all enumerable properties in every object and their protoypes and invoke every getter property to warn you when there are too many. It will also check whether the getter implementations are pure and always evaluate to the same values. Be sure that you don't have any Proxies, or getters in any of your props, recursively, even when not used during render, as this may cause all kinds of performance or behavior changes in your application, if there are any side effects in any of these implementations.

[jraoult]

FWIW, I still think this diff is disruptive enough that, if the output doesn't add value to you (like if you are not using that instrumentation), it should be possible to disable it entirely.

[scottlet]

+1 - this breaks eg mobx-state-tree in interesting and unexpected ways that can't be worked around in dev mode without a huge refactor.

[mobx-state-tree] You are trying to read or write to an object that is no longer part of a state tree
[mobx-state-tree] assertion failed: the creation of the observable instance must be done on the initializing phase

MST nodes have strict lifecycle rules - once detached from the tree, they throw errors when accessed. React 19.2's new dev-mode object inspection (addObjectDiffToProperties in logComponentRender) traverses all object properties for debugging purposes, which triggers these MST assertions.

Would be great if there was a way to at least turn off this functionality, but not accessing getters and setters on object properties would be preferred. It is hitting getters for proxy objects inside of MST and outside the flow of the application, causing them to be created, breaking and then throwing. Renders the entire UI unusable in development mode.

[coolsoftwaretyler]

Hey folks, yeah we had a bug reported about this in MobX-State-Tree a while ago: https://github.com/mobxjs/mobx-state-tree/issues/2277, but I thought it was isolated to a particular code path that users could opt out of with a different liveliness checking option. Essentially - I had hoped there was a way for MST users to change their MST settings to coexist with React 19.2+.

However, I just got @scottlet's issue, and it seems the problem is more widespread.

I haven't had time to explore a full solution yet, but my initial suggestion is: can React add some kind of setting that would turn off this behavior? As I understand it, this is a debugging tool, and I'd love it if users could make the tradeoff decision and get worse debugging but functioning code.