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

Improve ButtonWithMenu types, add tests for Button types #49585

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

ravicious
Copy link
Member

I was working on a component that was using ButtonWithMenu. I noticed that I was able to pass an onClick function to it that had a signature which didn't match what's expected from a regular onClick.

This was the case because of the type used to describe the rest of the props: [buttonBorderProp: string]: any. This makes it so that ButtonWithMenu accepts any kind of prop and its type doesn't matter.

I made an attempt to fix that by replacing this with props: { <known props> } & ButtonProps<'button'>. This seemed to work well until I found a callsite which uses the forwardedAs prop:

<ButtonWithMenu
text="Log In"
width="123px"
size="small"
target="_blank"
href={samlAppSsoUrl}
rel="noreferrer"
textTransform="none"
forwardedAs="a"
title="Log in to SAML application"
>

The type that I came up with didn't want to allow the target and href props because they don't exist on <button>. So I needed to somehow make TS treat ButtonWithMenu as a when forwardedAs is passed.

After wrestling with the types for an hour, I wrote a simple test which serves two purposes. First, it checks if wrong props are rejected by TS. Second, it checks if the component under tests actually renders as <button> or <a>. This test case reproduces the problem that I ran into in ButtonWithMenu.

I found that depending on whether forwardedAs or as is needed, I need to use either ComponentPropsWithoutRef or ComponentPropsWithRef – I can't quite explain why it's the case, but this works for now and the test passes.

As for forwardedAs vs as, I believe forwardedAs is needed because when the css prop is used, it creates a wrapper component around the original styled component.

The third idea behind the test is to make it serve as a template whenever someone needs to provide a type for rest props. There's a bunch of other places with types for rest props (see rg "\[.*\]: any") which could use such change in the future.

@ravicious ravicious added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Nov 29, 2024
@ravicious ravicious requested a review from bl-nero November 29, 2024 14:57

This comment was marked as off-topic.

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from bl-nero December 2, 2024 10:41
@ravicious ravicious added this pull request to the merge queue Dec 2, 2024
Merged via the queue into master with commit 5fa4eb2 Dec 2, 2024
43 checks passed
@ravicious ravicious deleted the r7s/button-types branch December 2, 2024 11:47
@public-teleport-github-review-bot

@ravicious See the table below for backport results.

Branch Result
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants