Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix maximum update depth exceeded #387

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix maximum update depth exceeded #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
66347d8
Add function to deeply compare arrays
Mar 28, 2024
fcf2a53
Fix bug (infinite render loop) caused by wrong dependence arrays
Mar 28, 2024
4bab637
Improve performance by using a new primitive variable to trigger events
Apr 15, 2024
845b5c4
Update test to handle date and milliseconds for the setCellData action
Apr 15, 2024
6432922
Remove redundant renderings
Apr 22, 2024
5a24c81
Fix rendering issue: table size
Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/Cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,20 @@ export const Cell: React.FC<Types.CellComponentProps> = ({
[setCellDimensions, select, dragging, point]
);

const modeRef = React.useRef<Types.Mode>();
React.useEffect(() => {
modeRef.current = mode;
}, [mode]);

React.useEffect(() => {
const root = rootRef.current;
if (selected && root) {
setCellDimensions(point, getOffsetRect(root));
}
if (root && active && mode === "view") {
if (root && active && modeRef.current === "view") {
root.focus();
}
}, [setCellDimensions, selected, active, mode, point, data]);
}, [setCellDimensions, selected, active, point, data]);

if (data && data.DataViewer) {
// @ts-ignore
Expand Down
19 changes: 9 additions & 10 deletions src/Spreadsheet.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,17 +233,17 @@ const Spreadsheet = <CellType extends Types.CellBase>(
}, [onActivate, onBlur, state.active]);

// Listen to data changes
const prevDataRef = React.useRef<Matrix.Matrix<CellType>>(state.model.data);
const currentModelDataRef = React.useRef<Matrix.Matrix<CellType>>(
state.model.data
);
React.useEffect(() => {
if (state.model.data !== prevDataRef.current) {
// Call on change only if the data change internal
if (state.model.data !== props.data) {
onChange(state.model.data);
}
}
currentModelDataRef.current = state.model.data;
}, [state.model.data]);

prevDataRef.current = state.model.data;
}, [state.model.data, onChange, props.data]);
React.useEffect(() => {
if (state.lastUpdateDate === null) return;
onChange(currentModelDataRef.current);
}, [state.lastUpdateDate, onChange]);

const prevEvaluatedDataRef = React.useRef<Matrix.Matrix<CellType>>(
state.model.evaluatedData
Expand All @@ -252,7 +252,6 @@ const Spreadsheet = <CellType extends Types.CellBase>(
if (state?.model?.evaluatedData !== prevEvaluatedDataRef?.current) {
onEvaluatedDataChange(state?.model?.evaluatedData);
}

prevEvaluatedDataRef.current = state.model.evaluatedData;
}, [state?.model?.evaluatedData, onEvaluatedDataChange]);

Expand Down
8 changes: 7 additions & 1 deletion src/reducer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,13 @@ describe("reducer", () => {
],
];
test.each(cases)("%s", (name, state, action, expected) => {
expect(reducer(state, action)).toEqual(expected);
if (name === "setCellData") {
// Addressing this case separately since the test may fail due to slight time-related
// differences between the generation of the expected and obtained results.
const result = reducer(state, action);
result.lastUpdateDate = null;
expect(result).toEqual(expected);
} else expect(reducer(state, action)).toEqual(expected);
});
});

Expand Down
5 changes: 5 additions & 0 deletions src/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const INITIAL_STATE: Types.StoreState = {
rowDimensions: {},
columnDimensions: {},
lastChanged: null,
lastUpdateDate: null,
hasPasted: false,
cut: false,
dragging: false,
Expand Down Expand Up @@ -132,6 +133,7 @@ export default function reducer(
...state,
model: updateCellValue(state.model, active, cellData),
lastChanged: active,
lastUpdateDate: new Date(),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Author

@EdoardoGruppi EdoardoGruppi May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Iddan,

First off, it's commonly advised against including objects directly in the dependency array of a useEffect. This is because objects are compared by reference, not by value. What this means is that even if the content of the object hasn't changed, if a new object is created on each render, the effect will still run on every render because the references are different.

So, although comparing object references directly might seem like a good idea, it's actually not recommended.

In the case of react-spreadsheet, the code below can lead to an infinite loop due to shallow comparisons made within the useEffect body and the dependency array.

<Spreadsheet
  data={data}
  onChange={(newData: any) => {
    console.log(newData);
    setData(newData);
  }}
/>
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]);

2024-05-1012-03-19-ezgif com-video-to-gif-converter

To illustrate this point further, you can try changing the second block to:

