Break dependency on lit/localize #126

oyamauchi · 2024-02-05T18:02:21Z

Description

This is unfortunately a huge PR, so here's a detailed guide.

Using @lit/localize at runtime also entails depending on the
entirety of Lit; the localize package is not factored so that you can
just use the runtime string-substitution part. So we gotta get rid of it.

The idea here was to be minimally disruptive by simply swapping out
the msg and str functions from lit-localize with our own
implementations. That's in src/i18n/use-translated.ts and
src/i18n/str.ts. The strings:build command now also invokes sed to
adjust the import in the generated translated-strings files.

Rather than putting the current locale in global state like Lit does,
I put it in React context, accessed via the new custom hook
useTranslated. The hook returns the msg function, bound to the
current locale; callsites then use that msg function as before.

Most of the changed files are just plumbing in the new hook and/or msg
function. The most substantive changes are in:

state-calculator.tsx where the locale context is set
calculator.tsx -- this is the old embed, which was never actually
localized; I removed all the msg calls here since we're getting
rid of it Soon™
i18n/use-translated.ts -- the core of the new stuff

Test Plan

Run yarn strings:extract and make sure there are no changes to es.xlf.
Run yarn strings:build and make sure there are no changes.
View the calculator and hit Calculate; make sure all strings come up
looking correct in English. Add the lang="es" attribute to the
calculator element and make sure the UI immediately switches to
Spanish, except for incentive blurbs (which come from the API).

Hit Calculate again and make sure the incentive blurbs are now in Spanish.

vercel · 2024-02-05T18:02:26Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
embed-rewiringamerica-org	✅ Ready (Inspect)	Visit Preview	Feb 9, 2024 7:56pm

oyamauchi · 2024-02-05T19:16:18Z

src/state-incentive-details.tsx

