-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Use FES internationalization within the minor version #7878
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
base: main
Are you sure you want to change the base?
Conversation
aa7780b
to
a8bbc48
Compare
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.
Sorry this took awhile to get to. One suggestion on alternative approach.
@@ -127,6 +129,7 @@ export let translator = (key, values) => { | |||
* Set up our translation function, with loaded languages | |||
*/ | |||
export const initialize = () => { | |||
let latestMinorVersionPath = 'https://cdn.jsdelivr.net/npm/p5@' + constants.VERSION.replace(/^(\d+\.\d+)\.\d+.*$/, '$1'); |
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.
Hi @ksen0 , @limzykenneth just a thought here.
Will it be a better idea to separate out the base CDN
URL here and the version parsing logic for better readability and reusability too, and then compose the final loadPath
. I was thinking to encapsulate it like below:
const getCDNPath = (() => {
const base = 'https://cdn.jsdelivr.net/npm/p5@';
const version = constants.VERSION.replace(/^(\d+\.\d+)\.\d+.*$/, '$1');
return `${base}${version}`;
})();
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'm not sure it's worth to split it out in this way, especially also encapsulating it in a function, if needed, just a string interpolation should suffice as well.
`https://cdn.jsdelivr.net/npm/p5@${constants.VERSION.replace(/^(\d+\.\d+)\.\d+.*$/, '$1')}`
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.
Yes, I see that there's no broader scope for reusability in this case, so I agree — using direct string interpolation is sufficient and keeps it clean.
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.
My initial thought was that separating the base URL and version logic might help with readability and clarity, especially if more dynamic paths were ever added later. But given the current usage, simplicity could be more prioritized.
Thanks for sharing this!
@@ -149,8 +152,7 @@ export const initialize = () => { | |||
}, | |||
backend: { | |||
fallback: 'en', | |||
loadPath: | |||
'https://cdn.jsdelivr.net/npm/p5/translations/{{lng}}/{{ns}}.json' | |||
loadPath: latestMinorVersionPath + '/translations/{{lng}}/{{ns}}.json' |
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.
and then here, we could compose it like
loadPath: `${getCDNPath}/translations/{{lng}}/{{ns}}.json`
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.
and then I think with string interpolation we could simply write with one liner change:
loadPath: `https://cdn.jsdelivr.net/npm/p5@${constants.VERSION.replace(/^(\d+\.\d+)\.\d+.*$/, '$1')}/translations/{{lng}}/{{ns}}.json`
Addresses #7813
npm run lint
passes