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

Added utility method to resolve locale supplied and fallback #2430

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rajkeshwar
Copy link
Contributor

@rajkeshwar rajkeshwar commented Dec 12, 2024

What I did

  1. Implemented utility method to resolve locale files. If supplied it loads the target locale file. If it doesn't find then it strips everything from - from the filename and loads it.
  2. Loaded localizeNamespaces dynamically for all the components.
  3. Modified resolveLocaleConfig to accept multiple locale directory and merge the results to one object
  4. Removed all the localizeNamespaceLoader.js because they are loaded dynamically

ex supplied de-DE loads de-DE.js file
supplied de-AB tries to load de-AB.js since the file doesn't exists it loads de.js file
if none of the locales are loaded then loads en.js

Copy link

changeset-bot bot commented Dec 12, 2024

🦋 Changeset detected

Latest commit: 93d2f74

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lion/ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

* @param {any} localeDir Directory of the locale
* @returns Promise<LocaleConfig>
*/
export const resolveLocaleConfig = (namespace, localeDir) => ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we apply the resolveLocaleConfig() to all lion-ui components that have translations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rajkeshwar.

Don't want to introduce a show stopper, but... keep in mind the reason for the current verbosity is to support all bundlers.

We came from a situation with oneliners like return import('@lion/ui/combobox-translations/${myLocale}.js').
Rollup could handle this, Webpack couldn't.

I'm afraid that delegating the dynamic import to an external function makes it harder for all bundlers to make correct chunks.

Copy link
Member

@tlouisse tlouisse Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N.B. it really is a good idea to standardize/simplify the localize code: the current state can definitely be improved.
But maybe we need to do some ready work/discussion together and formulate requirements before we do so. Like:

  • performance/bundling
    • will it be statically analyzable? (allowing for build optimizations like for instance the one described above)
    • will it result in a high Lighthouse score? (json files do better than js files that need to be parsed. We're looking in a fwd compatible way to introduce json files, so your function might help here (maybe we can use it under the hood in LocalizaManager to keep apis simpler?))
  • backwards compatibility
    • is it not breaking? Deprecate old apis to start with, so we can remove them at some point in the future
  • forwards compatibility
  • DX
    • The more competing apis and combinations we allow, the more complex it becomes: to understand, to maintain and to statically analyze (needed for performance)).
    • if we introduce something new, do we deprecate older apis and update our docs? So that we have one recommended way of doing things (also important for static analysis)
    • can we easily go from current conventions to new ones? Is it lintable? Is it codemoddable?

So maybe it would be good to co-operate with a colleague of us (Tom Nys (unfortunately I don't have his github handle atm)), who is already thinking about above things.

@rajkeshwar maybe you can start this discussion here: https://github.com/ing-bank/lion/discussions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tlouisse,

Thank you so much for the detailed explanation and for outlining the key areas to consider. To further explore this topic, I’ve started a discussion to gather input on the direction: #2431.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants