fix(types): Add explicit types for children #1652
Conversation
| @@ -200,6 +200,7 @@ export interface LineProps extends CommonPathProps { | |||
| export const Line: React.ComponentClass<LineProps>; | |||
|
|
|||
| export interface LinearGradientProps { | |||
| children?: React.ReactNode, | |||
There was a problem hiding this comment.
Shouldn't this PR concern other interfaces, which accept children as well? Or is there something special about LinearGradientProps?
There was a problem hiding this comment.
This was the only interface that needed it. Other componens are either not tested for children or they may not even support them.
There was a problem hiding this comment.
What about RadialGradientProps, which then uses the same method which alters the children and right now seems to accept ReactElement[] instead of ReactNode: https://github.com/react-native-svg/react-native-svg/blob/cf808ae79db788abb7d2c03875a33920df71a3d6/src/elements/RadialGradient.tsx#L44. I am concerned about it since I made a PR with removing the index.d.ts files so the types are the same as used in component files, and it is one of the issues I encountered (see e.g. https://github.com/react-native-svg/react-native-svg/pull/1708/files#diff-1981d696a687c364c10179618dc5edf0feb68bbfa395f99c59b53e289845eec0R299 where I had to provide such type for children in order for the correct input to the extractGradient function).
There was a problem hiding this comment.
If there's a concern, please open a PR that adds new tests. Then I can check if we need to do anything to work with no implicit children.
The changes in this PR are the only changes required to make the current tests pass when working without implicit children. If the implementation relies on children then tests should reflect that.
There was a problem hiding this comment.
I just realized that this is not a DT repo.
I'm not sure I understand what the issue is here. If there's more there needs to be done, why does this block this particular PR?
There was a problem hiding this comment.
I was just curious why there was a change for this particular component, but if it is for the tests in DT repo, then it makes sense. Could you also point to the test files concerning this change? I think it could be merged, but I think it would be consistent if ReactNode in LinearGradientProps was changed to ReactElement[].
|
Don't actually need this anymore it seems like. |
Summary
We plan to remove implicit types for
childrenin@types/react@18.x. However, some of your components rely on implicit children.Would greatly appreciate a release of 6.x so that we don't have to inline the types in the DefinitelyTyped repo
Test Plan
Changes make the relevant failures in DefinitelyTyped/DefinitelyTyped#56210 disappear
What's required for testing (prerequisites)?
npm run test-allwith changes from this PR in DefinitelyTyped/DefinitelyTyped#56210What are the steps to reproduce (after prerequisites)?
npm run test-allin DefinitelyTyped/DefinitelyTyped#56210Compatibility
Checklist
README.md__tests__folder