feat(@aws-amplify/ui-components): I18n Support

[ericclemmons]

What This Does

• @aws-amplify/ui-components injects I18n with en translations set.

• Move UI strings from constants.ts to Translations enum for typechecking and usage within components.

• All components have been retrofitted to use I18n.get(Translations.*).

• Inlined single-use strings from constants.ts.

• Supports custom messages via:

   I18n.putVocabulariesForLanguage('en-US', {
     [Translations.SIGN_OUT]: 'Howdy',
   });

Callouts

Why only English?

Originally, I attempted to re-use translations from https://github.com/aws-amplify/amplify-js/blob/master/packages/aws-amplify-react/src/AmplifyI18n.tsx.

The problem is that those languages have ~41 translated strings, while the new UI components have ~65, many new & others being different from what existed before.

Because the new components weren't created to truly be 1-to-1, getting the two translations at parity would require a costly deep-dive into the two implementations to reconcile the differences (if even possible).

Supporting other languages would require effort similar to what it took for React to translate their docs:

https://www.youtube.com/watch?v=V9PW4HeZyBw

Punting on I18n was a costly mistake

By delaying I18n, this PR required a significant level of visual & automated (TypeScript, Jest) validation touching every component created. Had we opted for translation strings early via I18n.get, each component would've supported I18n out of the box with the same level of effort as it took to abstract strings into constants.ts.

constants.ts was an incorrect abstraction

Much has been written on this top that's worth reading, as engineers really love abstractions:

• https://twitter.com/CHERdotdev/status/1112819136147742720
• https://kentcdodds.com/blog/aha-programming
• https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction

In retrospect, there are seemingly clear abstraction boundaries to abide by:

• constant.ts for sharing true constants, such as response codes/strings from services like Cognito.

• AuthMessages for enumerating translation constants based on placement & intent (e.g. a header or submit button), rather than English phrases.

• Inlining strings that are used once or a few times meant for developers (and not end-users). These are typically logger.debug messages, or an exception like throw new Error(...). Co-locating messages with the context in which they happen is a feature, not a bug to abstract away.

• There are several calls to throw new Error(NO_AUTH_MODULE_FOUND) throughout the code. There are a couple ways to address this:

  • Preferably, an invariant utility to encapsulate the if logic as well:

   checkForAuthModule();
  

  • Alternatively, a custom Error:

   throw new MissingAuthModuleError(additionalContext);
  

[Delivers #171445892]

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ericclemmons]

@ashika01 A lot of our tests are really just validating the default @Prop() values, which doesn't seem as valuable as (1) type assertions or (2) snapshots that we already have.

You recently removed emotion & overrideStyle tests in #4947, and I feel confident that I can do the same here:

• Testing rendered values is best left to snapshots & newSpecPage:

  https://stenciljs.com/docs/unit-testing

  The value in snapshots is validating the rendered output (HTML) for a given input (props). Changing I18n definitions, default language, structural changes are potentially breaking changes, which is why these are so valuable! 🤗

• Testing interactivity should stay:

  https://stenciljs.com/docs/unit-testing#testing-component-class-logic

tldr; Remove .toEqual and .toBe assertions in favor of snapshots. Keep interactivity tests. Otherwise, every test will require changes just to match what snapshots are doing out-of-the-box:

diff --git a/packages/amplify-ui-components/src/components/amplify-sign-out/amplify-sign-out.spec.ts b/packages/amplify-ui-components/src/components/amplify-sign-out/amplify-sign-out.spec.ts
index 8fcf8a670..b8ea3259f 100644
--- a/packages/amplify-ui-components/src/components/amplify-sign-out/amplify-sign-out.spec.ts
+++ b/packages/amplify-ui-components/src/components/amplify-sign-out/amplify-sign-out.spec.ts
@@ -1,6 +1,7 @@
+import { I18n } from '@aws-amplify/core';
 import { newSpecPage } from '@stencil/core/testing';
 import { AmplifySignOut } from './amplify-sign-out';
