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

allow iterable with objects as children #1531

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

gshokanov
Copy link
Contributor

This PR fixes TypeScript error which prevents multiple nodes to be used inside core React elements when allowObjectInHTMLChildren is set to true.

Code example that fails on the latest published version with React 18 but works correctly with this PR:

<Trans t={t}>
  View <b>{{ count }} cards</b>
</Trans>

Resolves #1506

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided
  • commit message and code follows the Developer's Certification of Origin

@coveralls
Copy link

coveralls commented Jul 5, 2022

Coverage Status

Coverage remained the same at 95.862% when pulling 4e045a0 on gshokanov:update-object-as-children into 2759ed2 on i18next:master.

@adrai adrai requested a review from pedrodurek July 5, 2022 15:13
@gshokanov gshokanov force-pushed the update-object-as-children branch from 97f92a0 to c5b3c2b Compare July 6, 2022 10:48
@@ -100,7 +100,7 @@ declare module 'i18next' {
}

type ObjectOrNever = TypeOptions['allowObjectInHTMLChildren'] extends true
? Record<string, unknown>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be cleaner:

type HTMLChild = ReactNode | ObjectOrNever;
declare module 'react' {
  interface HTMLAttributes<T> {
    children?: HTMLChild | Iterable<HTMLChild>;
  }
}

I've used Iterable because that's what react itself uses for ReactNode's circular reference, and given the type a specific name so that ObjectOrNever still actually represents ObjectOrNever.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, updated MR. Thx for the suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly enough Iterable does not work correctly when combined with t function, but Array works as expected 🤔
Here's a code example:

<div>
    {t('hello')}
    <span>Hi</span>
</div>

I get a compilation error on outer div

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either disabling allowObjectInHTML or changing Iterable to Array fixes it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is:

This JSX tag's 'children' prop expects a single child of type 'ReactI18NextChild | Iterable<ReactI18NextChild>', but multiple children were provided.ts(2746)

typescript will now allow core react HTML tags to accept multiple nodes
mixed with translation objects as children
@gshokanov gshokanov force-pushed the update-object-as-children branch from c5b3c2b to 4e045a0 Compare July 7, 2022 15:48
Copy link
Member

@pedrodurek pedrodurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thanks!

@adrai adrai merged commit 3cf7230 into i18next:master Jul 13, 2022
@adrai
Copy link
Member

adrai commented Jul 13, 2022

included in v11.18.1

@adrai adrai mentioned this pull request Feb 2, 2023
7 tasks
adrai added a commit that referenced this pull request Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript declaration does not include new allowObjectInHTMLChildren option
5 participants