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

[React I18n] I18n for partners #2589

Open
jonathanhamel4 opened this issue Mar 7, 2023 · 13 comments
Open

[React I18n] I18n for partners #2589

jonathanhamel4 opened this issue Mar 7, 2023 · 13 comments
Labels
Type: Enhancement 📈 Enhancement to our codebase

Comments

@jonathanhamel4
Copy link

jonathanhamel4 commented Mar 7, 2023

Creating this issue as a follow up on the Segmentation project migrating templates to a 1P app. This migration also adds i18n to the new Web sandbox runtime to allow apps to run:

const {i18n} = useApi();
const translation = i18n.translate('key', {replacement: value});

A custom implementation of i18n was added in the sandbox to be able to pass a synchronous function to handle translation replacements.

Reasons we could not use the exact implementation of react i18n:

Limitations of the current checkout-web implementation (the Admin implementation for UI Extensions is almost the same)

  • No support for repeated placeholders. (Web's implementation covered it)
  • No rigid behaviour when it comes to error handling
  • Supporting {{}} instead of {} as placehoders. It could be more flexible and accept both. The react-i18n translate function can take a interpolate function to support any of them.
  • No support for scope parameter
  • Flattening/merging could affect PluralRules.
@jonathanhamel4 jonathanhamel4 added the Type: Enhancement 📈 Enhancement to our codebase label Mar 7, 2023
@jonathanhamel4
Copy link
Author

cc @trishrempel @movermeyer @carolopolo

Summarizing the discussion we had earlier in this issue so we can keep track of the discussions.

Feel free to move the issue to a more appropriate place :)

@movermeyer
Copy link
Contributor

movermeyer commented Mar 9, 2023

The new API for translations for UI Extensions will send a flattened and merged locale object.

@jonathanhamel4

