[data grid] Doesn't work with Preact

[vsacom]

Steps to reproduce

Link to live example: Codesandbox: https://codesandbox.io/p/devbox/datagridpro-test-7-2-0-47rlrc

Steps:

1. Install Preact with default installation using typescript
2. Install DataGrid 7.* or DataGridPro v7.*
3. Run using vite in development mode ( npm run dev )

Attached is a simplified example:
datagrid_720_preact.zip

Current behavior

The Datagrid appears multiple times when running in development mode.
This concerns both DataGrid and DataGridPro after updating version from v6 to v7.

Note that preview-mode (npm run preview, build&serve) displays the DataGrid just once.

Expected behavior

The Datagrid appears only once

Context

No response

Your environment

npx @mui/envinfo

  System:
    OS: Linux 6.1 Debian GNU/Linux 12 (bookworm) 12 (bookworm)
  Binaries:
    Node: 20.9.0 - /usr/local/bin/node
    npm: 9.8.1 - /usr/local/bin/npm
    pnpm: 8.10.2 - /usr/local/share/npm-global/bin/pnpm
  Browsers:
    Chrome: Not Found
  npmPackages:
    @emotion/react: ^11.11.1 => 11.11.4 
    @emotion/styled: ^11.11.0 => 11.11.5 
    @mui/base:  5.0.0-beta.40 
    @mui/core-downloads-tracker:  5.15.15 
    @mui/material:  5.15.15 
    @mui/private-theming:  5.15.14 
    @mui/styled-engine:  5.15.14 
    @mui/system:  5.15.15 
    @mui/types:  7.2.14 
    @mui/utils:  5.15.14 
    @mui/x-data-grid: 7.2.0 => 7.2.0 
    @types/react:  18.2.79 
    react:  18.2.0 
    react-dom:  18.2.0 
    typescript: 5.4.5 => 5.4.5 

Search keywords: datagrid preact
Order ID: 82100

[michelengelen]

Ok, I can confirm this bug.
I have no idea why this is happening though.

@romgrk Could you have a look?
I have added the code to a testing repo here

[michelengelen]

Thanks for raising this issue @vsacom ... I have added it to our board!

[romgrk]

@vsacom Have you considered opening an issue with preact? The component runs fine under react, and if there is a difference when run under preact I would tend to think that preact's compatibility mode is failing one way or another.

[vsacom]

I have not reported this to Preact yet as the issue appeared just on the update of MUI-X.
Do you have any insight at the moment on what could be the cause and to be reported to Preact?

[romgrk]

No idea, I don't know what preact does and how it differs from react, and why it also differs in dev/prod modes. If you open an issue with them, they can probably provide more information that we could use to assist on our side. Our codebase is a pretty standard react codebase.

[rschristian]

To copy my reply from the Preact thread:

Preact doesn't support using NaN in dependency arrays, and it's questionable whether we ever will. Because of this, we throw an error via preact/debug (which you can see in the console) to warn the user as it's nearly always unintended. In this user's tooling, preact/debug is limited to dev mode and so it won't throw in prod, thereby "working", but the dependency array will not necessarily work as intended so long as NaN can end up in it.

Whether you consider than an issue here or not, up to you, but thought I'd mention it.

[romgrk]

Thanks for the feedback. Out of curiosity, why would you not support NaN values?

@vsacom I think I would consider this a bug in preact as it doesn't behaves the same as react does. I'm not against making small changes to the datagrid to make it work with preact as it offers benefits compared to react, but for this issue specifically I wouldn't support it. My thinking is that fixing this individual case still leaves you open to runtime production bugs of other instances of this issue that might be unnoticed at the moment, so fixing the root cause is much safer than working around individual cases like this one. I would suggest to look into running a build of the PR that fixes the issue for your production builds. I could also suggest running react in dev mode and preact in prod mode, I've done it in the past and with proper testing to catch the few incompatibilities present, it offers the best of both worlds.

[rschristian]

It's the same issue as trying to put an object in a dependency array. useEffect's dependency array (and all other dependency arrays) are meant to do a quick & cheap equality check (i.e., x == y), which NaN breaks as you are not meant to ever compare it like that. It doesn't make sense to support it as a special case as it really shouldn't end up in dependency arrays to begin with.

Preact has never aimed for 100% compatibility and this seems like something we shouldn't support (IMO, not speaking for the team).

I could also suggest running react in dev mode and preact in prod mode

Absolutely do not do this, you will run into unexpected bugs and we cannot recommend against this enough. It's a very, very bad idea to run different pieces of software in dev & prod. I know a few years ago someone on the NextJS team was promoting this but we've had dozens of users run into nasty surprises doing this.

On the Preact thread I gave some better options, such as patching out the error from preact/debug or disabling it (preact/debug), however, this just stops the error from being thrown. The inequality check will still fail and result in unnecessary renders.

[romgrk]

But for objects, referential shallow comparison is the correct behavior, while for NaN it's incorrect. And I feel like disallowing NaN pushes a lot of responsability onto users, as number variables need to be carefully evaluated each time they're used in dependencies to assess the possibility of a NaN. That's very tedious to deal with on the user side.

