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.

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

[DataGrid] localeText update does not re-render the grid with new translation #4119

Closed
2 tasks done
alexfauquette opened this issue Mar 8, 2022 · 12 comments · Fixed by #8029
Closed
2 tasks done

[DataGrid] localeText update does not re-render the grid with new translation #4119

alexfauquette opened this issue Mar 8, 2022 · 12 comments · Fixed by #8029
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!

Comments

@alexfauquette
Copy link
Member

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

From a stackoverflow message

https://codesandbox.io/s/datagridprodemo-material-demo-forked-ozkkjv?file=/demo.js

When changing the localeText the grid does not update. It displays the previous language. To have the correct one, we have to for a rerender by clicking on the grid

Expected behavior 🤔

When localText prop is updated, the components re-render texts

Steps to reproduce 🕹

No response

Context 🔦

No response

Your environment 🌎

`npx @mui/envinfo`
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Order ID 💳 (optional)

No response

@alexfauquette alexfauquette added status: waiting for maintainer These issues haven't been looked at yet by a maintainer bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Other labels Mar 8, 2022
@flaviendelangle
Copy link
Member

flaviendelangle commented Mar 8, 2022

apiRef.current.getLocaleText calls an outdated version of getLocaleText which do not yet have the new locals in its scope

That's weird, useGridApiMethod is supposed to handle this kind of update correctly

EDIT: apiMethodsRef is updated in a useEffect (which is necessary to avoid tearing in React 18)
But the consequence is that it has the outdated value here.
This probably causes other bug.

@alexfauquette alexfauquette removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 8, 2022
@flaviendelangle
Copy link
Member

flaviendelangle commented Mar 8, 2022

Related to facebook/react#14099 (comment)

Using useEventCallback from the core has the same behavior (update in effect)

@alexfauquette
Copy link
Member Author

I don't get what is the interest of useGridApiMethod compared to updating directly apiRef.current as follows

diff --git a/packages/grid/x-data-grid/src/hooks/core/useGridLocaleText.tsx b/packages/grid/x-data-grid/src/hooks/core/useGridLocaleText.tsx
index 9640ba0c..45321792 100644
--- a/packages/grid/x-data-grid/src/hooks/core/useGridLocaleText.tsx
+++ b/packages/grid/x-data-grid/src/hooks/core/useGridLocaleText.tsx
@@ -1,7 +1,6 @@
 import * as React from 'react';
 import { GridApiCommunity } from '../../models/api/gridApiCommunity';
 import { GridLocaleTextApi } from '../../models/api/gridLocaleTextApi';
-import { useGridApiMethod } from '../utils/useGridApiMethod';
 import { DataGridProcessedProps } from '../../models/props/DataGridProps';
 
 export const useGridLocaleText = (
@@ -13,14 +12,13 @@ export const useGridLocaleText = (
       if (props.localeText[key] == null) {
         throw new Error(`Missing translation for key ${key}.`);
       }
       return props.localeText[key];
     },
     [props.localeText],
   );
 
-  const localeTextApi: GridLocaleTextApi = {
-    getLocaleText,
-  };
-
-  useGridApiMethod(apiRef, localeTextApi, 'LocaleTextApi');
+  if (getLocaleText !== apiRef.current.getLocaleText) {
+    apiRef.current.getLocaleText = getLocaleText
+  }
 };

@flaviendelangle
Copy link
Member

With React 18, updating a ref in the render is unsafe.
Because you can have tearing (see this discussion)

And I don't see how to fix this problem in a React 18 compatible way, without deep changes

@flaviendelangle
Copy link
Member

I suppose similar problems can occur whenever we have

  • An api method A that uses some prop B
  • The method A is called in the render of DataGrid or a child component
  • The prop B changes

@alexfauquette
Copy link
Member Author

Since we are using a mutable store, I don't think we can have a reliable solution only with current hooks.

To keep the current implementation and support v18 we will need useSyncExternalStore

Did someone already had a look at useMutableSource now replaced by useSyncExternalStore which seems to be the solution adopted by react-redux?

@flaviendelangle
Copy link
Member

I did not yet
Do you know if Redux will handle React 17 on there future release ? That's a major decision that we will have to make at some point.

@m4theushw m4theushw changed the title localText update does not re-render the grid with new translation [DataGrid] localeText update does not re-render the grid with new translation Mar 14, 2022
@m4theushw
Copy link
Member

m4theushw commented Mar 14, 2022

I took a look at this issue and I noticed that it could happen with other props as well if a rerender doesn't occur after the prop change. In the first render after changing the locale, getLocaleText is called and it returns the English locale. Then, the effect below is called and apiMethodsRef is updated. If there was a 2nd render here, getLocaleText would return the Hindi locale, but there's none.

React.useEffect(() => {
apiMethodsRef.current = apiMethods;
}, [apiMethods]);

This issue doesn't occur with other props only because we usually call forceUpdate after syncing the state. To proof this, apply the diff below and access selectionModel inside an API method during the render phase. It will be called only once and with the outdated value.

diff --git a/packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts b/packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts
index 4c71685a..16167707 100644
--- a/packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts
+++ b/packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts
@@ -130,8 +130,6 @@ export const useGridSelection = (
       const currentModel = gridSelectionStateSelector(apiRef.current.state);
       if (currentModel !== model) {
         logger.debug(`Setting selection model`);
-        apiRef.current.setState((state) => ({ ...state, selection: model }));
-        apiRef.current.forceUpdate();
       }
     },
     [apiRef, logger],

As a workaround we could trigger a rerender manually once localeText changes. Or do IHMO the right thing which is to replace the ref with a state. Then, we ensure that the grid will rerender if apiMethods changes. The problem with this last option is that we'll have to go through all hooks to memoize the API methods.

@m4theushw m4theushw moved this from 🆕 Needs triage to 🔖 Ready in MUI X Data Grid Sep 9, 2022
@m4theushw m4theushw moved this to 🆕 Needs triage in MUI X Data Grid Sep 9, 2022
@m4theushw m4theushw moved this from 🔖 Ready to 📋 Backlog in MUI X Data Grid Sep 9, 2022
@m4theushw
Copy link
Member

The diff below seems to fix this problem:

diff --git a/packages/grid/x-data-grid/src/hooks/utils/useGridApiMethod.ts b/packages/grid/x-data-grid/src/hooks/utils/useGridApiMethod.ts
index 10dd4171..12130819 100644
--- a/packages/grid/x-data-grid/src/hooks/utils/useGridApiMethod.ts
+++ b/packages/grid/x-data-grid/src/hooks/utils/useGridApiMethod.ts
@@ -31,5 +31,6 @@ export function useGridApiMethod<Api extends GridApiCommon, T extends Partial<Ap
     installMethods();
   }, [installMethods]);
 
+  apiMethodsRef.current = apiMethods;
   installMethods();
 }

I didn't explore the consequences but it might not be recommended, according to #4119 (comment).

@mracette
Copy link

This is a bit of a sore spot for us as we just upgraded to Pro. Are there any workarounds that we can use locally while we await a long-term fix?

@scott-avery
Copy link

As one of the licensed users, we also found this issue recently.

@PrateekPadhy
Copy link

PrateekPadhy commented Feb 22, 2023

We are also facing a similar issue.

reproduced the issue here: https://codesandbox.io/s/focused-artem-fytv9q?file=/demo.tsx
localText for toolbarFilters don't update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants