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

Cannot pass objects to <Trans> with React 18 type definitions #1483

Closed
LukasKalbertodt opened this issue Apr 11, 2022 · 38 comments · Fixed by #1486
Closed

Cannot pass objects to <Trans> with React 18 type definitions #1483

LukasKalbertodt opened this issue Apr 11, 2022 · 38 comments · Fixed by #1486
Assignees

Comments

@LukasKalbertodt
Copy link

LukasKalbertodt commented Apr 11, 2022

🐛 Bug Report

With version 18 of @types/react, Typescript complains when passing objects to the Trans component.

It previously worked because ReactNode had a | {} in its definition, allowing arbitrary objects to be passed. That has been removed (for a good reason) in version 18 of the type definitions. See this PR for more information.

To Reproduce

From this example:

<Trans i18nKey="userMessagesUnread" count={count}>
  Hello <strong title={t('nameTitle')}>{{name}}</strong>, you have {{count}} unread message. <Link to="/msgs">Go to messages</Link>.
</Trans>

This fails type checking: TS2322: Type '{ name: void; }' is not assignable to type 'ReactNode'.

Expected behavior

Type checking works, i.e. the TS compiler does not complain.

I would think that {} (or something like Record<string, unknown>) should be explicitly added to the children type of Trans.

Your Environment

  • react version: 18.0.0
  • i18next version: 11.16.5
  • TS version: 4.6.3
@adrai
Copy link
Member

adrai commented Apr 11, 2022

@high1 maybe related?

@high1
Copy link
Contributor

high1 commented Apr 14, 2022

@adrai - React did remove implicit children on their components in @types/react18, as @LukasKalbertodt aleady said, so I would expect something like this to happen - if this is children related, Trans component just needs to allow children explicitly.
DefinitelyTyped/DefinitelyTyped#56210

@high1
Copy link
Contributor

high1 commented Apr 16, 2022

I took a look at this today. @LukasKalbertodt is 100% on the point - another breaking change in React 18 typings is that objects are removed from ReactNode. And the suggested solution should work - but if React.ReactNode | Record<string, unknown> gets added to definitions, another null-undefined union error happens. I would disable this rule at this point, if that's fine, @adrai - and update tslint to it's latest version, 6.1.3. And then I would suggest that the repo upgrades to @typescript-eslint.

@adrai
Copy link
Member

adrai commented Apr 16, 2022

need @pedrodurek's feedback here 🙏

@LukasKalbertodt
Copy link
Author

This is not yet fixed. The code in the top level comment now (11.16.7) results in:

ERROR 
TS2322: Type '{ children: (string | Element | { count: number; })[]; i18nKey: string; count: number; }' is not assignable to type '{ children?: Record<string, unknown> | ReactNode; components?: readonly ReactNode[] | { readonly [tagName: string]: ReactNode; } | undefined; count?: number | undefined; ... 9 more ...; t?: TFunction<...> | undefined; }'.
  Types of property 'children' are incompatible.
    Type '(string | Element | { count: number; })[]' is not assignable to type 'Record<string, unknown> | ReactNode'.
      Type '(string | Element | { count: number; })[]' is not assignable to type 'Record<string, unknown>'.
        Index signature for type 'string' is missing in type '(string | Element | { count: number; })[]'.

ERROR 
TS2322: Type '{ name: string; }' is not assignable to type 'ReactNode'.
  Object literal may only specify known properties, and 'name' does not exist in type 'ReactElement<any, string | JSXElementConstructor<any>> | ReactFragment | ReactPortal'.

The linked PR allowed passing one object as children, but not a list of different objects mixed with strings or nodes. All of these use cases were "supported" via {}, which, as far as I know, accepts a whole lot of types. That's why using {} is generally discouraged.

For the first error, I think this should work:

type TransChild = React.ReactNode | Record<string, unknown>;

export type TransProps<...> = {
  children?: TransChild | TransChild[];
  ...
};

However, this doesn't fix the second error. And I'm not sure we can fix it? Here it's about the children of strong, which is defined in React itself.

@adrai adrai reopened this Apr 19, 2022
LukasKalbertodt added a commit to LukasKalbertodt/tobira that referenced this issue Apr 19, 2022
This came with a few breaking changes. See this for the full
explanation: DefinitelyTyped/DefinitelyTyped#56210

The most impactful for us is that `React.FC` does not have implicit
`children` anymore. So all components that use children, have to declare
them explicitly. This is a lot of changes, but a net win for type
safety.

Also, due to some changes, `TResult` (what `t()` returns) was not
assignable to `ReactNode`. This was fixed in `react-i18next`. Thus,
that package is also updated.

Further, `{}` was removed from `ReactNode`, which made some `Trans`
usages not compile anymore. I opened this issue:
i18next/react-i18next#1483
This is not completely fixed yet, but by just changing one line, we can
make it compile already.
@stale
Copy link

stale bot commented Apr 27, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 27, 2022
@LukasKalbertodt
Copy link
Author

This is still a problem. Please don't close.
(I'm sorry but I can't think of a single reason why stalebot would be useful...)

@stale stale bot removed the stale label Apr 27, 2022
@adrai
Copy link
Member

adrai commented Apr 27, 2022

@pedrodurek Do you have some feedback?

@pedrodurek
Copy link
Member

I'll be investigating this one today

@pedrodurek
Copy link
Member

pedrodurek commented Apr 28, 2022

Sorry for the delay guys!
We can change the TransProps to also accept array of object and ReactNode, but that doesn't solve the problem entirely, HTML element children are no longer accepting objects, which means this will not work:

<Trans i18nKey="userMessagesUnread" count={count}>
  Hello <strong>{{name}}</strong>
</Trans>

So strong element doesn't "accept" objects anymore.

That's not something that we can't control in "theory"... We can always use type augmentation to change the HTMLAttributes children prop to also accept objects. The only problem is that all HTML elements will start accepting objects again, not only the HTML elements wrapped within Trans component.

Anyway, I created a draft PR #1492 that solves it. If you accept the condition above, we can merge it. I want you to hear your thoughts first!

@eps1lon
Copy link

eps1lon commented Apr 28, 2022

HTML element children are no longer accepting objects, which means this will not work:

To be clear: Objects were never accepted by the runtime. You would've seen an error at runtime. Objects were unintentionally accepted by the types which we fixed in the React 18 types.

We can always use type augmentation to change the HTMLAttributes children prop to also accept objects.

Note that this would affect every code in an application if they import from your library. Since accepting objects is unsound, I would advise against this.

Maybe you're using a different JSX factory that somehow transforms these objects? In this case I don't have a good solution.

@pedrodurek
Copy link
Member

pedrodurek commented Apr 28, 2022

Note that this would affect every code in an application if they import from your library.

That's my biggest concern.

Maybe you're using a different JSX factory that somehow transforms these objects? In this case I don't have a good solution.

Hey @adrai, what's your thoughts?

@adrai
Copy link
Member

adrai commented Apr 28, 2022

Sorry, I've no idea... I'm not a TypeScript user... maybe html-parse-stringify => https://github.com/i18next/react-i18next/blob/master/src/Trans.js#L136 is influencing this?
fyi: I can't see any warning at runtime... https://codesandbox.io/s/react-i18next-forked-nk3fcc?file=/src/app.js:810-1014 maybe only when using TypeScript?

@eps1lon
Copy link

eps1lon commented Apr 28, 2022

I can't see any warning at runtime... codesandbox.io/s/react-i18next-forked-nk3fcc?file=/src/app.js:810-1014 maybe only when using TypeScript?

Trans seems to me ensuring that no objects are actually passed. If you just render(<div>{{ name: 'test' }}</div>) you'll see the error.

@jamuhl
Copy link
Member

jamuhl commented Apr 28, 2022

It's not a runtime issue...as (not like typescript) javascript is just code ;)...react's JSX is nothing but a stack of function calls...the JSX element's children is just an array of objects. So we take that {{notAllowedObjectInChildren}} and make what we need with it -> parse it to interpolation values.

That way we can use i18next JSON interpolation syntax inside JSX elements...it's somewhat clever but has its drawbacks with over-restrictive typescript...if it's good or bad thing might depend on the user.

If typescript users prefer not extending this...the other option is to not allow using Trans in that way -> but use it like for ICU syntax https://react.i18next.com/misc/using-with-icu-format#using-the-trans-component

@pedrodurek
Copy link
Member

pedrodurek commented Apr 28, 2022

Alternatively, we can add a new property allowObjectInHTMLChildren to TypeOptions, so it can be up to each user to toggle it on/off (default value false)... at their own risk. I updated the draft so you can see it https://github.com/i18next/react-i18next/pull/1492/files#diff-61782f9118a033e04ba53dd09fb1e38d10dc39d2dfb9f8474f3dad935f16ea21R65-R109

@adrai
Copy link
Member

adrai commented Apr 28, 2022

Alternatively, we can add a new property allowObjectInHTMLChildren to TypeOptions, so it can be up to each user to toggle it on/off (default value false)... at their own risk. I updated the draft so you can see it https://github.com/i18next/react-i18next/pull/1492/files#diff-61782f9118a033e04ba53dd09fb1e38d10dc39d2dfb9f8474f3dad935f16ea21R65-R109

Is this something that could work for you @LukasKalbertodt and other TypeScript users?

@adrai
Copy link
Member

adrai commented Apr 30, 2022

@high1 @enoh-barbu what's your opinion?

@enoh-barbu
Copy link

enoh-barbu commented May 2, 2022

I think there is no other simpler solution to this without chaning the current API. Passing object to JSX is not a common procedure indeed, but in this context we can live with this as it's simpler to read the code and understand what's the intention of that object. Otherwise a change in the API will be needed, eventaully passing more props to <Trans />

tl;dr : yes, I'm fine with extra allowObjectInHTMLChildren

@adrai
Copy link
Member

adrai commented May 2, 2022

@nickclyde what about you?

@adrai
Copy link
Member

adrai commented May 5, 2022

v11.16.8 offers allowObjectInHTMLChildren option

@rphmee-lo
Copy link

rphmee-lo commented Aug 5, 2022

Came across this issue as well, here is how I resolved it:

<Trans
    ns="NAMESPACE"
    i18nKey="KEY"
    values={{ lower, upper }}
    components={{ 1: <strong />, 3: <strong /> }}
  >
    {`Consider <1>{{lower}}</1> to <3>{{upper}}</3> magnification.`}
  </Trans>

With the translation using the same <1></1> and <3></3>.

@ghost
Copy link

ghost commented Aug 12, 2022

For anyone stumbling across this in the future and needs to figure out how to use the allowObjectInHTMLChildren option, you just have to create a type declaration file like src/types/react-i18next.d.ts and then add the following to it

import 'react-i18next';

declare module 'react-i18next' {
  interface CustomTypeOptions {
    allowObjectInHTMLChildren: true;
  }
}

@ripvannwinkler
Copy link

ripvannwinkler commented Aug 15, 2022

Using the suggested .d.ts mod from @dave-multiplier breaks all my other JSX tags with errors like:

error TS2746: This JSX tag's 'children' prop expects a single child of type 'ReactI18NextChild | Iterable<React
I18NextChild>', but multiple children were provided.

The above error is thrown for elements like:

<label htmlFor={`${id}-phone`}>
    {t('Phone Number')}
    <RequiredMark />
</label>

Wrapping {t('Phone Number')} inside a span tag fixes the problem.

@davehowson
Copy link

Apologies for that @ripvannwinkler
I am facing the same issue right now and is awaiting a response here on #1543.
You can follow that thread for any updates

okudayuuki added a commit to okudayuuki/react-i18n-sample that referenced this issue Aug 26, 2022
App.tsx:28のエラーは↓をちゃんと読まないといけないけど読んでない。動いてはいる。
i18next/react-i18next#1483
@joshkel
Copy link

joshkel commented Oct 5, 2022

In case it helps anyone else: Using allowObjectInHTMLChildren felt fairly drastic (since it changes the allowed prop types of all React components, whether or not they're part of <Trans>), and it isn't perfect (#1543), so I've started using typecasts instead:

<Trans i18nKey="userMessagesUnread" count={count}>
  Hello <strong title={t('nameTitle')}>{{name} as any}</strong>, you have {{count}} unread message.
</Trans>

I use a type alias to make this more self-documenting:

/**
 *  Interpolation for `t` - see https://github.com/i18next/react-i18next/issues/1483
 */
export type TI = any;
// ...

return (
  <Trans i18nKey="userMessagesUnread" count={count}>
    Hello <strong title={t('nameTitle')}>{{name} as TI}</strong>, you have {{count}} unread message.
  </Trans>
);

(This relies on new work in i18next-parser; see i18next/i18next-parser#603.)

@seemX17
Copy link

seemX17 commented Feb 28, 2023

This issue still exists even with 12.2.0 , I'm hoping there will be a fix for this. Making use of any is not the workaround I want to go for

nbudin added a commit to neinteractiveliterature/intercode that referenced this issue Mar 15, 2023
@wangpin34
Copy link

wangpin34 commented Jul 10, 2023

Upgraded to v13.0.1 (other i18next pkgs were upgraded as well) and the error was gone.

@lgenzelis
Copy link

For anyone landing here today, this worked fine for me:

    <Trans
      t={t}
      values={{ minutesToLogout, secondsToLogout }}
      components={[<span className={minutesToLogout === 0 ? styles.danger : ''} key="dummy-key" />]}
    >
      {'You will be signed out in <0>{{ minutesToLogout }}m {{ secondsToLogout }}s</0>'}
    </Trans>

where the key is automatically detected as 'You will be signed out in <0>{{ minutesToLogout }}m {{ secondsToLogout }}s</0>'

@AsuraKev
Copy link

Upgraded to v13.0.1 (other i18next pkgs were upgraded as well) and the error was gone.

Doesnt work man, error is still there

@KakakuCZ
Copy link

The solution is:

  • create new i18next.d.ts file
  • declare new module and "override" CustomTypeOptionsType
// src/@types/i18next.d.ts

declare module 'i18next' {
	interface CustomTypeOptions {
		allowObjectInHTMLChildren: true
	}
}

@pmioduszewski
Copy link

The solution is:

  • create new i18next.d.ts file
  • declare new module and "override" CustomTypeOptionsType
// src/@types/i18next.d.ts

declare module 'i18next' {
	interface CustomTypeOptions {
		allowObjectInHTMLChildren: true
	}
}

This works but also breaks other types; I've noticed that the t type from useTranslation breaks.

image

@goorm-philip
Copy link

The solution is:

  • create new i18next.d.ts file
  • declare new module and "override" CustomTypeOptionsType
// src/@types/i18next.d.ts

declare module 'i18next' {
	interface CustomTypeOptions {
		allowObjectInHTMLChildren: true
	}
}

While this solution resolves my current issue, it inadvertently permits objects as valid children types in other contexts.
I'm concerned that this may lead to issues in the future.

Are there alternative solutions that don't involve altering the JSX children type?

@joshkel
Copy link

joshkel commented Feb 19, 2024

@goorm-philip, as I understand it, the only two solutions possible (given i18next's design and React's type definitions) are to loosen React's type definitions (using allowObjectInHTMLChildren) or use a typecast (my preference, for the reasons you gave).

@inkognitro
Copy link

inkognitro commented Sep 5, 2024

For everyone still facing problems due to the breaking ReactNode type change, you could change the usage of the Trans component to something like so:

<Trans
   i18nKey="foo"
   t={t}
   components={{
      Bar: <strong>bar</strong>,
   }}
/>

And the according translation with the foo key:
This is a translated text with strong <Bar /> in it.

References: https://react.i18next.com/latest/trans-component#overriding-react-component-props-v11.5.0

@literalpie
Copy link

I have another idea for a workaround that I think is better than the others suggested so far

  • create a helper function: export const transParameters = (s: Record<string, unknown>) => s as unknown as ReactNode;
  • add this to your parser lexers config: transIdentityFunctionsToIgnore: ['transParameters']
  • <Trans><div>{transParameters({ myValue })}</div></Trans>

It should also be possible to make a linting rule to ensure that the helper is only used as a descendant of Trans

@Jme797
Copy link

Jme797 commented Sep 24, 2024

This doesn't work with React 18
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet