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

Improved localization APIs #327

Open
8 tasks
aklinker1 opened this issue Jan 6, 2024 · 22 comments · May be fixed by #343
Open
8 tasks

Improved localization APIs #327

aklinker1 opened this issue Jan 6, 2024 · 22 comments · May be fixed by #343
Assignees
Labels
Milestone

Comments

@aklinker1
Copy link
Collaborator

aklinker1 commented Jan 6, 2024

Feature Request

Instead of using browser.i18n.getMessage directly, include a custom i18n API that can be used in it's place.

It should:

  • Read localizations from <srcDir>/locales/[country-code].(json|json5|yaml|yml)
  • Generate correctly formatted .output/[target]/_locales/[country-code]/messages.json files
  • Support plural forms
  • Support interpolation

Basically, for message files that look like this:

# locales/en.yml
simple: Hello world!
manifestStyle:
  message: This is the translated text
  description: This is a description to give translators more context (it's never translated)
namedInterpolation: Hello $name1$, my name is $name2$
arrayInterpolation: Hello $1, my name is $2
some:
  nested:
    translation: You can nest translations inside deep objects
escapeTheDollarSign: You owe me $$100
pluralForm:
  0: Zero items
  1: 1 item
  n: $count$ items

Would be translated to look like this:

// .output/_locales/en.json
{
  "simple": {
    "message": "Hello world!"
  },
  "manifestStyle": {
    "message": "This is the translated text",
    "description": "This is a description to give translators more context (it's never translated)"
  },
  "namedInterpolation": {
    "message": "Hello $name1$, my name is $name2$",
    "placeholders": {
      "name1": {
        "content": "$1"
      },
      "name2": {
        "content": "$2"
      }
    }
  },
  "arrayInterpolation": {
    "message": "Hello $1, my name is $2"
  },
  "some_nested_translation": {
    "message": "You can nest translations inside deep objects"
  },
  "escapeTheDollarSign": {
    "message": "You owe me $$100"
  },
  "pluralForm": {
    "message": "Zero items | 1 item | $count$ items",
    "placeholders": {
      "count": {
        "content": "$1"
      }
    }
  }
}

The API would look something like this:

// Auto-imported from:
// import { createI18n } from 'wxt/i18n';

const i18n = createI18n();

i18n.t("simple");                                             // Hello world!
i18n.t("manifestStyle");                                      // This is the translated text
i18n.t("namedInterpolation", { name1: "John", name2: "Joe" }) // Hello John, my name is Joe
i18n.t("arrayInterpolation", ["John", "Joe"])                 // Hello John, my name is Joe
i18n.t("some.nested.translation");                            // You can nest translations inside deep objects
i18n.t("escapeTheDollarSign");                                // You owe me $100
i18n.t("pluralForm", { count: 0 });                           // Zero items
i18n.t("pluralForm", { count: 1 });                           // 1 item
i18n.t("pluralForm", { count: 4 });                           // 4 items

Acceptance Criteria

  • Currently, we generate types for browser.i18n.getMessage to make sure the keys passed in are type-safe. Let's keep this logic, but how it is generated will be different.
    async function writeI18nDeclarationFile(
    config: InternalConfig,
    ): Promise<string> {
  • Not everyone will be happy with the name "locales" for the new directory. We'll use that as a default, but let's add localesDir to the config to allow customization
  • After copying public assets, we'll need to read in all files in <srcDir>/<localesDir>/*, transform them, and output them to .output/[target]/_locales/[country-code]/messages.json. Public assets are copied here:
    const publicAssets = await copyPublicDirectory(config);
  • i18n.t
    • Plural form support
      • Use a special named placeholder, count, to detect keys that are plural
    • Interpolation
    • Escape $ correctly

What are the alternatives?

See #217 for a discussion of alternative methods.

Additional context

Created based on #217 (comment)

@dvlden
Copy link
Contributor

dvlden commented Jan 6, 2024

I love the idea.

Perhaps these two can come as improvement in the future:

  • Support plural forms
  • Support interpolation

I dislike the $ syntax for interpolation. Can't we use {xxx} curly braces as placeholder with variable name inside?

@ongnxco
Copy link

ongnxco commented Jan 7, 2024

Hi, may I ask about the code snippet within the file: entrypoints/popup/App.vue?

document.title = browser.i18n.getMessage('__MSG_extName__')

Even though the code still returns the expected result, in Visual Studio Code, it consistently shows a red underline under 'browser.' I am curious to know what the issue is with Visual Studio Code.

@aklinker1
Copy link
Collaborator Author

Even though the code still returns the expected result, in Visual Studio Code, it consistently shows a red underline under 'browser.' I am curious to know what the issue is with Visual Studio Code.

@ongnxco you may have to restart the typescript server after running pnpm i. The wxt prepare command generates a .wxt/types/imports.d.ts file, which, when included in the Typescript project, will define the type for the browser variable.

I don't know specifically why your VSCode might not respect the file generated in the .wxt directory.

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Jan 7, 2024

I love the idea.

Perhaps these two can come as improvement in the future:

  • Support plural forms
  • Support interpolation

I dislike the $ syntax for interpolation. Can't we use {xxx} curly braces as placeholder with variable name inside?

@dvlden plural forms can come later, but interpolation is a part of the existing browser.i18n.getMessage, so to replace that API effectively, we'll need to include interpolation in the initial release.

As for $name$ vs {name}... I agree. I think curly braces are better too lol. But my current thinking is that:

  1. I need to use the built-in APIs instead of creating a fully-custom solution to avoid importing and bundling all translations in all entrypoints
  2. Transforming the message files in the output directory to use $s adds a little complexity.
  3. I'm hesitant to make the message files too different than the standard chrome extension format - it would just add another step to migrating to/from WXT

But I could be convinced otherwise lol. I really don't like the built-in APIs and formats, but need to leverage them to avoid inflating the final bundle size.

Maybe I make a cleaner placeholder syntax a future enhancement? Edit: No, I only want to support a single method and I don't want to make breaking changes to this in the future

@dvlden
Copy link
Contributor

dvlden commented Jan 7, 2024

Oh so $ is already built in with browser.i18n? I did not know that it existed at all.

I think making a tiny custom wrapper to support curly braces is well worth it. The output then can contain $'s...

I guess it's more unnecessary work, but considering that WXT is opinionated, I think it should be built in for cleaner syntax.

Regarding migration, perhaps the invers cli, that they can run, that would take all public/_locales/lang-code/messages.json and transform/parse them to assets/locales/lang-code.json?

I can try to tackle this outside of WXT, to see how simple it could be achieved?

@aklinker1
Copy link
Collaborator Author

@dvlden Sure, if you want to take a crack at, I'll leave it to you!

@dvlden
Copy link
Contributor

dvlden commented Jan 8, 2024

After some investigation and playing, I thought that it'd be nice if in this opinionated approach, we drop description property, when it exists and transform it in the comment instead.

In this case, json goes out the way as it does not support comments natively (or I am outdated) and we can instead allow only json5 and yml/yaml cause they do have syntax for comments.

So instead of transforming this:

{
  "manifestStyle": {
    "message": "This is the translated text",
    "description": "This is a description to give translators more context (it's never translated)"
  }
}

Into the exact same output, we can do this:

{
  // This is a description to give translators more context (it's never translated)
  "manifestStyle": "This is the translated text"
}

Keeping it readable and avoid nesting... However, after further reading, I realised that browser also supports placeholders property. So perhaps instead of this:

{
  "manifestStyle": {
    "message": "This is the translated text with {somevar} here",
    "placeholders": {
      "somevar": {
        "content": "dynamic content placeholder"
      }
    }
  }
}

We do this instead?

{
  "manifestStyle": ["This is the translated text with {somevar} here", {
    "somevar": "dynamic content placeholder" 
  }]
}

I'm just trying to minimize nesting that native api implementation does. With lots of message properties and optional description and placeholder. For those, like me, that only use message it makes it a nightmare to look at, with its constant repetition.

So for my use-case, I think I can build custom script for now that I can use internally to have clean locale files that would be ran before dev/build scripts, to create appropriate _locale dirs and files.

I will play a bit more to see if I can get comments and placeholders to work the way I'd want it and give you a link to a repo, as I am trying it randomly with Node, outside of WXT for simplicity.

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Jan 8, 2024

The main reason I allowed the original manifest style is so people can migrate to WXT easily, without changing anything. I'd rather we continue allowing that style.

If an object has 3 or less keys, and all the keys belong to ["message", "description", "placeholders"], we know that object is a standard chrome extension object, and can just copy it over without any transformation.

There are obviously edge cases here with "reserved" key names, but I'm not too concerned because the typescript support will basically tell them if something's wrong.


I'd also prefer we auto-detect placeholders, so we don't have to define them at all in the source translation files, and generate them in the output JSON files. Actually, I don't even think we need to define the placeholders key for them to work in chrome extensions? Edit: We are required to define them, so we should just generate them when found in the message.

If someone wants to define placeholder information, they should use the original style. I don't think your array style is very readable, someone new to the framework would not recognize what the second object is for.

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Jan 8, 2024

I've updated the issue's description with valid JSON that we need to transform the file into. Some things that changed:

  • Added placeholders key for namedInterpolation
  • Replaced some.nested.translation with some_nested_translation, since you can't use . in keys

@aklinker1
Copy link
Collaborator Author

Here's a basic example that includes the JSON from above. i18n-example.zip

I don't know why I thought the browser.i18n API included support for named parameters, it doesn't. So if we want to support named parameters, we'll need to do it ourselves... So I think {} might make sense.

import { WxtI18n } from "wxt/browser";

export default defineBackground(() => {
  printMessage("simple");
  printMessage("manifestStyle");
  printMessage("namedInterpolation", ["Joe", "John"]);
  printMessage("arrayInterpolation", ["Joe", "John"]);
  printMessage("some_nested_translation");
  printMessage("escapeTheDollarSign");
  printMessage("pluralForm", ["0"]);
  printMessage("pluralForm", ["1"]);
  printMessage("pluralForm", ["4"]);
});

const printMessage: WxtI18n["getMessage"] = (key, substitutions, options) => {
  const str = browser.i18n.getMessage(key, substitutions, options);
  console.log(key, "-", str);
  return str;
};
Screenshot 2024-01-08 at 9 17 05 AM

@dvlden
Copy link
Contributor

dvlden commented Jan 8, 2024

Oh yea, looks like "named parameter" is only for placeholder use-case. I am not sure how is one being used, if there are multiple placeholders? Shouldn't it accept an K/V object to know where to place each dynamic value?

@aklinker1
Copy link
Collaborator Author

My understanding is that named placeholders are just inserted into the message where stated. The placeholders don't have to include interpolated parameters, they can be constant strings. Google provides a reason for this:

To define the text for a part of your message that shouldn't be translated. Examples: HTML code, trademarked names, formatting specifiers.

https://developer.chrome.com/docs/extensions/how-to/ui/localization-message-formats#placeholders

So maybe we can't support named substitutions like { name: "John" }. We're forced to use arrays like ["John"] if we use the browser.i18n.getMessage function under the hook.

A couple work-arounds I can think of:

  1. If we added type-safety to substitutions, we could add the placeholder's name as the name of the tuple, [name: string] instead of just [string]
  2. At build-time, we construct a mapping of named substitutions to their index, and use that mapping to convert the Record<string, string> to an array passed into browser.i18n.getMessage. That would require the mapping be bundled with each entrypoint though...

So I would say we go with option 1. Add types for substitutions and use named tuples instead of anonymous tuples.

@aklinker1
Copy link
Collaborator Author

Or a third option, stop using $name$ and use {name} like you suggested. Then have our library in charge of doing the substituations instead of passing an array into browser.i18n.getMessage.

Main concern here is performance loss. vue-i18n optimizes it's JSON by converting all keys to functions at built-time so that it doesn't have to call string.replaceAll("{name}", "value") at runtime.

I imagine the browser is doing some kind of optimization in it's native code as well, so doing the replacements at runtime is probably not a good idea.

So I still say we go with option 1 from the previous message. We're stuck with arrays :/

@dvlden
Copy link
Contributor

dvlden commented Jan 8, 2024

Ah I think we're not on the same page here.

My thinking is, that we can create a script that would be used during development and/or production build. This script would take public/_locales if directory exists and convert it to assets/locales. Otherwise, it'd take assets/locales and convert it to public/_locales.

It'd be a wishful thinking implementation, that WXT would handle in the background and the output of the build, would actually be the files that browser.i18n understands.

So people would use browser.i18n API as usual, but they wouldn't need to touch the messy files from public/_locales. Instead they just manage assets/locales with prettier syntax and every change, means remap our syntax to built-in required syntax.

I am not sure if I am explaining this well enough? Does that make any sense, is it worth it at all?

@aklinker1
Copy link
Collaborator Author

OK, so you're just considering the translation between the two file types.

That's fine, but I'm also considering writing a better API than browser.runtime.getMessage that is used at runtime. Which is what my previous 2 comments were about.

IE: How would we implement this?

const i18n = createI18n();

i18n.t("namedInterpolation", { name1: "John", name2: "Joe" }) // Hello John, my name is Joe
i18n.t("arrayInterpolation", ["John", "Joe"])                 // Hello John, my name is Joe

@yunsii
Copy link
Contributor

yunsii commented Jan 9, 2024

It seems UI framework agnostic? I want to know how to change locale if I use it in React?

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Jan 9, 2024

@yunsii The built-in localization APIs provided to chrome extensions do not provide a way to change the current language. Since WXT's wrapper will use those APIs internally, the API discussed here will not support changing languages either.

If you're using react, you can use a library like react-i18next for your UIs. I've provided a similar example of using vue-i18n along side the built-in APIs. You can refer to it as a reference to get started with React, it's very similar:

https://github.com/wxt-dev/wxt-examples/tree/main/examples/vue-i18n#readme

I'd recommend reading this one as well to get a better understanding of how to use the built-in APIs.

https://github.com/wxt-dev/wxt-examples/tree/main/examples/vanilla-i18n#readme

@yunsii
Copy link
Contributor

yunsii commented Jan 9, 2024

The built-in localization APIs provided to chrome extensions do not provide a way to change the current language. Since WXT's wrapper will use those APIs internally, the API discussed here will not support changing languages either.

Got it, thanks very much.

@aklinker1 aklinker1 linked a pull request Jan 13, 2024 that will close this issue
14 tasks
@aklinker1
Copy link
Collaborator Author

@dvlden I wanted to use this in one of my extensions, so I implemented this last night. See #343.

@dvlden
Copy link
Contributor

dvlden commented Jan 13, 2024

That's actually great... Can you make an alpha release, so I can test it within my extension?

@aklinker1
Copy link
Collaborator Author

@dvlden will do 👍

@dvlden
Copy link
Contributor

dvlden commented Jan 18, 2024

Will unassign myself as you already got it. It's not what I wanted, but it's still cool and useful.

@dvlden dvlden removed their assignment Jan 18, 2024
@aklinker1 aklinker1 self-assigned this Jan 19, 2024
@aklinker1 aklinker1 removed this from the v1.0 milestone Mar 3, 2024
@aklinker1 aklinker1 added this to the v1.1 milestone Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants