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

Removed conflicting or redundant children props #2172

Merged
merged 3 commits into from
Jan 31, 2023

Conversation

rdennis
Copy link
Contributor

@rdennis rdennis commented Jan 31, 2023

Purpose

Fixes issues with conflicting or redundant children type definitions.

Issue History

I've included some history here to explain the issue as I understand it, what has been done to address it, and what is needed to fix the outstanding issues.

Conflicting children

As mentioned in #1798, the release of react@18 removed the implicit children prop. It appears that the types-react-codemod was used which added React.PropsWithChildren<> to all components. While this works for the majority of components, there are a few (BlobProvider, PDFViewer, and PDFDownloadLink) which have more specific requirements for children. The addition of React.PropsWithChildren with the existing definitions of children for these components causes issues like #2169.

Redundant children

In #1825 a few definitions of children?: React.ReactNode were added which became redundant when React.PropsWithChildren was added to the components that use those prop types. In these cases I just removed the redundant children definition and left the React.PropsWithChildren.

Fixes #2169

@@ -215,7 +210,7 @@ declare namespace ReactPDF {
children: string;
}

class Note extends React.Component<React.PropsWithChildren<NoteProps>> {}
class Note extends React.Component<NoteProps> {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While the current type does limit children to a single string, the error message is convoluted.

No overload matches this call.
Overload 1 of 2, '(props: PropsWithChildren<NoteProps> | Readonly<PropsWithChildren<NoteProps>>): Note', gave the following error.
Type 'Element' is not assignable to type 'string & ReactNode'.
Type 'ReactElement<any, any>' is not assignable to type 'string & ReactPortal'.
Type 'ReactElement<any, any>' is not assignable to type 'string'.
Overload 2 of 2, '(props: PropsWithChildren<NoteProps>, context: any): Note', gave the following error.
Type 'Element' is not assignable to type 'string & ReactNode'.

vs the new error

No overload matches this call.
Overload 1 of 2, '(props: NoteProps | Readonly<NoteProps>): Note', gave the following error.
Type 'Element' is not assignable to type 'string'.
Overload 2 of 2, '(props: NoteProps, context: any): Note', gave the following error.
Type 'Element' is not assignable to type 'string'.

@jeetiss
Copy link
Collaborator

jeetiss commented Jan 31, 2023

@rdennis thanks for your contribution! Could you please remove React.PropsWithChildren type for Canvas and Image components because they don't have children?

and one other thing: Could you please run yarn changeset to generate a change, that will be used to release this patch?

@rdennis
Copy link
Contributor Author

rdennis commented Jan 31, 2023

@jeetiss

@rdennis thanks for your contribution! Could you please remove React.PropsWithChildren type for Canvas and Image components because they don't have children?

I think that's a very good idea, but it does cause a possibly annoying issue where even a single whitespace will cause an error.

{/* Error */}
<Image // Property 'children' does not exist on type 'IntrinsicAttributes & (IntrinsicClassAttributes<Image> & Readonly<ImageProps>)'.
    source={''}>
</Image>
<Image // Property 'children' does not exist on type 'IntrinsicAttributes & (IntrinsicClassAttributes<Image> & Readonly<ImageProps>)'.
    source={''}> </Image>
{/*             ^ Notice the space */}

{/* Valid */}
<Image
    source={''}></Image>
<Image
    source={''} />

That is probably reasonable, but could be annoying for consumers who didn't use self-closing tags for Image or Canvas. I have a commit ready to push up if that change is acceptable. I have pushed the change, but can roll it back if that is best.

and one other thing: Could you please run yarn changeset to generate a change, that will be used to release this patch?

Should I run this with every commit, or just once with the final changes? I didn't see anything about it in the contributing doc. I think I understand now from looking at past merges.

@jeetiss jeetiss merged commit d0cc0bd into diegomura:master Jan 31, 2023
@jeetiss
Copy link
Collaborator

jeetiss commented Jan 31, 2023

Thanks!

@rdennis rdennis deleted the ts-children-patch branch February 1, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlobProvider type error TS2769
2 participants