GridItem: Fix cursor desync when used with React 18

[ashharrison90]

Description

Hi! Love the library ❤️

This PR fixes desynchronisation of the cursor with the grid item by synchronously applying state changes.

I don't think you can reproduce this on your demo page since it runs with React 16, however you can reproduce the problem on something like https://play.grafana.org/. Repro steps:

• visit https://play.grafana.org/ in chrome
• open chrome dev tools -> performance tab -> set cpu slowdown to 6x
• click and drag in the bottom right of a panel to resize
• notice the large desync between panel and cursor position

I believe this is caused by the new automatic batching introduced in React 18. The current logic in GridItem relies on the props and state being up to date each time the handler (e.g. onResizeHandler) is called. With automatic batching, the state is less likely to be up to date. The result is this drift or desync between the cursor position and e.g. the drag handle position.

The recommendation from the react team to opt out of automatic batching is to use flushSync. Maybe there's some refactoring that can be done to avoid using flushSync, but I think that would require a maintainer weighing in and taking a look since I'm not so familiar with the codebase. 😅

Another thing I'm not exactly sure about is when flushSync was exposed as part of the react-dom API. Using this may require a change to the minimum required version of react-dom specified in the peerDeps.

What type of PR is this? (check all applicable)

• 🍕 Feature
• 🐛 Bug Fix
• 📝 Documentation Update
• 🎨 Style
• 🧑‍💻 Code Refactor
• 🔥 Performance Improvements
• ✅ Test
• 🤖 Build
• 🔁 CI
• 📦 Chore (Release)
• ⏩ Revert

Related Tickets & Documents

Fixes #1975
Fixes #2003

Mobile & Desktop Screenshots/Recordings

Added tests?

• 👍 yes
• 🙅 no, because they aren't needed
• 🙋 no, because I need help

Added to documentation?

• 📜 README.md
• 📓 examples
• 🙅 no documentation needed

[shannonhochkins]

This fixes my issue's as well - for those who don't wish to wait for the update, you can have a simple postinstall script to update the package build files to get this going now:

const fs = require('fs');
const path = require('path');

// Path to the GridItem.js file
const gridItemPath = path.join(__dirname, 'node_modules/react-grid-layout/build/GridItem.js');

// Read the content of the file
fs.readFile(gridItemPath, 'utf8', (err, data) => {
  if (err) {
    console.error('Error reading GridItem.js:', err);
    return;
  }

  // Check if flushSync is already required
  const importStatement = "const { flushSync } = require('react-dom');\n";
  if (!data.includes(importStatement)) {
    // Add the import statement at the top of the file
    data = importStatement + data;
  }

  // Wrap all this.setState(...) calls with flushSync from react-dom
  const updatedData = data.replace(/this\.setState\(([^)]+)\)/g, 'flushSync(() => this.setState($1))');

  // Write the updated content back to the file
  fs.writeFile(gridItemPath, updatedData, 'utf8', (err) => {
    if (err) {
      console.error('Error writing to GridItem.js:', err);
      return;
    }

    console.log('GridItem.js has been successfully updated.');
  });
});

[Arkellys]

Hello @STRML, sorry to ping you, but do you think you have time to check this PR? It actually solves a major problem, as you can see on the attached issues. There are also other PRs fixing bugs that have been lying around. Thank you! 🙂

[shannonhochkins]

Is he still active? Or has someone else taken ownership?

[STRML]

I'll take a look.

[STRML]

I don't know why, but I'm unable to replicate the problem, neither in the demo code updated to React 18, nor in the Grafana dashboard. No matter how much I move the mouse at 6x slowdown, the resize catches up. What am I missing?

[Arkellys]

I don't know why, but I'm unable to replicate the problem, neither in the demo code updated to React 18, nor in the Grafana dashboard. No matter how much I move the mouse at 6x slowdown, the resize catches up. What am I missing?

If you want I described another way to reproduce it here.

[ashharrison90]

I don't know why, but I'm unable to replicate the problem, neither in the demo code updated to React 18, nor in the Grafana dashboard. No matter how much I move the mouse at 6x slowdown, the resize catches up. What am I missing?