React.useEffect(() => {
  console.log("Use Effect called");
}, [state.model.data]);

You'll notice that even though the object might appear to be the same instance, its reference changes after every rendering. This causes the console log to be called every time there is a modification.

To summarize:

  1. state.model.data changes its reference every time setCellData is called.
  2. Since the reference changes, the useEffect body is executed.
  3. The first if condition is verified as the reference is always different from the one previously stored.
  4. The second if condition is verified because there is no guarantee props.data has a reference equal to the new state.model.data reference.
  5. The onChange function calls the setData function within the SpreadSheet object in App.tsx.
  6. The loop restarts without an end.

Solution

Adding a new lastUpdateDate property addresses this issue while avoiding resource-intensive deep nested comparisons.

With this addition, we track the last time setCellData is executed. If the data displayed in the UI has a different lastUpdateDate value, the useEffect is called, and the displayed information is updated accordingly.

Copy link

@thsorens thsorens Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

React.useEffect(() => {

}, [JSON.stringify(state.model.data)]);

Why not use just a string compare? For the huge datasets, it might be too slow though, but there is other libraries that can handle stringifying of an object more performant. Comparing a string, instead of having to traverse a huge object-graph is way easier.

The lastUpdate date check can probably cause some bad sideeffects as well.

Copy link
Owner

@iddan iddan Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @EdoardoGruppi for the detailed writing. I am unaware that any official React resources do not recommend object reference comparison. The real problem is the array reference changes which I don't see why it should happen more than once per mutation to the data.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EdoardoGruppi are you using the latest version of the code? The controlled storybook story works correctly and it uses the same code as you posted.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thsorens in the same spirit of React, I try to avoid unsimple comparisons on data.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @iddan,
thanks for your responses.

I confirm that I am using the latter version of the code. To be 100% sure, I re-did the test by cloning the repo from scratch, but nothing changed. You can find a sandbox at this link to check the results I am getting. To see the logs, you just need to open the console in the embedded CodeSandbox browser.

You're correct that React doesn't explicitly advise against using non-primitive values inside the dependency array. However, many developers recommend avoiding this practice because it can lead to mistakes and unintended re-renders.

I am confident that this is the exact problem with the react-spreadsheet library. In one of my initial commits, I used a function to deep compare objects, which solved the issue. The same error can be highlighted when adding a log inside the outer if statement. The comparison state.model.data !== prevDataRef.current returns true endlessly every time a cell changes its value.

React.useEffect(() => {
  if (state.model.data !== prevDataRef.current) {
      console.log('state.model.data is not equal to prevDataRef.current')
  }
  prevDataRef.current = state.model.data;
}, [state.model.data]);

It's worth mentioning that I later replaced that function to avoid slowdowns with a comparison made on a new lastUpdatedDate property of the state object. I think that using the date is a nice way to solve the issue since: it doesn't add much code or memory occupation, makes the comparison very fast, and provides additional information that may be useful for future features.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iddan : I also found a different weird behavior in the storybook version. Add some random text in a cell. Put the focus on another cell. Then go back to the cell, type 1 (the number 1). The focus will automatically disappear, so it will not be possible to continue writing in the cell. It might be related to the grid running in storybook, since i cant reproduce it in the "demo" version. (Running chrome)

};
}
case Actions.SET_CELL_DIMENSIONS: {
Expand Down Expand Up @@ -206,6 +208,7 @@ export default function reducer(
hasPasted: true,
mode: "view",
lastCommit: commit,
lastUpdateDate: new Date(),
};
}

Expand Down Expand Up @@ -276,6 +279,7 @@ export default function reducer(
hasPasted: true,
mode: "view",
lastCommit: acc.commit,
lastUpdateDate: new Date(),
};
}

Expand Down Expand Up @@ -385,6 +389,7 @@ function clear(state: Types.StoreState): Types.StoreState {
...state,
model: new Model(createFormulaParser, newData),
...commit(changes),
lastUpdateDate: new Date(),
};
}

Expand Down
1 change: 1 addition & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export type StoreState<Cell extends CellBase = CellBase> = {
>;
dragging: boolean;
lastChanged: Point | null;
lastUpdateDate: Date | null;
lastCommit: null | CellChange<Cell>[];
};

Expand Down
1 change: 1 addition & 0 deletions src/util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const EXAMPLE_STATE: Types.StoreState = {
},
},
lastChanged: null,
lastUpdateDate: null,
hasPasted: false,
cut: false,
dragging: false,
Expand Down