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

[fusion-cli#645] Improved Translation API via Hooks #340

Closed
chrisdothtml opened this issue May 9, 2019 · 12 comments · Fixed by #462
Closed

[fusion-cli#645] Improved Translation API via Hooks #340

chrisdothtml opened this issue May 9, 2019 · 12 comments · Fixed by #462
Assignees

Comments

@chrisdothtml
Copy link
Member

chrisdothtml commented May 9, 2019

This issue was migrated from fusionjs/fusion-cli#645 and was originally reported by @ganemone.


We should investigate if we can improve our translations API using hooks.

Type of issue

Feature Request

Description

The current API for working with raw translated strings is a bit cumbersome, as it requires duplicating the translation keys, once in the withTranslations call, and once in the translate call. This is further described here: fusionjs/fusion-cli#643

We should investigate various options of improving this API, potentially taking advantage of hooks.

@micburks
Copy link
Contributor

Previous discussions (fusionjs/fusion-cli#713 and fusionjs/fusion-cli#643).

Background

<Translate id="greeting" />

The above does 2 things

  • At build-time: The id value is picked up by static analysis to create a translation manifest
    • The manifest contains a mapping of which translations are used in which chunks
  • At run-time: The id value is passed into translate function on the i18n plugin
    • i18n plugin initializes by loading all translations
    • id keys into the loaded translations object

If a translation is not found, you don't know about it until the server attempts to grab that translation (on a render request or when a dynamic chunk is requested). There's no validation here, so it really just returns undefined.

Proposal

In supporting dynamic translation keys, we could potentially keep these 2 objectives together, but I think it would be simpler and make a more intuitive API to separate them.

// We can statically validate that this function is passed an array of static values
// (with some wildcard or other DSL)
staticAnalysisFunction(['static-key', 'dynamic-key.*']);

// No validation needed on this
runtimeTranslation(`dynamic-key.${foo}`);

The API would look like:

const translate = useTranslations(['greetings.*']);
translate('greetings.hello');

A custom hook lends well to this pattern since it ties one function call to a return function.

  • You wouldn't want to do one without the other
  • The runtime translation function would need to come from a hook since it needs access to the i18n service

Not much else needs to change. The manifest includes all static translation keys as well as any wildcard values. When the server attempts to get translations for a chunk, it just needs to be aware of wildcard values and use them to match against all of the loaded translation keys. The plugin middleware again just returns undefined if it can't find the translation key.

Full example:

function Component() {
  const translate = useTranslations(['greetings.*']);
  return (
    <h1>
      translate('greetings.hello')
    </h1>
  );
}

Full disclosure

This would be possible since the translate function just gives you access to i18n.translate

useTranslations(['greetings.*']);
const ctx = useContext(FusionContext);
const i18n = useService(I18nToken).from(ctx);
i18n.translate('greetings.hello');

@ganemone
Copy link
Contributor

I think this looks good. The only downside is we don't have any validation checking that the runtime values match the dynamic keys specified by the hook.

@micburks
Copy link
Contributor

Yeah that's true. You could also technically do this:

const translate = useTranslations(['greetings.*']);
translate('something-completely-different');

@ganemone
Copy link
Contributor

This is probably bad, but could we do some scoping?

const [greetings, hello] = useTranslations(['greetings.*', 'hello.*]);
<div>{greetings.a}</div>
<div>{hello.thing({data: 'test'})}</div>

@micburks
Copy link
Contributor

That's pretty interesting. The problem might be that we'd be converting a flat object whose keys can contain . to an actual object. It makes an assumption that people are using keys that way, but as far as I can find there is no strict standard way to implement translation maps other than needing to be flat.

@micburks micburks self-assigned this May 31, 2019
@rtsao
Copy link
Member

rtsao commented May 31, 2019

In the following case, I think the developer-supplied translation matcher is more or less entirely redundant:

const translate = useTranslations(["greetings.*"]);
translate(`greetings.${expression}`);

I think it stands to reason that if a convenient matcher exists, then an equivalent template literal could be used. It seems a bit pointless to require developers to supply a matcher themselves. This is just extra code that has to be updated in two places. In my view, the only benefit would be more granular matchers such as "greetings.{terse,verbose}" (to avoid over-fetching), but it's unclear if we will support anything but wildcards.

Suspense should empower us to have an API that "just works", at the expense of possible request waterfalls. However, manual, developer-provided translation matchers and/or automatic, best-effort static analysis can be used to alleviate this. Before Suspense, it would just mean if a translation is used before being fetched, it would return undefined. I think this is an OK outcome if we have more aggressive buildtime/runtime checks to avoid this in common scenarios. Ultimately, having an API that "just works" would be really nice.

I think a lint rule should be sufficient to guard against the worst footgun, which would be:

const translate = useTranslations();
translate(expression);

@rtsao
Copy link
Member

rtsao commented May 31, 2019

This is probably bad, but could we do some scoping?

const [greetings, hello] = useTranslations(['greetings.*', 'hello.*]);
<div>{greetings.a}</div>
<div>{hello.thing({data: 'test'})}</div>

This is interesting, I wonder if we could somehow get good static types for an API like this. Maybe generated typedefs?

@ganemone
Copy link
Contributor

I think we need to check that assumption with consumers. This also makes it more difficult to do more complex optimizations. For example:

const translate = useTranslations(["cities.*.areas.*]);
return <div>Welcome to {translate(`cities.${props.key}`)}</div>

If you were purely relying on the translate call, it would require loading everything under each city, when you only need the areas. Now you could potentially refactor that component to be:

const translate = useTranslations();
return <div>Welcome to {translate(`cities.${props.city}.areas.${props.area}`)}</div>

And that is arguably cleaner, but maybe the data comes from a backend with the full translation key, not split into city and area, and the FE may not want to deal with parsing the key into constituent parts as that adds complexity. etc etc.

@ganemone
Copy link
Contributor

This is probably bad, but could we do some scoping?

const [greetings, hello] = useTranslations(['greetings.*', 'hello.*]);
<div>{greetings.a}</div>
<div>{hello.thing({data: 'test'})}</div>

This is interesting, I wonder if we could somehow get good static types for an API like this. Maybe generated typedefs?

yea potentially. Although I'm not a huge fan of the aesthetics of this API. But maybe we could come up with something inspired by it. I was also trying to think of some way where we could get rid of static analysis and replace it all with code generation, but it gets ugly with nested keys and doesn't really work. But type generation could be possible.

@micburks
Copy link
Contributor

micburks commented Jun 1, 2019

I think what I'm seeing is there are some cases where static analysis completely fails to deliver the fewest possible translations (translate('cities.${props.city}.areas.${props.area}')), so the developer might agree that Suspense is worth the runtime cost in these situations.

Each of these methods has tradeoffs, but I see 3 patterns that are important to allow. Hopefully these APIs are suggestive of whether they're including translations in the bundle or not.

Static

  • lint and build errors catch non-static values
<Translate id="static"/>

Dynamic, include in bundle

  • lint and build errors catch non-hinted values
  • all values will be used, just don't want to manually type out each one
const translate = useTranslations();
const days = ['sunday', 'monday', 'tuesday', 'wednesday', 'thursday', 'friday', 'saturday'];
{days.map(day => (
  <div>{translate(`weekday.${day}`)}</div>
))}

Dynamic, fetch at runtime

  • no validation
  • huge number of possible matches
const Map = React.lazy(() => import('./map.js'));
function City() {
  const translated = translationResource.read(`cities.${props.city}`);
  return (
    <div>
      <div>Welcome to {translated}</div>
      <Map city={props.city}/>
    </div> 
  );
}
export default () => (
  <Suspense>
    <City/>
  </Suspense>
);

@micburks
Copy link
Contributor

micburks commented Jun 6, 2019

Is there any reason to not uplift the <Translate/> component to support hinted ids? After playing around with this, it's simple to support an API that handles both static and hinted values. I think if we're going to support APIs that combine hooks and Suspense (such as what's been discussed with RPC), we may want to avoid using hooks for things that will ultimately need Suspense. Bad idea for every plugin to come up with its own React usage pattern.

const days = ['sunday', 'monday', 'tuesday', 'wednesday', 'thursday', 'friday', 'saturday'];
{days.map(day => (
  <Translate id={`weekday.${day}`} />
))}

@ganemone
Copy link
Contributor

ganemone commented Jun 6, 2019

Is there any reason to not uplift the <Translate/> component to support hinted ids?

We could do this, but there will ultimately be a need for a hook (or some non component API) for translations in cases where the user needs to work with the translated string directly. For example, if you have a translated string for a native dom attribute (input placeholder for example).

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 a pull request may close this issue.

4 participants