@STRML ah apologies, we actually yarn patched these changes to react-grid-layout in grafana in the mean time with grafana/grafana#88099

edit: if there's a branch with the demo updated to react 18 i can try finding a recreate for you? 🤔

[STRML]

I don't know why, but I'm unable to replicate the problem, neither in the demo code updated to React 18, nor in the Grafana dashboard. No matter how much I move the mouse at 6x slowdown, the resize catches up. What am I missing?

@STRML ah apologies, we actually yarn patched these changes to react-grid-layout in grafana in the mean time with grafana/grafana#88099

edit: if there's a branch with the demo updated to react 18 i can try finding a recreate for you? 🤔

Sure I'll push one.

I want to recreate it so I can find a solution without flushSync as a crutch. Thanks!

[Abdullahbutt3434]

This fixes my issue's as well - for those who don't wish to wait for the update, you can have a simple postinstall script to update the package build files to get this going now:

const fs = require('fs');
const path = require('path');

// Path to the GridItem.js file
const gridItemPath = path.join(__dirname, 'node_modules/react-grid-layout/build/GridItem.js');

// Read the content of the file
fs.readFile(gridItemPath, 'utf8', (err, data) => {
  if (err) {
    console.error('Error reading GridItem.js:', err);
    return;
  }

  // Check if flushSync is already required
  const importStatement = "const { flushSync } = require('react-dom');\n";
  if (!data.includes(importStatement)) {
    // Add the import statement at the top of the file
    data = importStatement + data;
  }

  // Wrap all this.setState(...) calls with flushSync from react-dom
  const updatedData = data.replace(/this\.setState\(([^)]+)\)/g, 'flushSync(() => this.setState($1))');

  // Write the updated content back to the file
  fs.writeFile(gridItemPath, updatedData, 'utf8', (err) => {
    if (err) {
      console.error('Error writing to GridItem.js:', err);
      return;
    }

    console.log('GridItem.js has been successfully updated.');
  });
});

how I can fetch that changes in react? please help me thanks

[STRML]

@ashharrison90 #2049 has a branch running React 18. Tests are broken until we migrate off enzyme.

[ashharrison90]

@STRML thanks for the branch. attached a video of how i can repro on your demo page.

you need to set CPU throttling to 6x, grab a corner to resize a grid item and move the mouse back and forth very quickly:

Screenshare.-.2024-06-13.9_59_19.AM.mp4

notice how the panel corner is no longer aligned with the cursor.

i'm running chrome on an m1 macbook pro. if your machine is beefier it might be a little trickier. obviously this isn't representative of a real scenario, but the heavier your page is in general the easier this is to reproduce. it's a bit harder in your demo page because of how lightweight it is 😄 on something like grafana for example, just dragging normally with 6x slowdown will cause this type of desync. or if you're running on a slower machine like a raspberry pi.

and here's that branch with flushSync applied:

Screenshare.-.2024-06-13.10_02_23.AM.mp4

understand you wanna try and solve without flushSync, just wanted to highlight how i think it should behave.

[STRML]

Wildly, the latest Chrome build I started using has a "20x slowdown" option and I was able to very easily replicate the issue. This is great! I will see if a lightweight fix is possible.

[michaelnesen]

Thank you @ashharrison90 for finding patch and to @STRML for looking into a more permanent solution in #2049 🙏

I just wanted to drop a quick note to check in on the timeline for fixing this issue related to React 18. If there’s any way we can help move this over the finish line, just let us know. We’d be more than happy to lend a hand if you need it

[STRML]

@michaelnesen it's pretty much stalled on getting enzyme out of the build stack, which is a pretty big push that I just haven't had the time for. If that's something you would want to tackle (you could continue on and merge into #2049), that would be a huge help.

[michaelnesen]

@STRML is the idea to migrate to React Testing Library?

[STRML]

@STRML is the idea to migrate to React Testing Library?

Yep.

[ghoullier]

We use a patched version of react-grid-layout based on @ashharrison90 branch and it works perfectly.