-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix react merged types #1606
Fix react merged types #1606
Conversation
|
||
declare module 'react' { | ||
interface HTMLAttributes<T> { | ||
children?: ReactI18NextChild | Iterable<ReactI18NextChild>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why and when we did that, but it doesn't make much sense, since ReactNode
is already Iterable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originally introduced with #1531 by @gshokanov to address #1506
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2c544f5
to
11bfe43
Compare
@@ -39,11 +39,12 @@ declare module 'i18next' { | |||
type ObjectOrNever = TypeOptions['allowObjectInHTMLChildren'] extends true | |||
? Record<string, unknown> | |||
: never; | |||
type ReactI18NextChild = React.ReactNode | ObjectOrNever; | |||
|
|||
type ReactI18NextChildren = React.ReactNode | ObjectOrNever; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, children will behave exactly like the actual one unless allowObjectInHTMLChildren
is true
.
@adrai, yes patch version |
included in v12.1.5 |
Was this change overridden in the latest release (13.2.0)?
|
@YutaMoriJP don't think so... correct @nicegamer7 ? |
It shouldn't have been affected, no. Though, at the company I'm working at, we had issues with this option already. I'll create a PR with the fix I implemented. I didn't open this PR previously because I didn't see anyone else complaining about this issue. |
@nicegamer7 Oh gosh yes please 🙏 |
@nicegamer7 Never mind you already created a PR and it got merged. You're fast! Thank you! |
Closes #1543
Checklist
npm run test
Checklist (for documentation change)