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

Add ComponentProps reference as first React Types example #626

Merged
merged 4 commits into from
May 30, 2023

Conversation

filiptammergard
Copy link
Collaborator

This is a visual example of what I'm envisioning in #625.

Having dedicated reference pages for each important React type with explanations and usage examples could probably be very useful.

I haven't been documenting types this way before so I'm not sure what a good way is. Exploring one way here. Happy to receive feedback!

What do you think about the overall idea here @eps1lon?

@filiptammergard
Copy link
Collaborator Author

filiptammergard commented May 19, 2023

Here's the first page:

image

@filiptammergard
Copy link
Collaborator Author

filiptammergard commented May 19, 2023

Here's the second page:

image

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Thank you for leading this! Definitely important work.

I think we currently mention somewhere that you should prefer *WithRef or *WithoutRef but I don't know why. Could you double check we have a consistent messaging between this cheatsheet and @types/react

docs/react-types/ComponentProps.md Outdated Show resolved Hide resolved
docs/react-types/ComponentProps.md Outdated Show resolved Hide resolved
docs/react-types/ComponentProps.md Outdated Show resolved Hide resolved
@filiptammergard
Copy link
Collaborator Author

Great feedback @eps1lon! I made some updates now.

Added a note similar to the note from @types/react that ComponentPropsWithRef and ComponentPropsWithoutRef should be preferred. Note sure exactly when ComponentProps actually would break, though. I have never stumbled upon a case where ComponentProps could not be used – but it might just be a matter of explicitness.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I love the direction this is going. Finally getting API docs for React types. There's one naming confusion we need to resolve.

docs/react-types/ComponentProps.md Outdated Show resolved Hide resolved

:::note

Prefer `ComponentPropsWithRef<ElementType>` if ref is forwarded and `ComponentPropsWithoutRef<ElementType>` when ref is not forwarded.
Copy link
Member

Choose a reason for hiding this comment

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

I personally always found that confusing. The whole point of ComponentProps is to get the props but at the same time I should know that it has a ref? Or does that mean ComponentPropsWithoutRef is just a shorthand for Omit<ComponentPropsWithRef<T>, 'ref'>?

Either way, this confusion already exist in the types so I'd hope we can do a follow-up with either clearer instructions or a plan to only ComponentProps and deprecate/remove the ones related to ref.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I tend to use ComponentProps despite the recommendation to prefer "being explicit about the ref", and I have never had problems with it.

I made a small experiment and found that ComponentProps probably is a better default when passing a component type, because it correctly infers whether or not ref is forwarded.

TypeScript playground

Is there a case for being explicit when passing an element string literal? Is ComponentPropsWithRef<"div"> even something you'd want to do?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer having a single ComponentProps if the only justification for ComponentPropsWithoutRef is a shorthand for ComponentProps + Omit.

Long-term we won't have forwardRef anyway and then ref will be in the props interface for function components.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have any insights into how you'll define refs in the future? Just a regular prop like this?

interface Props {
  ref: Ref
}

function Component({ ref }: Props) {
  return <div ref={ref} />
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to investigate if we can deprecate ComponentPropsWithRef and ComponentPropsWithoutRef in @types/react. Any tips on how to go about it? Make a PR with these thoughts? Someone in particular to get in touch with before taking that step?

Copy link
Member

Choose a reason for hiding this comment

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

I'd try to find out when they got introduced and why. Then deprecate and provide a counter argument to the initial rationale. By default we don't want to add stuff unless we have to. And if it is convenience there needs to be frequent usage.

@filiptammergard filiptammergard merged commit 0facfcd into main May 30, 2023
7 checks passed
@filiptammergard filiptammergard deleted the ComponentProps branch May 30, 2023 07:57
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.

None yet

2 participants