-import { SIGN_OUT } from '../../common/constants';
+import { AuthMessages } from '../../common/types/AuthMessages';

 describe('amplify-sign-out spec:', () => {
   describe('Component logic ->', () => {
@@ -11,7 +12,7 @@ describe('amplify-sign-out spec:', () => {
     });

     it('should render default `buttonText`', () => {
-      expect(signOut.buttonText).toEqual(SIGN_OUT);
+      expect(signOut.buttonText).toEqual(I18n.get(AuthMessages.SIGN_OUT));
     });

     it('should render `overrideStyle` to false by default', () => {

[ericclemmons]

In retrospective, I believe my points are valid but can see emotional responses to "losing" tests.

The problem with our "component logic" tests is that they're not testing logic. But it's a relatively small lift to get them passing without removing them.

[codecov]

Codecov Report

Merging #4979 into ui-components/master will not change coverage.
The diff coverage is 66.66%.

@@                  Coverage Diff                  @@
##           ui-components/master    #4979   +/-   ##
=====================================================
  Coverage                 75.85%   75.85%           
=====================================================
  Files                       177      177           
  Lines                      9683     9683           
  Branches                   1910     1860   -50     
=====================================================
  Hits                       7345     7345           
- Misses                     2187     2198   +11     
+ Partials                    151      140   -11

Impacted Files	Coverage Δ
...ckages/storage/src/providers/axios-http-handler.ts	27.9% <0%> (ø)	⬆️
...torage/src/providers/AWSS3ProviderManagedUpload.ts	95.48% <100%> (ø)	⬆️
...ckages/storage/src/providers/httpHandlerOptions.ts	100% <100%> (ø)	⬆️
packages/storage/src/providers/AWSS3Provider.ts	87.09% <100%> (ø)	⬆️
...storage/src/providers/httpHandlerOptions.native.ts	100% <100%> (ø)	⬆️
packages/core/src/OAuthHelper/GoogleOAuth.ts	32.65% <0%> (ø)	⬆️
...pubsub/src/Providers/AWSAppSyncRealTimeProvider.ts	18.67% <0%> (ø)	⬆️
packages/core/src/Credentials.ts	31.45% <0%> (ø)	⬆️
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15e089a...c8cd9c0. Read the comment docs.

[ashika01]

I like this approach. Could you just do some knowledge sharing on the comments?

Also, a general question, I like the blogs you posted, it makes sense in cases of abstraction, but having strings in constant doesn't completely fall in the abstraction world. I like having the strings used in constant.ts so in future it makes changing the string value easier than going to all the place inside the code.

What are your thoughts there? Especially in similar cases like NO_AUTH_MODULE_FOUND make it as util might be unwanted abstraction vs keeping it in string constant. (Maybe I am missing clarity here :))

[ashika01]

Why do we set this as en-US? Could we make it something generic so anyone can inject another language too?

[ericclemmons]

This default is largely here for one reason: newSpecPage() doesn't set window.navigator.language by default, like browsers do.

This code needs to change, as there's a bigger problem: we only have an English translation, so I18n defaulting to fr-ca will return enum values (e.g. SIGN_OUT) instead of the English "defaults".

Based on how I18n.get works (https://aws-amplify.github.io/docs/js/i18n#get), we can address this a couple of ways:

1. Use a custom I18n wrapper for UI components that falls back to English when a translated value doesn't exist.

2. Document and encourage customization via the AuthMessages enum, but move the "default" English phrases to enum values:

   Customers will do this:

   I18n.putVocabulariesForLanguage('en-US', {
     [AuthMessages.SIGN_OUT]: 'Howdy',
   });

   Behind the scenes, we have:

   enum AuthMessages {
   	SIGN_OUT = 'Sign out'
   }

3. Update our I18n.get(...) calls to provide the English phrase as a default value:

   https://aws-amplify.github.io/amplify-js/api/classes/i18n.html#get

The biggest takeaway is that UI components should render default phrases even if I18n.setLanguage isn't English.

[ashika01]

UI components should render default phrases even if I18n.setLanguage isn't English.

Agreed. I like the concept of wrapper to fallback to english. Do you want to add it to this PR?

[ericclemmons]

@ashika01 I ended doing something "simpler", which can be described as _migrating constants.ts to Translations.ts:

• Customers can customize translations:

   import { Translations } from "@aws-amplify/ui-components";
  
   I18n.putVocabulariesForLanguage('en-US', {
     [Translations.SIGN_OUT]: 'Howdy',
   });

• No more need to default a language via I18n.setLanguage

• The values of the Translation enums are the default English values.

• No need for a wrapper.

• Still adds I18n.get to the components, so we're ready for customer-provided translations.

• No need to instrument tests.

[ericclemmons]

Confirmed:

$ node
> require('@aws-amplify/ui-components').Translations
{ BACK_TO_SIGN_IN: 'Back to Sign In',
  CHANGE_PASSWORD_ACTION: 'Change',
  CHANGE_PASSWORD: 'Change Password',
  CODE_LABEL: 'Verification code',
  CODE_PLACEHOLDER: 'Enter code',
...

[ericclemmons]

@ashika01 Great questions! I'd like to take the time to answer this, since my opinions have changed over the years on this:

I like having the strings used in constant.ts so in future it makes changing the string value easier than going to all the place inside the code.

You're right, as this is where things get subjective. We agree that it's about abstractions that make maintaining code easier & clearer. (My litmus test for when to or when not to abstract directly correlates with the friction/frustration to maintain code in it's present state.)

For example, IME bulk changes typically happen in dictionaries (e.g. I18n) or in per-env config files (e.g. node-config).

The majority of the time, changes were within the context of the UI (e.g. label text, button aesthetics, # of retries, etc.). Co-locating/inlining values served to:

• Maintain context between what the code does with how it does it.
• Eliminate dead code (i.e. unused constants & config values are difficult to keep up-to-date).
• Reduce cross-referencing (i.e. fewer files open to know what something does).

Ultimately, my opinions stem from frustration with this PR taking more effort than it should have, because one-time use values were removed from the context of the UI and coupled with real constants.

What are your thoughts there? Especially in similar cases like NO_AUTH_MODULE_FOUND make it as util might be unwanted abstraction vs keeping it in string constant. (Maybe I am missing clarity here :))

In this case, we have 24 occurrences of code like:

    if (!Auth || typeof Auth.federatedSignIn !== 'function' || typeof Auth.currentAuthenticatedUser !== 'function') {
      throw new Error(NO_AUTH_MODULE_FOUND);
    }

I think we agree that 24 occurrences of copy/pasta is a bit extra. I can't even say that this check even needs to exist, since we rely on import { Auth } from '@aws-amplify/auth'; (which would fail when bundling).

More importantly, the code isn't clear on why this check needs to happen and when it should happen in the UX.

Abtracting to an invariant check would be the first step to answering these questions.

[ashika01]

Cool. Thanks for explaining this @ericclemmons . I am good with this right now. Lets think more on the Auth-Module one and maybe revisit in later PR if needed.

[ashika01]

This is looking good @ericclemmons 🌮 🎉 🚢

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.