Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix the type of the components props of Trans. #1036

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

pcorpet
Copy link
Contributor

@pcorpet pcorpet commented Jan 8, 2020

No description provided.

@coveralls
Copy link

coveralls commented Jan 8, 2020

Coverage Status

Coverage remained the same at 95.261% when pulling dbbe0f2 on pcorpet:array-type into 0b734b9 on i18next:master.

@jamuhl jamuhl requested a review from rosskevin January 8, 2020 09:08
@rosskevin
Copy link
Collaborator

@pcorpet this is certainly acceptable, but the readonly modifier only affects the implementation. Since this is just a declaration without an implementation, I'm curious. Why do you think this might be a useful addition?

@pcorpet
Copy link
Contributor Author

pcorpet commented Jan 8, 2020

readonly React.ReactNode[] actually makes the array readonly not the react node. This is an indication that the Trans component has no intention of modifying the array (appending, sorting). This is useful as I have a readonly array myself that I want to set as components prop.

const homeLinkComponents = [<a href="/" />] as const

<Trans components={homeLinkComponents} i18nKey="Go <0>home</0>" />

That currently fails with:

error TS4104: The type 'readonly [Element]' is 'readonly' and cannot be assigned to the mutable type 'ReactNode[]'.

@rosskevin
Copy link
Collaborator

Ok, makes sense. I would like you to add a usage test to the test/typescript dir and push it here so that we have no regressions.

Thank you!

Copy link
Collaborator

@rosskevin rosskevin left a comment

Choose a reason for hiding this comment

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

Add a usage test to the test/typescript dir and push it here so that we have no regressions

@rosskevin rosskevin merged commit 04374c7 into i18next:master Jan 10, 2020
@rosskevin
Copy link
Collaborator

@jamuhl this can be a patch release.

@jamuhl
Copy link
Member

jamuhl commented Jan 13, 2020

published in react-i18next@11.3.0

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants