realm-js
realm-js copied to clipboard
realm
Incorrect wrapping of proxy objects on every render
How frequently does the bug occur?
All the time
Description
https://github.com/realm/realm-js/blob/master/packages/realm-react/src/useObject.tsx#L78 @takameyer
// Wrap object in a proxy to update the reference on rerender ( should only rerender when something has changed )
return new Proxy(object, {});
The intention of this code is that the reference will only change if the object has changed to play nicely with React's immutability conventions. However, the hook is not invoked only when the hook fires. It is invoked anytime the parent component renders. The component could be rendering because of a totally unrelated change to some other component state which will cause unnecessary Proxy wrapping, unnecessarily new references, break React's immutability conventions and contradicts the intention of this code.
Easier to explain with code
const initialObject = {};
function useObject() {
return new Proxy(initialObject, {});
}
export default function App() {
const [count, setCount] = useState(0);
const object = useObject();
useEffect(() => {
// This effect will be invoked everytime the component
// renders (eg click the button), even though the object
// never actually changed
console.log("object changed", object);
}, [object]);
return (
<div className="App">
<h1>{count}</h1>
<button onClick={() => setCount((x) => x + 1)}>Click me</button>
</div>
);
}
https://codesandbox.io/s/modern-field-7ety98?file=/src/App.js:69-642
The fix here is to create the new proxy in the updateCallback.
I think a further improvement would be to have a singleton observable object with a single realm listener with a reference count referring to the number of active hooks using it. This would:
- Reduce the overhead of fetching a new object each time because if the object is already in memory you can reuse it
- Improve resilience by reducing potential gotchas with lots of realm listeners such as #4375
- Using the same object via
useObjectin two different components will return the same reference and so works best with React's immutability tendencies.
Side issue: while the object cache and collection will make great strides to improving immutability, non-primitive values such as Dates, sets and dictionaries will not be immutably cached correctly. Are you aware of that?
Stacktrace & log output
No response
Can you reproduce the bug?
Yes, always
Reproduction Steps
No response
Version
main
What SDK flavour are you using?
Local Database only
Are you using encryption?
No, not using encryption
Platform OS and version(s)
ios
Build environment
Which debugger for React Native: ..
Cocoapods version
No response
@mfbx9da4 Good catch. Really appreciate the feedback. We will investigate this as time allows and get back to you when we have something in review.
@mfbx9da4 We have a good idea for fixing the proxy wrapper. That being said, can you please make a separate issue for:
Side issue: while the object cache and collection will make great strides to improving immutability, non-primitive values such as Dates, sets and dictionaries will not be immutably cached correctly. Are you aware of that?
We would also appreciate an example where this is not being handled correctly 🙂 Thanks so much for your feedback!
@takameyer what do you make of my singleton object proposal? Is there somewhere better to discuss that?
@mfbx9da4 I think it's worth looking into. We have discussed various ways we could improve or notification systems. I think making a feature request would be a good way to invoke more discussion.
Is there any progress on this? In my app, I do a CPU-intensive operation using the data stored in the database. I tried the following:
const result = useMemo(() => {
console.log('starting CPU intensive operation');
return intensiveOperation(object);
}, [object]);
but it seems like this if fired every time the component state changes, not when the realm "object" dependency changes.
Is it what's described in this ticket?
It looks like the same thing that is described @maxencehenneron, yes. There's a PR https://github.com/realm/realm-js/pull/4519 with a fix for this issue which we are reviewing right now.
Hi, I noticed that useQuery has the same problem.
Is there a previous version where this works correctly? The README file caught my attention:
useQuery
Returns Realm.Results from a given type. This Hook will update on any changes to any Object in the Collection and return an empty array if the Collection is empty. The result of this can be consumed directly by the data argument of any React Native VirtualizedList or FlatList. If the component used for the list's renderItem prop is wrapped with React.Memo, then only the modified object will re-render.
const Component = () => {
// ObjectClass is a class extending Realm.Object, which should have been provided in the Realm Config.
// It is also possible to use the model's name as a string ( ex. "Object" ) if you are not using class based models.
const collection = useQuery(ObjectClass);
// The methods `sorted` and `filtered` should be wrapped in a useMemo.
const sortedCollection = useMemo(() => collection.sorted(), [collection]);
return (
<FlatList data={sortedCollection} renderItem={({ item }) => <Object item={item}/>
)
}
That is not working as expected, all items in my list are being re-rendered.
As a workaround, I've implemented a caching hook on top of the ones provided by @realm/react:
export const createRefCache = object => {
return new Proxy(
{},
{
get: function (target, property) {
return object[property];
},
set: function (target, property, value) {
object[property] = value;
},
},
);
};
export const useCachedObject = (object, otherDeps = []) => {
const [counter, setCounter] = useState(0);
const forceRender = () => setCounter(n => n + 1);
// eslint-disable-next-line react-hooks/exhaustive-deps
const memoObject = useMemo(() => createRefCache(object), [counter, ...otherDeps]);
useEffect(() => {
const listener = (obj, changes) => {
if (changes.deletions.length || changes.insertions.length || changes.modifications.length) {
forceRender();
}
};
memoObject.addListener(listener);
return () => {
memoObject.removeListener(listener);
};
}, [memoObject]);
return memoObject;
};
Let me hear what you think about it!
@takameyer
I'm not sure what I'm missing, but I clearly still observe the issue described in #4913 for both useQuery and useObject.
Android 10
"react-native": "^0.70.5",
"realm": "^11.2.0",
"@realm/react": "^0.4.1"