@@ -187,28 +182,37 @@ const LinkButton: FC<PropsWithChildren<{ href: string }>> = ({
  </a>
 );

-const renderIncentiveCard = (key: Key, incentive: Incentive) => (


I turned this into its own component because passing the Key around felt like not what you're supposed to do

oyamauchi · 2024-02-05T19:16:55Z

src/state-incentive-details.tsx

@@ -301,7 +306,9 @@ const renderCardCollection = (incentives: Incentive[]) => (
    {incentives
      // Put IRA rebates after everything else
      .sort((a, b) => +isIRARebate(a) - +isIRARebate(b))
-      .map((incentive, index) => renderIncentiveCard(index, incentive))}
+      .map((incentive, index) => (
+        <IncentiveCard key={index} incentive={incentive} />


I know you're not supposed to use a numerical index as a key, but incentives don't have identifiers for now

I'd be in favor of returning one!

oyamauchi · 2024-02-05T19:17:34Z

src/state-calculator.tsx

@@ -397,7 +393,6 @@ export const StateCalculator: FC<{
          stateId={stateId}
          initialValues={getInitialFormValues()}
          showEmailField={!!showEmail && !emailSubmitted}
-          showProjectField={true}


Got rid of this as a drive-by change since it's always true now

RandomEtc · 2024-02-09T16:41:01Z

src/icon-tab-bar.tsx

@@ -42,23 +43,23 @@ export const IconTabBar: FC<Props> = ({ tabs, selectedTab, onTabSelected }) => {
        )}
        role="tab"
        aria-selected={isSelected}
-        aria-label={PROJECTS[project].label()}
+        aria-label={PROJECTS[project].label(msg)}


Reading these as "label using msg (to translate it)" - would it be clearer as "translated msg of label"? i.e. msg(PROJECTS[project].label) to avoid passing the msg helper into every label? Not sure if that breaks the use of hooks to trigger a re-render if the locale changes, but I don't think it does?

Oh I see, because we're still using lit localize at build time to extract strings, we still need a msg call site to allow it to know which strings need translations. Is that it?

Right, we're still using lit/localize-tools, which needs the argument to msg to be a string literal so it can extract it.

RandomEtc · 2024-02-09T16:47:18Z

package.json

@@ -24,7 +23,7 @@
    "lint": "tsc --noEmit && prettier --check . && eslint .",
    "prepare": "husky install",
    "strings:extract": "lit-localize extract",
-    "strings:build": "lit-localize build && prettier --write src/locales"
+    "strings:build": "lit-localize build && sed -e 's!@lit/localize!../str!' -i '' src/i18n/strings/*.ts && prettier --write src/i18n"


Do we get this from a dev dependency now?

You mean sed? It's included in every Unix-like OS. (Also this command is only run in local dev, not in CI, so it only needs to work on macOS.)

To be clear, the command is making this change: src/i18n/strings/es.ts

No I meant the lit-localize command - I see there's still lit/localize-tools in dev deps, which is what I was checking. From the diff I could only see the removal of the runtime package.

RandomEtc

I think this makes sense. Definitely jumping through a few hoops to use the msg function to tag things. I wonder if there's a way to use tagged strings to make them magically re-render based on context? Or if we should evaluate an alternative approach - I've used t('some.string.id') in other apps, with every language in a strings file (yaml or json) and english doesn't need to be hard-coded at the point of use. That might help avoid threading the msg function through, if we want to part ways with the lit toolchain entirely?

But this seems fine to keep us moving toward React-all-the-things.

RandomEtc · 2024-02-09T16:50:45Z

Note: I'm relying on the Cypress test here to verify the translated behavior is actually working, since we don't have a preview link with Spanish. It might be worth putting the preview build into a local copy of our consumer site to test more thoroughly?

oyamauchi

I admit this is a bit of a weird Frankenstein approach, but my rationale was:

This minimized the grunt work of going around changing every single msg callsite
It lets us keep using the same build-time translation workflow/tooling
I looked at alternatives, including the apparent standard react-i18next. It uses the t('string.id') approach, which I actually don't like -- I think having the user-visible English string in situ is a plus. You can see what string will show up just by scanning the usage site; you don't need to flip to another file.

This approach has the key limitation that it doesn't handle localizing strings that need to interpolate HTML, rather than just a string. (We had this problem in a recent PR with needing to localize View our <a href="...">terms</a>.)

We may very well switch to another localization framework at some point, but I think this is the quickest reasonable way forward for now.

oyamauchi · 2024-02-09T17:28:52Z

package.json

@@ -24,7 +23,7 @@
    "lint": "tsc --noEmit && prettier --check . && eslint .",
    "prepare": "husky install",
    "strings:extract": "lit-localize extract",
-    "strings:build": "lit-localize build && prettier --write src/locales"
+    "strings:build": "lit-localize build && sed -e 's!@lit/localize!../str!' -i '' src/i18n/strings/*.ts && prettier --write src/i18n"


You mean sed? It's included in every Unix-like OS. (Also this command is only run in local dev, not in CI, so it only needs to work on macOS.)

To be clear, the command is making this change: src/i18n/strings/es.ts

oyamauchi · 2024-02-09T17:29:50Z

src/icon-tab-bar.tsx

@@ -42,23 +43,23 @@ export const IconTabBar: FC<Props> = ({ tabs, selectedTab, onTabSelected }) => {
        )}
        role="tab"
        aria-selected={isSelected}
-        aria-label={PROJECTS[project].label()}
+        aria-label={PROJECTS[project].label(msg)}


Right, we're still using lit/localize-tools, which needs the argument to msg to be a string literal so it can extract it.

## Description This is unfortunately a huge PR, so here's a detailed guide. Using `@lit/localize` at runtime also entails depending on the entirety of Lit; the localize package is not factored so that you can just use the runtime string-substitution part. So we gotta get rid of it. The idea here was to be minimally disruptive by simply swapping out the `msg` and `str` functions from lit-localize with our own implementations. That's in `src/i18n/use-translated.ts` and `src/i18n/str.ts`. The `strings:build` command now also invokes sed to adjust the import in the generated translated-strings files. Rather than putting the current locale in global state like Lit does, I put it in React context, accessed via the new custom hook `useTranslated`. The hook returns the `msg` function, bound to the current locale; callsites then use that msg function as before. Most of the changed files are just plumbing in the new hook and/or msg function. The most substantive changes are in: - `state-calculator.tsx` where the locale context is set - `calculator.tsx` -- this is the old embed, which was never actually localized; I removed all the `msg` calls here since we're getting rid of it Soon™ - `i18n/use-translated.ts` -- the core of the new stuff ## Test Plan - Run `yarn strings:extract` and make sure there are no changes to es.xlf. - Run `yarn strings:build` and make sure there are no changes. - View the calculator and hit Calculate; make sure all strings come up looking correct in English. Add the `lang="es"` attribute to the calculator element and make sure the UI immediately switches to Spanish, except for incentive blurbs (which come from the API). Hit Calculate again and make sure the incentive blurbs are now in Spanish.

oyamauchi assigned veekas Feb 5, 2024

oyamauchi requested review from veekas and RandomEtc February 5, 2024 18:02

vercel bot deployed to Preview February 5, 2024 18:02 View deployment

oyamauchi changed the title ~~owen.localize~~ Break dependency on lit/localize Feb 5, 2024

oyamauchi commented Feb 5, 2024

View reviewed changes

Base automatically changed from owen.tooltips to main February 6, 2024 22:42

oyamauchi force-pushed the owen.localize branch from 2f65630 to 9583ec8 Compare February 6, 2024 23:07

vercel bot deployed to Preview February 6, 2024 23:07 View deployment

RandomEtc mentioned this pull request Feb 9, 2024

Get rid of <sl-icon> #122

Merged

RandomEtc reviewed Feb 9, 2024

View reviewed changes

RandomEtc approved these changes Feb 9, 2024

View reviewed changes

oyamauchi commented Feb 9, 2024

View reviewed changes

oyamauchi added 2 commits February 9, 2024 14:51

remove unused tooltip string

89527fa

oyamauchi force-pushed the owen.localize branch from 9583ec8 to 89527fa Compare February 9, 2024 19:52

vercel bot deployed to Preview February 9, 2024 19:53 View deployment

fix lint

5f3dced

vercel bot deployed to Preview February 9, 2024 19:56 View deployment

oyamauchi merged commit 2513ffc into main Feb 9, 2024
3 checks passed

oyamauchi deleted the owen.localize branch February 9, 2024 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break dependency on lit/localize #126

Break dependency on lit/localize #126

oyamauchi commented Feb 5, 2024 •

edited

Loading

vercel bot commented Feb 5, 2024 •

edited

Loading

oyamauchi Feb 5, 2024

oyamauchi Feb 5, 2024

RandomEtc Feb 9, 2024

oyamauchi Feb 5, 2024

RandomEtc Feb 9, 2024 •

edited

Loading

RandomEtc Feb 9, 2024

oyamauchi Feb 9, 2024

RandomEtc Feb 9, 2024

oyamauchi Feb 9, 2024

RandomEtc Feb 9, 2024

RandomEtc left a comment

RandomEtc commented Feb 9, 2024

oyamauchi left a comment

oyamauchi Feb 9, 2024

oyamauchi Feb 9, 2024

Break dependency on lit/localize #126

Break dependency on lit/localize #126

Conversation

oyamauchi commented Feb 5, 2024 • edited Loading

Description

Test Plan

vercel bot commented Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomEtc Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomEtc left a comment

Choose a reason for hiding this comment

RandomEtc commented Feb 9, 2024

oyamauchi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyamauchi commented Feb 5, 2024 •

edited

Loading

vercel bot commented Feb 5, 2024 •

edited

Loading

RandomEtc Feb 9, 2024 •

edited

Loading