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.

Sign up for GitHub

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
Open

Added utility method to resolve locale supplied and fallback #2430

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f79fde7
Added utility method to resolve locale supplied and fallback
rajkeshwar Dec 12, 2024
82d05d9
Added the changeset
rajkeshwar Dec 12, 2024
0c9ea8c
fixed eslint
rajkeshwar Dec 16, 2024
6f43a92
Removed console.log
rajkeshwar Dec 16, 2024
93d2f74
Merge branch 'master' into feature/i18-dynamic-imports
rajkeshwar Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/lucky-crews-drop.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@lion/ui': minor
---

Added utility method to resolve locale supplied and fallback
20 changes: 20 additions & 0 deletions packages/ui/components/localize/src/utils/resolveLocaleConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
*
* @param {string} namespace Namespace for the locale
* @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 •
edited
Loading

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.

[namespace]: (locale = 'en') => {
const localePath = (/** @type {string} */ loc) => [localeDir, `${loc}.js`].join('/');

const fallbackLocale = localePath(locale.replace(/-\w+/, ''));

return import(localePath(locale)).catch(reason => {
console.warn(`

Check warning on line 14 in packages/ui/components/localize/src/utils/resolveLocaleConfig.js

View workflow job for this annotation

GitHub Actions / Verify changes 

Unexpected console statement
Locale ${locale} not found. Reason ${reason}.
Loading fallback locale ${fallbackLocale}`);
return import(fallbackLocale);
rajkeshwar marked this conversation as resolved.
Show resolved Hide resolved
});
},
});
19 changes: 19 additions & 0 deletions packages/ui/components/localize/test/resolveLocaleConfig.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { expect } from '@open-wc/testing';
import { resolveLocaleConfig } from '../src/utils/resolveLocaleConfig.js';

describe('resolveLocaleConfig', () => {
it('works correctly', async () => {
const localePath = new URL('./translations', import.meta.url);
const localeConfig = await resolveLocaleConfig('demo', localePath);

// When no locale is supplied `en.js` is loaded
expect((await localeConfig.demo()).default.hello).to.equal('en.js loaded');

// When `de-DE` locale is supplied `de-DE.js` is loaded
expect((await localeConfig.demo('de-DE')).default.hello).to.equal('de-DE.js loaded');

// When `de-AB` locale is supplied and the file does not exists.
// Then everything from - is removed and `de.js` is loaded. Which is equivalent to the default case.
expect((await localeConfig.demo('de-AB')).default.hello).to.equal('de.js loaded');
});
});
3 changes: 3 additions & 0 deletions packages/ui/components/localize/test/translations/de-DE.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default {
hello: 'de-DE.js loaded',
};
3 changes: 3 additions & 0 deletions packages/ui/components/localize/test/translations/de.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default {
hello: 'de.js loaded',
};
3 changes: 3 additions & 0 deletions packages/ui/components/localize/test/translations/en-GB.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default {
hello: 'en-GB.js loaded',
};
3 changes: 3 additions & 0 deletions packages/ui/components/localize/test/translations/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default {
hello: 'en.js loaded',
};
Loading