-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
[locale] Split locales into separate files #46933
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
Conversation
|
The PR looks ok to me. I let core team check this does nto imply a breaking change @LukasTy From what I remeber you had to do an extra effort to get
|
| @@ -0,0 +1,74 @@ | |||
| import type { Localization } from './utils/LocaleTextApi'; | |||
|
|
|||
| const amET: Localization = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it would be simpler to replace these lines with immediate named exports and then use the same re-exporting strategy as in: https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/locales/index.ts
WDYT?
| const amET: Localization = { | |
| export const amET: Localization = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LukasTy I tried that approach originally, but ESLint got mad. It is currently configured to prefer default export on a file with one export. How should I proceed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add an exclusion to disable this rule on packages/mui-material/src/locale/* alongside this:
Lines 249 to 255 in 6afef27
| // Migrated config from packages/api-docs-builder/.eslintrc.js | |
| { | |
| files: ['packages/api-docs-builder/**/*'], | |
| rules: { | |
| 'import/prefer-default-export': 'off', | |
| }, | |
| }, |
Unless there is a clear objection from @mui/material-ui? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LukasTy I have made the recommended changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, LGTM. 👍
But I'd still like to see approval from someone more actively maintaining this repo. 🤔
P.S. @christopherschroer could you please run pnpm prettier and commit the fixes? 🙏
Signed-off-by: Lukas Tyla <[email protected]>
Netlify deploy previewhttps://deploy-preview-46933--material-ui.netlify.app/ Bundle size report
|
Done |
Can you elaborate on this? I think this PR would help the development because a single file is loaded instead of the whole localization but for production it should be the same because of tree-shaking. For "dynamically import individual locales", I don't think it's possible with |
|
@siriwatknp You are correct about my first claim. However when using a bundle analyzer, having a monolithic locale file makes it harder to determine which locales are being included. As for the second claim, you can dynamically import locales from MUI by creating individual locale files in your project that simply re-export the applicable locale from MUI and then lazy loading your project's locale files. It's not the slickest way of doing it by far, but it works with how MUI exports everything. It's how I dynamically loaded the locales from MUI-X |
|
Thank you for the comments. |
This non-breaking PR splits the single giant locale file found here into individual files for each locale.
Benefits: