iddan
Fix maximum update depth exceeded
Hi, I encountered an issue in my project similar to that described in this open issue link.
When using the code snippet below:
<Spreadsheet
data={data}
onChange={(newData: any) => {
console.log(newData);
setData(newData);
}}
/>
Every time I made a change in a cell, I noticed that console.log was being called infinitely.
Upon investigation, I found that the issue lies within the Spreadsheet.tsx file, specifically in the following useEffect:
React.useEffect(() => {
if (state.model.data !== prevDataRef.current) {
// Call onChange only if the data changes internally
if (state.model.data !== props.data) {
onChange(state.model.data);
}
}
prevDataRef.current = state.model.data;
}, [state.model.data, onChange, props.data]);
The problem arises from the comparisons made inside the useEffect's callback function and within the dependency array. Since we are comparing two non-primitive variables, these inequality comparisons always return true. Consequently, when the value of a cell changes, state.model.data varies, triggering the useEffect. However, the two if conditions always evaluate to true, leading to the invocation of onChange. As onChange sets new props, the useEffect is triggered again, resulting in an infinite loop.
To address this issue, I created a new function in the util.ts file that enables deep comparison of arrays. By using this function within the useEffect body, I ensure that the infinite loop is halted, and onChange is called only once.
Hello Iddan,
Thank you for taking the time to review my pull request.
I understand your concerns regarding the potential slowdown for very large datasets. However, in my humble opinion, some slowdowns are preferable than infinite rendering loop. The main problem is that, in the original code, when the two object references are compared (either implicitly in the array of dependencies of the useEffect hook or explicitly within the useEffect's callback body) an endless loop can occur after any change. This is because there is no guarantee the object references will remain the same between renderings.
To address both issues, I have introduced a new update that adds a 'lastUpdateDate' property to the state object. The concept is to keep track of the last time the data within state.model has changed. This allows us to compare the model inner statuses and execute the desired task as soon as the information in state.model.data varies. Furthermore, this modification works efficiently by replacing all the nested deep comparisons.
Thank you for your attention and consideration.
The fact that you are comparing 2 non-primitives in the useEffect and triggering re-renders at the top level like this means this whole package is dead on arrival. There should be a warning to users to know this is a massive bug before actually using this library.
This thing needs to be torn apart and re-architected from the ground up. If you want to make comparisons like that, it needs to be on lower-level components like the cells themselves (if possible) OR find a way to compare the equality of two primitives.
While @EdoardoGruppi's solution isn't perfect, it's far better than what's happening today.