[Disclaimer: I'm a n00b in this area] Why? Does it have to be? Any links you could share?

@vividviolet
Copy link
Member

vividviolet commented Mar 9, 2023

The new API for translations for UI Extensions will send a flattened and merged locale object.

This is so that the logic for figuring out what translations is available based on the configured locales (with fallback replacements) and the current user/shop locale is all encapsulated in the backend instead of being replicated on the front end for each UI Extension surface area (Checkout, Admin, POS, Customer Accounts, etc...). It's also better for performance as we only need to send back a single flattened translation blob instead of all possible languages.

Note that we're only talking about flattening the translation JSON. This is not related to the actual translate function.

You can read more about it in the tech design.

@emileber
Copy link
Contributor

emileber commented Mar 10, 2023

a single flattened translation blob instead of all possible languages.

I'm not sure I see how sending a single language is related to flattening the whole thing?

@vividviolet
Copy link
Member

a single flattened translation blob instead of all possible languages.

I'm not sure I see how sending a single language is related to flattening the whole thing?

Since we want to reduce the amount of data sent the translations are flattened with fallback logic for filling in missing keys. If you look at the Tech Design there's an example in there that's pretty clear.

locales/en-GB.json

{
  "main": {
    "colorPickerPrompt": "Please select a colour"
  }
}

locales/en.json

{
  "main": {
    "colorPickerPrompt": "Please select a color"
  },
  "footer": {
    "privacyPolicyLabel": "Privacy Policy"
  }
}

Final flattened translation when the requested language is en-GB

{
  "main.colorPickerPrompt": "Please select a colour",
  "footer.privacyPolicyLabel": "Privacy Policy"
}

@emileber
Copy link
Contributor

emileber commented Mar 10, 2023

I still don't understand the need to flatten, as this could be done as easily with the original format?

{
  "main": {
    "colorPickerPrompt": "Please select a colour"
  },
  "footer": {
    "privacyPolicyLabel": "Privacy Policy"
  }
}

Which is what we do in customer accounts. The backend takes all the sources (defaults, themes overrides, fallback, etc) and creates a single set of non-flattened translations, injected into the DOM. This is then compatible out-of-the-box with @shopify/react-i18n.

Is it a limitation of the GraphQL schema?

@vividviolet
Copy link
Member

No, we can return in any format from GraphQL we want but the flattened structure is simpler and eliminate the need to do recursions as the translate function only need to take in a single key. It also match more closely with how the extension is expected to use the translation i18n.translate("main.colorPickerPrompt")

cc @kumar303 who might have more to add

@emileber
Copy link
Contributor

the flattened structure is simpler

Is it? Does the format even matter as we're going to provide the API to use the translation anyway, right? 🤔

eliminate the need to do recursions as the translate function only need to take in a single key

I'm still not sure I get it. If the i18n's translate function already takes a single key without any recursion needed with nested JSON, then which function are you referring to?

It also match more closely with how the extension is expected to use the translation

It doesn't match how the translations are defined though, which seems odd. 😕


Note that I don't really care if it's flattened or not, as long as the final translate implementation can handle both (maybe check for the whole key first, then split it and see if it's available in any nested object). I'm just wondering why we're going through this flattening process as it's unclear right now.

@emileber
Copy link
Contributor

Reading the different documents and PRs related to this, I've noticed something that might explained the confusion:

  • It seems that what is referred to as "flattening" is actually the process of merging multiple data sources

    flattens the up-to-3 locale dictionaries into one:

  • the actual flattening of the keys is referred to as unnesting

    Takes the flattened dictionary and "unnests" / compresses all the nested keys of the dictionary into a single level of keys

I understand the merging process and its benefits, it's the flattening process that seems unnecessary, or I was not able to find the requirement explaining the need to flatten the keys.


I won't be linking to the sources since this issue is public while all sources are private.

@movermeyer
Copy link
Contributor

movermeyer commented Mar 13, 2023

Note that this merging won't work for things like pluralization, since in order to choose which pluralization key to use, you need to know the locale of the keys you're picking from.

e.g., If you've fallen back to en (from fr), you need en pluralization rules, not fr rules, etc.

@kumar303
Copy link

The flattening / unnesting was done for a few reasons (maybe more 🤔):

  • the network request only includes the bare minimum needed to display strings in the current locale
  • the frontend can do a constant time lookup of key for any call to translate(key) (a minor optimization)
  • the frontend does not have to calculate fallback strings

FWIW, these are all implementation details. One could implement translate() however they want to as long as it adheres to the public interface and translation file formats.

Note that this merging won't work for things like pluralization

Checkout flattens / unnests the pluralization key, too:

{
  "main.colorPickerPrompt": "Please select a colour",
  "main.colorPickerSelection.one": "You have selected {{count}} colour",
  "main.colorPickerSelection.other": "You have selected {{count}} colours"
}

The translate() function uses count to create the correct lookup key based on pluralization.

@emileber
Copy link
Contributor

Thanks for the answers! As an extensibility dev on customer-accounts, a common i18n API meant for extension sandboxes sounds great.

That said, I still don't get why we would change the format? It sounds like we're mixing the merging and flattening process into the same argument, but from my point of view, they're different processes, each with their own concerns and caveats.

So let me share my understanding below.

Flattening (unnesting)

the network request only includes the bare minimum needed to display strings in the current locale

This could be an argument for merging multiple locales into a single dataset. Though it's a counter-argument for flattening as repeated key prefixes makes the dataset actually heavier (memory and parsing).

the frontend does not have to calculate fallback strings

This also seems to be related to merging data sources into a single one. Flattened or not, fallbacks could be used similarly in the same dataset.

the frontend can do a constant time lookup of key for any call to translate(key) (a minor optimization)

This is the first argument I read for flattening yet. Was there any noticeable impact on performance that could explain the new format? Isn't the flattening process impacting performance in some ways as well?

Merging multiple locales

Checkout flattens / unnests the pluralization key

@movermeyer was referring to merging different locales pluralization keys. It would not work unless we're aware of the locale used for the fallback, which is lost during the merging process. Flattening isn't really a concern here.

I'm onboard with merging different sources into a single dataset, as long as it's all under the same language tag (e.g. fr-CA).

  • default Shopify translations if any
  • theme translations if any
  • merchant overrides (default content, markets, etc)
  • etc.

However, there are some caveats with merging anything else for fallbacks:

  • e.g. we can't merge en fallbacks in a fr dataset (zero isn't plural in french).
  • we also can't assume that all locale files for the same language can be merged together. e.g. pt-PT and pt-BR

Alternative?

Based on these 2 example files:

// locales/en-GB.json
{
  "main": {
    "colorPickerPrompt": "Please select a colour"
  }
}
// locales/en.json
{
  "main": {
    "colorPickerPrompt": "Please select a color"
  },
  "footer": {
    "privacyPolicyLabel": "Privacy Policy"
  }
}

The Query response could be defined like this:

{
  "data": {
    "uiExtensions": [
      {
        "locale": "en-GB",
        "translations": {
          "main": {
            "colorPickerPrompt": "Please select a colour"
          }
        },
        "fallbackLocale": "en",
        "fallbackTranslations": {
          "footer": {
            "privacyPolicyLabel": "Privacy Policy"
          }
        }
      }
    ]
  }
}

This has all the informations to be able to properly translate using the @shopify/react-i18n lib, or a subset of that lib, while still being minimal. Instead of a merge, it's based on a "set" difference process.

@kumar303
Copy link

Checkout flattens / unnests the pluralization key

@movermeyer was referring to merging different locales pluralization keys. It would not work unless we're aware of the locale used for the fallback, which is lost during the merging process. Flattening isn't really a concern here.

Ah, yeah. Good catch ✨ I see that @movermeyer explained the problem here, too: https://github.com/Shopify/shopify/pull/339144

Alternative?

This seems fine to me. As mentioned earlier, these are all implementation details (i.e. partners won't have to change code). Implementors wouldn't need to follow the exact same approach but, of course, it would be nice to only inflict one set of bugs on our partners 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement 📈 Enhancement to our codebase
Projects
None yet
Development

No branches or pull requests

5 participants