search

i18next / i18next-parser

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

Question(/maybe regression): should warnings be emitted for non-literal children when i18nKey (and defaults) are specified? #899

Closed glsignal closed 1 year ago

glsignal commented 1 year ago

💥 (Potential) Regression Report

Historically, with the JsxLexer, it has been possible to use the Trans component with a i18nKey and defaults specified via props, however since 8.6.0 this has produced a warning. Since we use the 'failOnWarnings' as a safety check in our build pipeline, this change has prevented an update.

I'm opening this to check the intended behaviour, and whether this is actually a regression or a behaviour that we have been unaware of that is now more visible due to the warnings.

I've added a couple of test cases here which illustrate two scenarios where I have previously expected no warnings, and wanted to check which cases are valid.

Link to the test cases

Scenarios where a warning is emitted for a non-literal child

  1. Trans is used with i18nKey => previously no warning, now warns -> this is correct
  2. Trans is used with i18nKey and defaults => previously no warning, now warns -> this should not warn

~For these cases, should both of these produce warnings? I'm reasonably sure that 2. shouldn't, but less sure about whether 1. is a legitimate warning or not.~ (see comments below)

For context, in our test suite we mock parts of the translation tooling and use non-literal children of the Trans component to test some dynamic aspects of the translations (injected components for example).

Last working version

Worked up to version: 8.5.0

Stopped working in version: 8.6.0

To Reproduce

Steps to reproduce the behavior:

<Trans i18nKey="test.key" defaults="defaults">
  {myVar}
</Trans>

When scanning, the JsxLexer will emit a warning about a non-literal child

Expected behavior

A clear and concise description of what you expected to happen.

<Trans i18nKey="test.key" defaults="defaults">
  {myVar}
</Trans>

Since both the key and the defaults are specified, I would expect no warning to be emitted based on descendants of the Trans element

Your Environment

karellm commented 1 year ago

@glsignal Thanks for raising this. At first sight, I agree that 2. shouldn't raise a warning. 1 should still trigger a warning imo since we are trying to extract a default value from a variable which doesn't work.

Do you think you can make a PR to tackle this?

glsignal commented 1 year ago

@karellm I'd tend to agree, even if it does make more work for my consumer 😉

I'll have a look into it and see if I can make some headway 🙂

glsignal commented 1 year ago

@karellm I've just opened https://github.com/i18next/i18next-parser/pull/900 which seems to address the warning. Would love your feedback, especially on whether it's an appropriate way to solve the issue (I'm not that jazzed about passing around the node, but didn't see another way)

karellm commented 1 year ago

Fixed as of 8.8.0