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

[data grid] Stop importing locales from @mui/material package #7028

Open
cherniavskii opened this issue Nov 28, 2022 · 7 comments
Open

[data grid] Stop importing locales from @mui/material package #7028

cherniavskii opened this issue Nov 28, 2022 · 7 comments
Labels
component: data grid This is the name of the generic UI component, not the React module! discussion l10n localization

Comments

@cherniavskii
Copy link
Member

cherniavskii commented Nov 28, 2022

Summary 💡

Currently, @mui/x-data-grid imports locales from @mui/material:

import { enUS as enUSCore } from '@mui/material/locale';

There are a few issues with this:

  1. When a new locale is added to @mui/material, the grid has to bump the minimal supported version of @mui/material to the version in which the new locale was released.
    Example: [DataGrid] Add Urdu (ur-PK) locale #6866
    This would force our users to upgrade their @mui/material dependency, even though most of them probably won't use the new locale.
  2. We need to decouple the grid core from @mui/material to support other UI libraries like @mui/joy

Examples 🌈

No response

Motivation 🔦

No response

Order ID 💳 (optional)

No response

@cherniavskii cherniavskii added component: data grid This is the name of the generic UI component, not the React module! discussion l10n localization labels Nov 28, 2022
@MBilalShafi
Copy link
Member

MBilalShafi commented Nov 28, 2022

That is a good initiative. 👍

One possible solution could be to duplicate translation needed (let's say for Table Pagination as it's the only one being used in x-data-grid from core) from @mui/material in @mui/x* but it can cause some further issues:

  1. When a new language is added in core, it won’t reflect for this component, i.e. inconsistencies in different locales for the same component, and we'll have to sync both the copies somehow
  2. If some other translation is needed let’s say we start using another component from core which has certain translations, we’ll have to replicate all those in x-repo too.

We can probably think about having a dedicated package for storing locales like @mui/locales which we can import like:

import { GridLocales } from '@mui/locales'
import { CoreLocales } from '@mui/locales'
import { PickerLocales } from '@mui/locales'

It may provide decoupling to be able to support other packages like Joy UI and also the need of bumping @mui/material will not be there for l10n reasons.

@flaviendelangle
Copy link
Member

I think that @mui/locales would be a pain to maintain.
It would mean doing a PR on another repo each time we want to update the locale of one of our components.
And duplicate some of the typing logic to have it available on @mui/locales (to type the pickers adapters for instance).

Not a huge fan of the idea to be honest

@MBilalShafi
Copy link
Member

The PR on other repo thing seems a real pain point, yes.
Then may be having a dedicated translation for MuiTablePagination in x-repo for the time being could be a good trade-off until we find a more scalable solution?

@flaviendelangle
Copy link
Member

Then may be having a dedicated translation for MuiTablePagination in x-repo for the time being could be a good trade-off until we find a more scalable solution?

If the whole pain comes from 2-3 translation keys re-used from the core, I do agree that duplicating is maybe the easiest way to remove this pain.

@flaviendelangle flaviendelangle changed the title [DataGrid] Stop importing locales from @mui/material package [data grid] Stop importing locales from @mui/material package Dec 1, 2022
@cherniavskii
Copy link
Member Author

I think we can solve (2) with something like this:

import { DataGrid, nlNL } from '@mui/x-data-grid';
import MaterialSlots from '@mui/x-data-grid/material';
// Material slots have their own locales that might import from `@mui/material`
import { nlNL as nlNLMaterial } from '@mui/x-data-grid/material/locales';

const localesText = {
  ...nlNL.components.MuiDataGrid.defaultProps.localeText,
  ...nlNLMaterial.components.MuiDataGrid.defaultProps.localeText,
};

<DataGrid localeText={localesText} />

This way we can decouple grid core locales and locales that are used by Material slots (e.g. GridPagination).

For (1) - maybe we should have a separate entry point for each locale?

import nlNL from '@mui/x-data-grid/locale/nlNL';

Then you'll only be affected if you use the specific locale.

But I also wonder what would actually happen if in #6866 we start exporting urPK locale and try using it with an older version of the @mui/material that doesn't export that locale yet.
Could you check it with a package built in Codesandbox CI from that PR @MBilalShafi ?

@flaviendelangle
Copy link
Member

flaviendelangle commented Dec 1, 2022

For (1) - maybe we should have a separate entry point for each locale?

The current bundling strategy do not support this depth of import.
That could be modified, and I like the nested locale import a lot.

with an older version of the @mui/material that doesn't export that locale yet.

Technically we should bump the minimum version of the peer deps.
Another approach would be to check if the material locale exists and throw otherwise with a message inviting the user to upgrade.

@MBilalShafi
Copy link
Member

MBilalShafi commented Dec 2, 2022

But I also wonder what would actually happen if in #6866 we start exporting urPK locale and try using it with an older version of the @mui/material that doesn't export that locale yet.
Could you check it with a package built in Codesandbox CI from that PR

I exported urPK by using enUS as a fallback from @mui/material and it works fine except the pagination component not translated, couldn't use urPK from @mui/material as it's not in the currently bumped version of @mui/material

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! discussion l10n localization
Projects
None yet
Development

No branches or pull requests

3 participants