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

Inaccurate type definitions for Button variant prop #1956

Closed
rezrah opened this issue Mar 14, 2022 · 4 comments · Fixed by #2445
Closed

Inaccurate type definitions for Button variant prop #1956

rezrah opened this issue Mar 14, 2022 · 4 comments · Fixed by #2445
Labels
bug Something isn't working react

Comments

@rezrah
Copy link
Contributor

rezrah commented Mar 14, 2022

Describe the bug
Type definitions for Button are not being exported correctly in the transpiled output.

E.g:

variant is being reported as a string type, rather than the VariantType union it should report.

This may be happening with other props, but hasn't been verified.

Due to this incorrect typing, the migration from the v34 Button to v35 becomes challenging as the v34 Button also had a variant prop that was used for size, so the TypeScript compiler assumes the size was valid and shows the following error (rather than one related to invalid discriminated unions).

<Button
    variant="small"
    onClick={() => onCreate(minimumStartDate, defaultDuration)}
    leadingIcon={PlusIcon}
    {...testIdProps('add-default-iteration-button')}
  >
    Add iteration
  </Button>

leads to this error:

Error: Uncaught [TypeError: Cannot convert undefined or null to object]

This can also be reproduced on StackBlitz:

To Reproduce
Steps to reproduce the behavior:

  1. Go to StackBlitz https://stackblitz.com/edit/primer-react-v35?file=index.tsx
  2. Hover over the variant prop on Button
  3. See error
    button props

Expected behavior
variant should show 'default' | 'primary' | 'invisible' | 'danger' | 'outline'

Additional context
Possibly related? #1948

@rezrah rezrah added bug Something isn't working react labels Mar 14, 2022
@lesliecdubs
Copy link
Member

👋🏻 @pksjce we're doing some board cleanup and wanted to check in on this one. Is it on your radar / still in progress?

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2022

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 9, 2022
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
@jclem
Copy link
Contributor

jclem commented Oct 17, 2022

This affects all props on Button other than the generic indexed properties for string, number and symbol. None of the other props are type-checked or offered up by language server assistance.

@mattcosta7
Copy link
Collaborator

I believe #2445 fixes this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working react
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants