gilbsgilbs / babel-plugin-i18next-extract

Babel plugin that statically extracts i18next and react-i18next translation keys.
https://i18next-extract.netlify.com
MIT License
161 stars 37 forks source link

Locales that include country code will break `keyAsDefaultValue` when combined with `locales` option #123

Closed Robin-Hoodie closed 4 years ago

Robin-Hoodie commented 4 years ago

Describe the bug

Specifying locales that have country codes in them, together with keyAsDefaultValue in the plugin options will disable extracting a <Trans/> component's content but will instead use the i18nKey attribute as the value

How to reproduce

Babel configuration:

{
  "presets": [
    "react-app"
  ],
  "plugins": [
    [
      "i18next-extract",
      {
        "locales": ["en-US"],
        "outputPath": "src/translations/{{locale}}/{{ns}}.json",
        "keyAsDefaultValue": ["en-US"],
        "discardOldKeys": true,
        "keySeparator": false,
        "nsSeparator": false
      }
    ]
  ]
}

Reproduction:

Add below snippet somewhere your <App> component

  <Trans i18nKey="hello">
          Hello <h1>world</h1> it is I!
   </Trans>

If you add this to your App.js file, the plugin will add a key value pair of hello: hello to your src/translations/en-US/translation.json file, instead of adding hello: Hello <1>world</1> it is I!

Expected behavior

The plugin will add the content of the Trans component as a value for the key hello to the file src/translations/en-US/translation.json {"hello": "Hello <1>world</1> it is I!"}

What actually happens

The plugin will add the value of the key itself to the file src/translations/en-US/translation.json { "hello": "hello" }

Your environment

Additional context

You can find a repo that reproduces this problem here: https://github.com/Robin-Hoodie/test-i18next-babel-plugin

Changing the locales to not include country codes, e.g .en instead of en-US will solve the issue and can suit as a workaround for now, but is not ideal

gilbsgilbs commented 4 years ago

Thanks for reporting this. The shortcode for american english is en_US, not en-US. Using a valid shortcode will probably fix your issue.

See #115. The error message should be improved.

gilbsgilbs commented 4 years ago

Actually, after testing against your project, it doesn't seem related to the bad shortcode. Sorry, but this is weird af.

gilbsgilbs commented 4 years ago

Actually, I don't really agree with the expected behavior you've given. The bug is for the en locale and not the en_US since you enabled keyAsDefaultValue and provided a key for the Trans component. If you don't want to use natural keys on Trans components, then don't provide keys for Trans components.

gilbsgilbs commented 4 years ago

Now I get it. By default useI18nextDefaultValue option is enabled for ["en"] locale only. It happens that this option supersedes the keyAsDefaultValue option (note that this is a conscious choice because default values are more likely to be good default values than the key names). So a solution in your case would be to set the useI18nextDefaultValue option to ["en_US"] (or to false if you do want to use the actual key as default value). Here is a config that would work in your case:

  {
        "locales": ["en_US"],
        "outputPath": "src/translations/{{locale}}/{{ns}}.json",
        "keyAsDefaultValue": ["en_US"],
        "useI18nextDefaultValue": ["en_US"],
        "discardOldKeys": true,
        "keySeparator": null,
        "nsSeparator": null
  }

I agree that this is rather confusing but I don't see how I could make a fix without breaking changes. We need to reconsider a couple of configuration defaults in v1. In the meantime, this question from FAQ is arguably incomplete and a PR would definitely be appreciated to indicate that useI18nextDefaultValue should be specified.

As a bottom note, I do really encourage you to rely on default values instead of natural keys in real-world apps. I think it's an inherently better approach to i18n because I've already encountered the limitations of natural keys. Natural keys look appealing at first, but as your application grows, you start to realize that they're very clumsy to work with. Also, note that you don't need to eject to use this plugin in create-react-app.

Robin-Hoodie commented 4 years ago

Now I get it. By default useI18nextDefaultValue option is enabled for ["en"] locale only. It happens that this option supersedes the keyAsDefaultValue option (note that this is a conscious choice because default values are more likely to be good default values than the key names). So a solution in your case would be to set the useI18nextDefaultValue option to ["en_US"] (or to false if you do want to use the actual key as default value). Here is a config that would work in your case:

  {
        "locales": ["en_US"],
        "outputPath": "src/translations/{{locale}}/{{ns}}.json",
        "keyAsDefaultValue": ["en_US"],
        "useI18nextDefaultValue": ["en_US"],
        "discardOldKeys": true,
        "keySeparator": null,
        "nsSeparator": null
  }

I agree that this is rather confusing but I don't see how I could make a fix without breaking changes. We need to reconsider a couple of configuration defaults in v1. In the meantime, this question from FAQ is arguably incomplete and a PR would definitely be appreciated to indicate that useI18nextDefaultValue should be specified.

As a bottom note, I do really encourage you to rely on default values instead of natural keys in real-world apps. I think it's an inherently better approach to i18n because I've already encountered the limitations of natural keys. Natural keys look appealing at first, but as your application grows, you start to realize that they're very clumsy to work with. Also, note that you don't need to eject to use this plugin in create-react-app.

Right I get it now, thanks for the clear explanation.

I hadn't given a thought to default values yet, I'll be sure to give it a try!

gilbsgilbs commented 4 years ago

Closed in #128 . Refer to #124 for changing the default behavior.