feat: add no-t-call-in-react-component

[Jack-Works]

I added a new rule: No t call in a React Component.

Background

There are two macros for this: t and <Trans>. The prior one is used outside of a React Component. Developers should not use t because it will not reflect React Provider changes (e.g. when the locale changes).

Wrong code:

function Component() {
  const x = useY();
  return <p>
    {t`Hello`}
  </p>;
}

Autofix (1):

import { useLingui } from '@lingui/react'
import { msg } from '@lingui/macro'
function Component() {
  const { _ } = useLingui();
  const x = useY();
  return <p>
    {_(msg`Hello`)}
  </p>;
}

Autofix (2):

This fix requires type-aware lint and only applies when the context accepts a React.ReactNode.

import { Trans } from '@lingui/macro'
function Component() {
  const x = useY();
  return <p>
    <Trans>Hello</Trans>
  </p>;
}

[andrii-bodnar]

Hi, thanks for the contribution!

In Lingui v5, this may be less relevant due to the new useLingui macro which allows to use the t macro inside React components safely.

[codecov]

Codecov Report

Attention: Patch coverage is 87.64706% with 21 lines in your changes missing coverage. Please review.

Project coverage is 93.64%. Comparing base (32c823c) to head (4b8bff2).

Files with missing lines	Patch %	Lines
src/rules/no-t-call-in-react-component.ts	87.11%	18 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   96.49%   93.64%   -2.86%     
==========================================
  Files          10       11       +1     
  Lines         371      535     +164     
  Branches      101      184      +83     
==========================================
+ Hits          358      501     +143     
- Misses         13       31      +18     
- Partials        0        3       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Jack-Works]

Hi, thanks for the contribution!

In Lingui v5, this may be less relevant due to the new useLingui macro which allows to use the t macro inside React components safely.

Hi! Happy to know that. When will v5 come out?

[timofei-iatsenko]

Thanks for contribution, this is a valuable addition. Especially autofixes, which is bringing DX on the next level.

As Andrii mentioned there is upcoming v5 release which is on finalizing step right now. So it would be released soon (we don'y have an exact date)

The v5 release make autofixes in this rule outdated, as well as detection logic become more complex.

Basically

Autofix (1):

import { useLingui } from '@lingui/react'
import { msg } from '@lingui/macro'
function Component() {
  const { _ } = useLingui();
  const x = useY();
  return <p>
    {_(msg`Hello`)}
  </p>;
}

Should be

import { useLingui } from '@lingui/react/macro'

function Component() {
  const { t } = useLingui();
  const x = useY();
  return <p>
    {t`Hello`)}
  </p>;
}

Because with v5 t`Hello!` is valid when it's exported from useLingui hook, the rule should check import of the t as well.

Another notice, in v5, macro imports would be changed to:

@lingui/macro -> @lingui/core/macro and @lingui/react/macro

Here is a full migration guide https://js-lingui-git-next-crowdin.vercel.app/releases/migration-5

---

I actually also thought about this issue with t, and stopped on the question: What is the valid usecase for the global t in modern applications?

You could not use it in the React components because they will not react to the language changes, you could not use it on the module level for the same reason. If you create a function on the module level with t and will try to use it in the React component, it again will not reflect the locale changes.

So I came to conclusion that there should be a rule which is blocking t usage completely, because in most of the cases usage of global t will lead to hard to catch issues.

[timofei-iatsenko]

I checked the code:

This fix requires type-aware lint and only applies when the context accepts a React.ReactNode.

it looks very complicated, have you considered to use only Nodes information for that? That probably would be less powerful, but still will cover 85% of cases i could imagine and will not require type information.

For example, we can check if the incorrect node is a direct child of JSXElement, so it will cover

<div>{t`Ola!`}</div>

[timofei-iatsenko]

fix1 -> fixAsHook
fix2 -> fixAsJsx

[timofei-iatsenko]

This is cool, it irritated me but i didn't find a way to infer a type automatically

[timofei-iatsenko]

This query is not complete. There are different signatures of t macro which should be also covered by this rule. Check LinguiTaggedTemplateExpressionMessageQuery versus LinguiCallExpressionMessageQuery in the helpers.ts

[timofei-iatsenko]

There are a little type guard helpers in the helpers.ts, they make code a little bit cleaner.

template.type !== AST_NODE_TYPES.Literal

!isLiteral(template)

You can use it and extend with your own, because there is not a full set.

[timofei-iatsenko]

There are many uncovered lines, could you please write tests that will cover all of them.

[Jack-Works]

Hi! Thanks for the review, before resolving any issue, I wonder if we still need this lint in v5? If not, I prefer to close this PR (or we can hold it until v5 is finally out).

[Jack-Works]

I checked the code:
This fix requires type-aware lint and only applies when the context accepts a React.ReactNode.
It looks very complicated

The type-aware fix is optional, I also tested for cases that do not have type-aware lint. The code was from our migration code from another i18n framework to lingui (https://github.com/DimensionDev/Maskbook/blob/732f352d09eddf8df791c6cc1f8e91178b1b75cd/packages/scripts/src/locale-kit-next/migrate.ts) it works well and can detect most of our cases like <Label title={...} /> where title accepts a React.ReactNode.

[timofei-iatsenko]

I wonder if we still need this lint in v5? If not, I prefer to close this PR (or we can hold it until v5 is finally out).

Yes, why not? v5 just adds a new syntax sugar, the all old ones are there, so problems related to them as well.

Regarding type-aware checking, there is a recommendation from the typescript-eslint team

We recommend against changing rule logic based solely on whether services.program exists. In our experience, users are generally surprised when rules behave differently with or without type information. Additionally, if they misconfigure their ESLint config, they may not realize why the rule started behaving differently. Consider either gating type checking behind an explicit option for the rule or creating two versions of the rule instead.

https://typescript-eslint.io/developers/custom-rules#conditional-type-information

[Jack-Works]

Regarding type-aware checking, there is a recommendation from the typescript-eslint team

The lint itself does not base on type information, it only provides one more suggestion (<Trans>) if type information is available. Suggestions cannot be automatically applied (rather than fixes), so do you still want to follow the typescript-eslint team's guide that gate type checking behind an explicit option for the rule or create two versions of the rule instead? I can do that but I think it is not necessary.

Yes, why not? v5 just adds a new syntax sugar, the all old ones are there, so problems related to them as well.

How do you think we should support v5? By auto-detecting the dependency version, or providing a configuration?