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.

Sign up for GitHub

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

Jump to bottom

fix(types): Add explicit types for children #1652

Closed
wants to merge 1 commit into from
Closed

fix(types): Add explicit types for children #1652

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export interface ClipPathProps {
}
export const ClipPath: React.ComponentClass<ClipPathProps>;

export const Defs: React.ComponentClass<{}>;
export const Defs: React.ComponentClass<{ children?: React.ReactNode }>;

export interface EllipseProps extends CommonPathProps {
cx?: NumberProp,
Expand Down Expand Up @@ -200,6 +200,7 @@ export interface LineProps extends CommonPathProps {
export const Line: React.ComponentClass<LineProps>;

export interface LinearGradientProps {
children?: React.ReactNode,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this PR concern other interfaces, which accept children as well? Or is there something special about LinearGradientProps?

Copy link
Author

@eps1lon eps1lon Mar 2, 2022 •
edited

Choose a reason for hiding this comment

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

This was the only interface that needed it. Other componens are either not tested for children or they may not even support them.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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[].

x1?: NumberProp,
x2?: NumberProp,
y1?: NumberProp,
Expand Down