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

Sidebar props not picked up correctly by TypeScript #5534

Closed
1 task done
robinzigmond opened this issue Oct 7, 2022 · 2 comments · Fixed by #5548
Closed
1 task done

Sidebar props not picked up correctly by TypeScript #5534

robinzigmond opened this issue Oct 7, 2022 · 2 comments · Fixed by #5548

Comments

@robinzigmond
Copy link
Contributor

Current behaviour

For the Sidebar component, the prop types are not enforced correctly by the TypeScript compiler. This can be seen on both VS Code and on Codesandbox in the codesandbox demo below. Note how:

  • the incorrect type given to disableEscKey is not flagged as an error at all by TS in the editor (although it does generate an error from PropTypes at runtime)
  • the type of all props, seen if you hover over them, is simply inferred from what is entered, rather than picked up from the actual type declaration. Eg that disableEscKey prop is thought to be a string (when it should be boolean), while open is inferred as just the literal true when it again should be boolean.
  • there is no autocompletion if you want to add another prop

Assuming this is consistent behaviour and not just a quirk of the editor (it seems to be consistent as I'm seeing the same on both VS Code and CodeSandbox - and even if it's specific to those VSC is a very common editor!), this needs to be fixed as it defeats the point of using TypeScript for this specific component.

Expected behaviour

TypeScript should check the types and flag an error for props of incorrect types, as well as show the correct declared type on hover for each prop.

CodeSandbox or Storybook URL

https://codesandbox.io/s/cool-satoshi-pdp5rp?file=/src/index.tsx

JIRA Ticket (Sage Only)

No response

Suggested Solution

No response

Carbon Version

111.1.0

Design Tokens Version

No response

What browsers are you seeing the problem on?

Chrome

What Operating System are you seeing the problem on?

MacOS

Anything else we should know?

on further investigation, I think this is due to the following in the DataProps interface, which are extended by TagProps which is in turn extended by SidebarProps (as well as other component props interfaces):

[restKeys: string]: any;

In particular, if I comment that out locally then the types on Sidebar seem to work correctly.

While other components use TagProps, Sidebar seems to be the only one that also uses forwardRef, and this does indeed seem to be the problem (due to how React itself types forwardRef). See this CSB demo for how adding [rest: string]: any to a props type causes type checking to fail when used with forwardRef, but not without.

I think the only solution is to remove the [restKeys: string]: any; and find some other way to achieve what we intended by that.

Confidentiality

  • I confirm there is no confidential or commercially sensitive information included.
@nicktitchmarsh
Copy link
Contributor

FE-5407

edleeks87 added a commit that referenced this issue Oct 12, 2022
…xported

Updates the TagProp interface to ensure only the `data-` props are exported and extended in the
publinc interfaces. A new private `RestProps` interface has been added to support passing the rest
props object into the `tags` util. The previous implementation was preventing `intellisense` from
displaying the correct types for the `Sidebar` component; instead it was being assigned `any`.

fix #5534
edleeks87 added a commit that referenced this issue Oct 12, 2022
…xported

Updates the `TagProps` interface to ensure only the `data-` props are exported and extended in the
public interfaces. A new private `RestProps` interface has been added to support passing the rest
props object into the `tags` util. The previous implementation was preventing `intellisense` from
displaying the correct types for the `Sidebar` component; instead it was being assigned `any`.

fix #5534
edleeks87 added a commit that referenced this issue Oct 13, 2022
…xported

Updates the `TagProps` interface to ensure only the `data-` props are exported and extended in the
public interfaces. A new private `RestProps` interface has been added to support passing the rest
props object into the `tags` util. The previous implementation was preventing `intellisense` from
displaying the correct types for the `Sidebar` component; instead it was being assigned `any`.

fix #5534
edleeks87 added a commit that referenced this issue Oct 17, 2022
…xported

Updates the `TagProps` interface to ensure only the `data-` props are exported and extended in the
public interfaces. A new private `RestProps` interface has been added to support passing the rest
props object into the `tags` util. The previous implementation was preventing `intellisense` from
displaying the correct types for the `Sidebar` component; instead it was being assigned `any`.

fix #5534
@robinzigmond robinzigmond added On Backlog and removed triage Triage Required labels Oct 18, 2022
edleeks87 added a commit that referenced this issue Oct 19, 2022
…xported

Updates the `TagProps` interface to ensure only the `data-` props are exported and extended in the
public interfaces. A new private `RestProps` interface has been added to support passing the rest
props object into the `tags` util. The previous implementation was preventing `intellisense` from
displaying the correct types for the `Sidebar` component; instead it was being assigned `any`.

fix #5534
carbonci pushed a commit that referenced this issue Oct 19, 2022
### [111.5.1](v111.5.0...v111.5.1) (2022-10-19)

### Bug Fixes

* **tags:** update TagProps interface so only the data tag types are exported ([e6d14ba](e6d14ba)), closes [#5534](#5534)
@carbonci
Copy link
Collaborator

🎉 This issue has been resolved in version 111.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants