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

Updated translations, added build tool to generate translations for any language. #320

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

Conversation

josundt
Copy link
Contributor

@josundt josundt commented Aug 30, 2020

Fixes #260

I found that translations for Norwegian were not working. Then I found that the language code was wrong (no should have been nb). I also noticed that there were some inconsistencies between languages regarding inclusion/exclusion of prepositions like in etc. I therefore had an idea how to improve the quality of the translations.

Relative time formatting for different locales is now available natively in JavaScript through Intl.RelativeTimeFormat and is supported in:

  • node (when including ICU - International Components for Unicode).
  • most browsers (except Safari 😣 - (And IE - does anybody still use this?). The supported locales may potentially vary from browser to browser).

Since still not all browsers support Intl.RelativeTimeFormat, it can unfortunately currently not be used in web applications that target users with any browser, and the relative time support in aurelia-i18n is therefore valuable.

But: The translations in aurelia-i18n for relative time are maintained by the community, and the quality of the translations were at best questionable. I therefore thought it would be a good idea to use Intl.RelativeTimeFormat to generate the correct translations for aurelia-i18n.

I have therefore written a few node scripts that generate translations taken directly from Intl.RelativeTimeFormat when running in node with full ICU (_build/translation-generator/index.ts).

The tool can generate the expected aurelia-i18n relative-time translations for any language supported in the full ICU!
The small drawback is that this "translations-generator" can't produce the now translation - this phrase can not be retrieved using Intl.RelativeTimeFormat. For this reason the process unfortunately can't be fully automated as part of the build.

For now I have added the npm run-script "build:translations" that will create a generated ts file next to the existing translations file.

I have then updated the original file with the generated translations, assuming these are more correct.

I would like to suggest that aurelia-i18n discontinues to support translating "now".
"in 0 seconds" should be good enough, and that's what Intl.RelativeTimeFormat.format would output.
And it would be smart to align with the Intl.RelativeTimeFormat features, since it may be what aurelia-i18n will use internally in the not to distant future. If you agree and decide to to this you should be able to use my scripts to generate the translations as a fully automated build step.

…ntl.RelativeTimeFormat with node full-icu; updated existing translations.
@josundt
Copy link
Contributor Author

josundt commented Aug 30, 2020

I have updated a few tests and verified that the whole test suite succeeded locally (npm test), and also verified that the build worked (npm run build)

@zewa666
Copy link
Member

zewa666 commented Aug 30, 2020

Thanks for the contribution @josundt. @Sayan751 and I have had already the Intl RelativeTime feature on our plate. Safari, and specifically Safari iOS was one of the major holdbacks to do the port.

With your approach we'd definitely get the best of it going forward for unsupported browsers but it comes with a large overhead for typically only a small amount of locales used.

The best approach perhaps would be that the plugin ships your script and users can generate their own translations and register them via the plugin options. What do you think @Sayan751?

PS, while Intl translations are regarded standard perhaps some users still prefer the current ones. So the recommended approach would allow both options

EDIT:
ok hold on I think I've misunderstood and you're actually just fixing the existing ones instead of adding all available locales. Nevertheless it might be worth thinking about the alternative approach proposed in order to reduce the plugins size.

Additionally may I ask you to exclude the files from dist folders from this PR, as these will be generated during the release.

@josundt
Copy link
Contributor Author

josundt commented Aug 30, 2020

In my opinion, translations that comes with i18n should not differ from the defined language standards even if certain contributors prefer some different flavor. Personal preferences should probably not override national standards. Pluralization rules cannot be fully represented through i18n translations - f.ex. in Arabic I noticed that the numeric is not included when the integer the count is 1, but also not when it is 2! Certain locales have different rules for 1, 2 and many (and some even more). Still, getting as close as possible with the national standards will probably make the potential future transition to Intl.RelativeTimeFormat easier.

The challenge with the current solution as mentioned is that as you increase the number of supported locales you also increase the size of this plugin.

In the Aurelia applications developed by my organization, we use Aurelia with webpack, we do not use bundling of translations; we simply copy (and compact) the json source translation files to the webpack output folder (using CopyWebpackPlugin), and we have made our own fetch-based i18next loader that dynamically loads translations over HTTP when the user changes user locale (appending a per-build unique queryString value for cache-boosting).

For us it would have been a sufficient solution if the aurelia-i18n npm package included standard i18n per-locale JSON translation files for the relative time translations, so that we could include the languages we support in the webpack build process.

As an alternative, there could have been a small cli tool, a webpack plugin or similar included with the package to generate these files during build. The package would need full-icu as a dependency/peerDependency (a.k.a. not devDependency) for this to work.

To test my added translation generator, please review the build/translation-generator/index.ts file and maybe pull the PR and run npm run build:translations once. I think the code I wrote can be easily reused for both scenarios described above.

I have updated the PR, removing the dist folder and with a few improvements to the generator script.

@Sayan751
Copy link
Member

In Au2 we are already using the Intl API. Personally, I find it pretty cool. It means less things for me as a developer to ship with the application. We can gladly make the switch in Au1 as well.

Regarding browser support, devs can always use polyfill like this: https://www.npmjs.com/package/relative-time-format to get the translation.

import RelativeTimeFormat from 'relative-time-format';
import * as deRt from 'relative-time-format/locale/de.json';
import * as enRt from 'relative-time-format/locale/en.json';
RelativeTimeFormat.addLocale(enRt['default']);
RelativeTimeFormat.addLocale(deRt['default']);
Intl['RelativeTimeFormat'] = Intl['RelativeTimeFormat'] || RelativeTimeFormat;

Generating the translation using the Intl API, and shipping that with aurelia-i18n package on the other hand seems bit counter intuitive.

@zewa666
Copy link
Member

zewa666 commented Aug 31, 2020

Thanks for the update @Sayan751 totally forgot that it already got introduced in v2.

I wont be able to do the backport in the next two weeks as I'm starting tomorrow at my new job. If you can wait all good otherwise you maybe want to give it a shot by yourself. You can find the newly introduced rt method from the i18n service here which then would be used in the ValueConverter instead of the custom RtService. So yep it would result in a minor breaking change with a major bump but it shouldn't be too dramatic since I don't expect lots of users consuming the coded approach to create RTs

@josundt
Copy link
Contributor Author

josundt commented Aug 31, 2020

I think I will probably not be able to contribute to any "backport", but looking forward to the Intl based version in Aurelia 2.
I have personally already written an Intl based service+value converter+binding behavior solution for our applicatoin suite, my plan was actually to use aurelia-i18n as a fallback solution (for Safari etc).

@Sayan751 I was actually not aware that a polyfill existed for Intl.RelativeTimeFormat, I thought this feature would be hard to polyfill. But from the looks of your code sample, it seems to be to possible to both bundle locale json files or to dynamically load them over HTTP. The latter should be a good solution in our case.

I have tested 3rd party libraries earlier that used "raw" locale pluralization rules data to generate language phrases of different kinds. Some of those libraries were dynamically generating and evaluating code, and this did not work with our Content-Security-Policy (no script-src: 'unsafe-eval'). I hope the RelativeTimeFormat polyfill does not rely on dynamic code generation and evaluation.

I think I will cancel this PR, but I would still like to create a new PR with updated translations, at least for the Scandinavian languages. The current translations are bad, and the Norwegian (bokmål) language code is even wrong.

Please let me know if you want me to update only the Scandinavian languages or all the currently supported languages.

@zewa666
Copy link
Member

zewa666 commented Sep 1, 2020

Sounds good, yeah we'd be glad for the updates languages meanwhile. While at it let's do it for all currently supported.

@josundt
Copy link
Contributor Author

josundt commented Sep 2, 2020

@zewa666 I have now updated the translations in "src/defaultTranslations/relative.time.ts" as requested using my generator script. (I realized that I can actually also generate the "now" translation with Intl, so those are also updated for all the currently supported locales.)

I think I will ask for this PR to be merged after all instead of creating a new one. Hope that's OK. This PR includes, in addition to the udpated translations; 3 additional ts files under the build/translation-generator folder + an added npm run-script command (in package.json) - I guess it does not hurt to include this - you decide if you want to use it or not. If someone asks for a translation to another language you will be able to generate it by adding the new locale to the "supported-locales.ts" file and run the npm command.

Remarks:
My tool (npm run build:translations) could now actually rewrite the whole src/defaultTranslations/relative.time.ts file, the single drawback is that the node full-icu does not include the zh-TW locale, so you would loose that single translation. For this reason I decided that the command will still just writes to src/defaultTranslations/relative.time.generated.ts - right next to the original. (The path can be changed in the npm script in package.json if you decide to loose zh-TW for the sake of full automation).
(PS! You may notice that the zh-CH is also removed, since my generator removes "sub-locales" (e.g. zh-CH) when every translation is identical to the main locale (e.g. zh). This should not cause any issues).

I had to update a few tests and ran all tests + build successfully on my machine, not sure why the circleci reports test/ci failures.

Hoping you will accept this PR, and I'm also hoping that a minor release can be scheduled ASAP, since an upcoming product feature in my company relies on the improved translations.

@josundt
Copy link
Contributor Author

josundt commented Sep 7, 2020

@zewa666 Do you have time to review and hopefully accept this soon?

@zewa666
Copy link
Member

zewa666 commented Sep 7, 2020

Hey, super sorry for the delay but I'm burried under work currently. Can I ask you to keep the PR open but create one merely with the unit test fixes and the updated translations. I simply won't have time for a proper review and could get this new one merged quicker along with a patch release.

Thanks again and sorry for the troubles, hope things will get better in near future.

@josundt
Copy link
Contributor Author

josundt commented Sep 7, 2020

@zewa666 I have now created a separate PR with only the updated translations and tests: #323.

Feel free to review the translation-generator in this PR at a later time. It is just a separate standalone npm run-script command that you may run on demand when needed; I have not altered any other build or test scripts..

@josundt
Copy link
Contributor Author

josundt commented Sep 8, 2020

Fixed the misplaced zh-TW in relative time tranlations - same fix as in #323

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.

translations for relative time lack months and years in ar, th, sv, da and no
3 participants