mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
92.44k stars 31.84k forks source link

enUS locale is exported, but as an empty object #21098

Open niklas-heeros opened 4 years ago

niklas-heeros commented 4 years ago

Current Behavior 😯

import { enUS } from '@material-ui/core/locale'

This import gives me an empty object.

Expected Behavior 🤔

To be able to import enUS (default) locale.

Steps to Reproduce 🕹

These steps are most likely not needed, because the locale is commented in the module: https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/locale/index.ts#L300

Context 🔦

I need to support dynamic language change, which seems not to be supported by current MUI localization. Therefore I'd like to import default locale, so I could use it with e.g. react-i18next.

oliviertassinari commented 4 years ago

For the en-US locale, the components already have these values by default. The exported object is meant to be empty. What's the issue?

niklas-heeros commented 4 years ago

As mentioned, the issue is when you need to support dynamic language change.

Context 🔦 I need to support dynamic language change, which seems not to be supported by current MUI localization. Therefore I'd like to import default locale, so I could use it with e.g. react-i18next.

If, for example, I set the pagination labels like this:

<TablePagination
  // `t` is from `react-i18next` and has MUI locales imported into `material-ui` namespace
  labelRowsPerPage={t('material-ui:MuiTablePagination.labelRowsPerPage')}
  labelDisplayedRows={t('material-ui:MuiTablePagination.labelDisplayedRows')}
  nextIconButtonText={t('material-ui:MuiTablePagination.nextIconButtonText')}
  backIconButtonText={t('material-ui:MuiTablePagination.backIconButtonText')}
/>

Is there something I'm missing? If there's out-of-the-box way for doing this in MUI, I would definitely use that.

oliviertassinari commented 4 years ago

@niklas-heeros Use the theme directly or provide undefined to your localization strings, which should already work.

niklas-heeros commented 4 years ago

Seems like theme does not contain the default locale or the default values. By using the theme you suggest to change/reset the whole MUI theme when language changes? There seems to be no way to change only the active locale.

undefined cannot be used as translation value as this is JavaScript and it would pretty much be the same as not passing the value at all. Usually i18n libraries have a fallback (e.g. the translation key) if the value is not found and default value is not given (undefined === not given).

Wouldn't it make more sense to set the default values from locales instead of having them directly coded into the components? This way the texts would be centralized and also could be exported, just like all the other locales.

oliviertassinari commented 4 years ago

Wouldn't it make more sense to set the default values from locales instead of having them directly coded into the components?

This would create duplication and can lead to out-of-sync, not ideal.

By using the theme you suggest to change/reset the whole MUI theme when language changes?

Yes, as in this demo https://master--material-ui.netlify.app/guides/localization/#example. Would it work for you?

niklas-heeros commented 4 years ago

How it would cause duplication and out-of-sync if the texts are only in the locale file?

I guess, it works, but it's far from optimal, as it forces part of the locale logic into another place - and also into a component. Currently our locale logic is tied to i18n.on('languageChanged') event handling.

oliviertassinari commented 4 years ago

@niklas-heeros Ok, well, maybe we can duplicate the information (uncomment the lines). It would improve the developer experience. In any case, we also need the en-US to be up to date to help with the translations in the other locales.

niklas-heeros commented 4 years ago

Requires a bit more work, but I meant the default values could be read from the locales like this:

Change backIconButtonText = 'Previous page' to backIconButtonText = enUS.MuiTablePagination.backIconButtonText

https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/TablePagination/TablePagination.js#L80

This way there would not be duplication.

oliviertassinari commented 4 years ago

@niklas-heeros We would need to break the locale.ts files into multiple chunks, one per locale if we want to do that, (for dev mode tree-shaking). I'm not sure we need to go as far.

niklas-heeros commented 4 years ago

Could also just have the default (en-US) in separate file and re-export it from the locale.ts?

oliviertassinari commented 4 years ago

@niklas-heeros It would still mean loading the English strings for all the components even if only importing one component. Not great. I'm more in favor of duplication.

diff --git a/packages/material-ui/src/locale/index.ts b/packages/material-ui/src/locale/index.ts
index 0877b0c9b..b68f0fdbf 100644
--- a/packages/material-ui/src/locale/index.ts
+++ b/packages/material-ui/src/locale/index.ts
@@ -298,7 +298,6 @@ export const deDE: Localization = {

 // default
 export const enUS: Localization = {
-  /*
   props: {
     MuiBreadcrumbs: {
       expandText: 'Show path',
@@ -346,7 +345,6 @@ export const enUS: Localization = {
       },
     },
   },
-*/
 };

 export const esES: Localization = {
eps1lon commented 4 years ago

@niklas-heeros Could you explain your concrete use case? It's not clear what the issue is with an empty object.

niklas-heeros commented 4 years ago

@oliviertassinari

Not sure how the compling is done, but would it really be the case if you define the default locale file like this and import only the component related texts:

export const MuiTablePagination = { ... }
export const SomeOtherComponent = { ... }
export const enUS = {
  MuiTablePagination,
  SomeOtherComponent
}

@eps1lon :

https://github.com/mui-org/material-ui/issues/21098#issuecomment-630053567

I do not know what more details you need than provided in the comment above and in the conversation overall. Empty object is not fine because it does not contain the en-US texts.

eps1lon commented 4 years ago

Empty object is not fine because it does not contain the en-US texts.

They are used in the components by default.

I guess the concern is bundle overhead if you're not using the english locale? That's a valid concern that applies to any defaults in the theme. We should provide a separate method that is built for customization. Localization shouldn't have happened in the theme. It ties us to a specific implementation before we really thought about it.

Either way the problem isn't an empty object. Ideally these objects would be opaque anyway since they're targeted at usage in Material-UI components. The only operation that's supported is creation of these objects for custom locales.

oliviertassinari commented 4 years ago

It's not clear what the issue is with an empty object.

@eps1lon I believe the issue is about the default string behavior. react-i18next will display the string key if missing.

For instance, t('material-ui:MuiTablePagination.labelDisplayedRows') will return "material-ui:MuiTablePagination.labelDisplayedRows" not undefined which prevent the default en-US props to be displayed by the component.

Localization shouldn't have happened in the theme. It ties us to a specific implementation before we really thought about it.

Time to explore the alternatives. What problem should we fix? :)

Ashok-da commented 2 years ago

I believe the question is rhetorical )

As far as I understand the root of the problem is how to use the localization of the mui along with the localization of the application that uses the mui.

I don't see any problem with this. We can use a separate provider (like https://react.i18next.com/latest/i18nextprovider) to localize application components as SomeOtherComponent.

Also, to customize the enUS locale of the mui components, in the end, it is needed to create a separate localization object. So I think that uncommenting the enUS locale is not necessary