Is the reason for the === check that you want to trade correctness for performance? If that's the case, I would also point out that on modern engines Object.is is more like an operator than a function call, here is a quick benchmark that shows how it can be faster than === in many cases. Even in the bad cases, the difference vs === is quite low.

Benchmark results

(higher is better)

Source code

likely-equal: Values are mostly equal. Most realistic case, likely to reflect real-world scenarios. I am a bit suspicious of why it's so massively more performant; I've tried to mess with the benchmark to prevent optimizations that would only be applicable to micro-benchmarks, but I wasn't able to disprove the results.
likely-unequal: Values are mostly unequal.

Object.is vs is: Whether the function is assigned to a variable beforehand, e.g. const is = Object.is

Engines: node:V8, bun:JSC, gjs:SpiderMonkey

---

Anyway I got distracted by performance benchmarks again, but this data convinces me further that === has no advantage and we shouldn't try to deal with the issue here.

[rschristian]

while for NaN it's incorrect

Importantly, it's incorrect if NaN is viewed as a valid dependency. We don't (at the moment) consider it as such.

as number variables need to be carefully evaluated each time they're used in dependencies

All dependencies should be carefully evaluated, yes. Passing around NaN is nearly always a bug from not properly handling an operation; e.g., not having a fallback for a failed attempt to parse an integer from a string.

Object.is is more like an operator than a function call

The Preact 10.x line supports IE11, Object.is is non-viable for that reason.

---

I'm not trying to change your mind, fwiw, just explaining why we don't (and perhaps won't) support this. We have the issue, workaround, & PR on our tracker which should address this reasonably.

[romgrk]

The Preact 10.x line supports IE11, Object.is is non-viable for that reason.

You could use a shim as a fallback, Object.is || function is(...) { ... }.

Passing around NaN is nearly always a bug from not properly handling an operation

But not necessarily, it can also denote "not applicable", e.g. requests/seconds when there hasn't been any data yet (0 / 0).

I'm not trying to change your mind

👍 I do think you should change your position on this issue but don't feel obligated to reply.

[rschristian]

You could use a shim as a fallback, Object.is || function is(...) { ... }

We have tight size constraints.

it can also denote "not applicable"

It's much more typical to fallback to another value immediately, usually -1 to mirror .indexOf and co, i.e., n = parseInt(s) || -1. Passing around the error instance itself (which NaN is) to form a value at a later time isn't a convincing use case, IMO. I'd rather push users to different patterns.

[cherniavskii]

While I agree with Romain's points that NaN should be considered a valid value for hooks dependencies (0 / 0 is a valid use case that shouldn't require fallbacks on the user side), I think we should be open to making the changes on the Data Grid side.

I wouldn't aim for 100% compatibility with preact, but if the fix for the issue doesn't require a significant effort on our side, I would go for it to unblock the users. I think it would be fair to our community.
I would appreciate a PR from the community if there's an interest in making it work with preact.

@romgrk What do you think?

[romgrk]

I like preact and I'm ok with making efforts to support it, but I'm worried we're just hiding an issue that could pop up at runtime again. I see value in being "definitely broken" rather than "possibly broken". Preact doesn't support NaN and that's incompatible with react codebases, the only proper solution to guarantee a working datagrid imho is for users to run a preact build of the PR that fixes the root cause.

I would appreciate a PR from the community if there's an interest in making it work with preact.

Yeah I'm ok with that, it would at least indicate interest in using the partial fix.

[github-actions]

The issue has been inactive for 7 days and has been automatically closed.

[cherniavskii]

The fix for this issue would probably be this:

diff --git a/packages/x-data-grid/src/components/virtualization/GridVirtualScrollbar.tsx b/packages/x-data-grid/src/components/virtualization/GridVirtualScrollbar.tsx
index 5be8dfaf89..5feb8eaf8e 100644
--- a/packages/x-data-grid/src/components/virtualization/GridVirtualScrollbar.tsx
+++ b/packages/x-data-grid/src/components/virtualization/GridVirtualScrollbar.tsx
@@ -97,7 +97,7 @@ const GridVirtualScrollbar = React.forwardRef<HTMLDivElement, GridVirtualScrollb
         : dimensions.viewportOuterSize.width;
 
     const scrollbarInnerSize =
-      scrollbarSize * (contentSize / dimensions.viewportOuterSize[propertyDimension]);
+      scrollbarSize * (contentSize / dimensions.viewportOuterSize[propertyDimension]) || 0;
 
     const onScrollerScroll = useEventCallback(() => {
       const scroller = apiRef.current.virtualScrollerRef.current!;

[github-actions]

The issue has been inactive for 7 days and has been automatically closed.

[vsacom]

Thank you for the efforts from both Mui and Preact side.

The solution proposed by @cherniavskii looks promising and it's working in initial testing.
There are a few similar cases in DataGridPro as well, should I include details for them as well here or into another ticket?

[romgrk]

You can add here, but I would recommend patching preact as it's a more robust way of fixing the issue, not only for the datagrid but also for any other react component you might be using.

Also note that we're going to wait for an external PR for this issue, if you're interested you can submit one.

[rschristian]

FWIW, we've received one issue report (this one) in a year of throwing upon encountering NaN in dep arrays; most of our users are pretty happily using the React ecosystem w/out issue.

This doesn't seem to be all that common of a practice.