i18next / i18next-parser

Parse your code to extract translation keys/values and manage your catalog files
MIT License
473 stars 195 forks source link

Keys extracted with escaped characters starting in v8.4.0 #886

Closed lgenzelis closed 1 year ago

lgenzelis commented 1 year ago

🐛 Bug Report

Up to v8.3.0, the following code would result in the key being extracted as "Don't click me":

<Trans shouldUnescape>Don&apos;t click me</Trans>

Starting in v8.4.0, it gets extracted as "Don&apos;t click me". The main reason this is an issue is that i18next will look for the key as "Don't click me", disregarding of whether shouldUnescape is true or false (I reported that as a separate issue here).

This basically means that all the keys extracted by i18next-parser which contained escaped characters will be ignored by i18next.

Your Environment

lgenzelis commented 1 year ago

Update: according to the answer I got in the issue I reported in react-i18next (https://github.com/i18next/react-i18next/issues/1662), shouldUnescape has nothing to do with the keys of Trans. So, this means that "Don&apos;t click me" should always be extracted as "Don't click me", no matter if shouldUnescape is set or not.

nicegamer7 commented 1 year ago

The only change in v8.4.0 that might've caused this (other than dependency updates) was a change to make it so that extracted keys are not equal to the defaults prop. In your example code, there is no defaults prop, and so this change is unlikely to have caused this.

I just want to make sure: you're sure this was working correctly without the defaults prop in v8.3.0? If so, I think this points to some dependency update as being the cause, though I could be incorrect about this.

karellm commented 1 year ago

I agree @nicegamer7 I don't think it was just introduced.

I'm trying to evaluate if we should actually unescape the key or not. It is a breaking change and not something I want to treat lightly. Is there a reason you don't follow @adrai suggestion to use the i18nKey option for the Trans component?

lgenzelis commented 1 year ago

I'm sorry, I didn't paste all my configuration because I didn't realize it was relevant (silly me, I know 😓 ). I guess the relevant part is this (but I'll paste everything below):

  defaultValue: function getDefaultValue(locale, __namespace, key) {
    if (locale !== 'en') {
      return '';
    }
    return key;
  },

The reason I don't use a key as suggested by @adrai is that I like to have my keys equal to their English translations.

Anyway, in case it is relevant, I can assure you this change in behavior is a result of 8.4.0. I just doubled checked: with 8.3.0, key is extracted with ', and in 8.4.0 it's extracted with &apos;.

As promised, this is all my i18next-parser.config.js :)

module.exports = {
  locales: ['en', 'es'],
  input: ['**/*.{ts,tsx}', '!cypress/**/*', '!node_modules/**/*'],
  output: 'public/locales/$LOCALE/$NAMESPACE.json',
  createOldCatalogs: true,
  keepRemoved: false,
  keySeparator: false,
  namespaceSeparator: false,
  defaultValue: function getDefaultValue(locale, __namespace, key) {
    if (locale !== 'en') {
      return '';
    }
    return key;
  },
  resetDefaultValueLocale: 'en',
  i18nextOptions: null,
  pluralSeparator: '_',
  contextSeparator: '_', 
  defaultNamespace: 'translation',
};
lgenzelis commented 1 year ago

I tried following @adrai 's suggestion and use a key, but that creates another issue. Since I'm using resetDefaultValueLocale: 'en',, if I give Trans a key then the engilsh translation will revert to being equal to the key whenever I run i18next.

nicegamer7 commented 1 year ago

We also use key fallback (where the value is the key) at the company I'm working at, but it seems to be working fine for us. I'll try to look into this in the coming days.

lgenzelis commented 1 year ago

I created a MWE of this issue using stackblitz. You can access it here. It's currently using i18next-parser 8.3.0. Line 18 of index.jsx shows

<Trans shouldUnescape>Don&apos;t click me!</Trans>

You can see in src/locales/en/translation.json that the key is Don't click me!. If you run npm run i18next, nothing will change.

But if you update i18next-parser to 8.4.0 (or 8.5.0/8.6.0), and run npm run i18next again, you'll see that the translation files are modified and the new key is Don&apos;t click me!. If you change the translation value to something different, you'll see that it has no effect, since i18next is actually looking for Don't click me!.

Please note that the usage of shouldUnescape shouldn't make any difference. The key should be extracted as Don't click me! no matter what (since i18next will look for it as Don't click me!, whether shouldUnescape is true or false). The fact that it did matter up to 8.3.0 was a useful workaround to this issue.

nicegamer7 commented 1 year ago

Thanks for the MWE and for your explanation @lgenzelis. After taking a look at our company's code, it looks like we're also being affected by this.

I believe this is indeed a bug @karellm, since the keys being extracted are not the same as the ones expected by react-i18next. Here's a screenshot of what react-i18next expects vs what we extract:

What react-i18next expects (got this using debug: true when initializing i18next):

image

What we extract it as:

image

The code itself:

image

This also made me realize the <br /> is not being extract correctly (might be a configuration error on our end, but I couldn't find any options for this).

edit: I found the config option transKeepBasicHtmlNodesFor, and we're not changing it. So I believe the above is a valid bug too.

edit2: never mind, we just didn't have transSupportBasicHtmlNodes set to true.

karellm commented 1 year ago

Should be fixed as of 8.7.0