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

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

Closed
lgenzelis opened this issue Aug 14, 2023 · 9 comments · Fixed by #892
Closed

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

lgenzelis opened this issue Aug 14, 2023 · 9 comments · Fixed by #892

Comments

@lgenzelis
Copy link

🐛 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

  • runtime version: node v18
  • i18next version: 23.4.4
  • react-i18next: 13.1.2
  • os: MacOs
@lgenzelis
Copy link
Author

lgenzelis commented Aug 14, 2023

Update: according to the answer I got in the issue I reported in react-i18next (i18next/react-i18next#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
Copy link
Contributor

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
Copy link
Member

karellm commented Aug 16, 2023

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
Copy link
Author

lgenzelis commented Aug 16, 2023

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
Copy link
Author

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
Copy link
Contributor

nicegamer7 commented Aug 16, 2023

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
Copy link
Author

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
Copy link
Contributor

nicegamer7 commented Aug 22, 2023

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.

SimonBackx added a commit to SimonBackx/Ghost that referenced this issue Aug 23, 2023
refs i18next/i18next-parser#886

- Locks the version of i18next-parser to v8.3.0
- Otherwise the `si` language gets updated with random \u200d characters in between.
SimonBackx added a commit to SimonBackx/Ghost that referenced this issue Aug 23, 2023
refs i18next/i18next-parser#886

- Locks the version of i18next-parser to v8.3.0
- Otherwise the `si` language gets updated with random \u200d characters in between.
SimonBackx added a commit to SimonBackx/Ghost that referenced this issue Aug 23, 2023
refs i18next/i18next-parser#886

- Locks the version of i18next-parser to v8.3.0
- Otherwise the `si` language gets updated with random \u200d characters in between.
SimonBackx added a commit to SimonBackx/Ghost that referenced this issue Aug 23, 2023
refs i18next/i18next-parser#886

- Locks the version of i18next-parser to v8.3.0
- Otherwise the `si` language gets updated with random \u200d characters in between.
SimonBackx added a commit to SimonBackx/Ghost that referenced this issue Aug 23, 2023
refs i18next/i18next-parser#886

- Locks the version of i18next-parser to v8.3.0
- Otherwise the `si` language gets updated with random \u200d characters in between.
SimonBackx added a commit to SimonBackx/Ghost that referenced this issue Aug 23, 2023
refs i18next/i18next-parser#886

- Locks the version of i18next-parser to v8.3.0
- Otherwise the `si` language gets updated with random \u200d characters in between.
SimonBackx added a commit to SimonBackx/Ghost that referenced this issue Aug 23, 2023
refs i18next/i18next-parser#886

- Locks the version of i18next-parser to v8.3.0
- Otherwise the `si` language gets updated with random \u200d characters in between.
@karellm
Copy link
Member

karellm commented Aug 23, 2023

Should be fixed as of 8.7.0

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.

3 participants