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

[Feature Request]: Export props for TypeScript components #16154

Closed
27 tasks done
Sam-Wool opened this issue Apr 9, 2024 · 12 comments
Closed
27 tasks done

[Feature Request]: Export props for TypeScript components #16154

Sam-Wool opened this issue Apr 9, 2024 · 12 comments
Labels
area: typescript good first issue 👋 Used by GitHub to elevate contribution opportunities needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. package: @carbon/react @carbon/react planning: umbrella Umbrella issues, surfaced in Projects views proposal: accepted This request has gone through triaging and we are accepting PR's against it. status: help wanted 👐 type: enhancement 💡
Milestone

Comments

@Sam-Wool
Copy link
Contributor

Sam-Wool commented Apr 9, 2024

The problem

In the @carbon/react project multiple components which have been converted into TypeScript in Carbon 11 are lacking either exports on their props,. Without access to the props, it is much more difficult to create wrappers for components, pass along props as part of parent components, and assigning types to objects that can help build out components neatly. This has affected my team's efforts to move over from Carbon 10 to 11.

For example, in my team's product we leverage the base component's props (aliased here as CarbonAccordionProps) to create the typings for a wrapper for the Accordion component with a simple overwrite. Without these props, we would have to redefine all of the parameters in our codebase by hand, which would open us up to issues down the road if the Carbon component's props change.

image

Included here is a list of all props that are missing exports that my team have found. Note that there may be additional components with this issue that we haven't identified yet.

Tasks

Preview Give feedback
  1. component: accordion severity: 3
    riddhybansal
  2. component: overflow-menu severity: 3
    Gururajj77
  3. component: structured-list severity: 3
    riddhybansal
  4. component: button severity: 3
    riddhybansal
  5. component: button severity: 3
    riddhybansal
  6. severity: 3
    riddhybansal
  7. component: dropdown severity: 3
    riddhybansal
  8. component: form severity: 3
    riddhybansal
  9. component: grid severity: 3
    riddhybansal
  10. component: loading severity: 3
    Gururajj77
  11. component: loading severity: 3
    Gururajj77
  12. component: modal severity: 3
    Gururajj77
  13. component: number-input severity: 3
    Gururajj77
  14. component: progress-bar component: progress-indicator severity: 3
    Gururajj77
  15. area: typescript component: content-switcher severity: 3 type: bug 🐛
    Gururajj77
  16. component: tabs severity: 3
    Gururajj77
  17. component: text-input severity: 3
    Gururajj77
  18. component: text-input severity: 3
  19. component: tooltip severity: 3
    Gururajj77
  20. component: list component: tile severity: 3
    Gururajj77

We've also noticed some components are missing an index.ts or .d.ts file - this has caused a similar issue when trying to import props belonging to that component. Below is a list of the components we've found with this issue:

Tasks

Preview Give feedback
  1. component: password-input severity: 3
  2. component: tile severity: 3
    Gururajj77
  3. component: overflow-menu severity: 3
    Gururajj77
  4. Gururajj77

The solution

To fix this, we need to export the props (see attached for example of an interface lacking an export).

image

There are also some instances where a prop is missing an index.ts or .d.ts file - in those cases, we should be able to fix this issue by refactoring an existing index.js to index.ts. See below for an example of a healthy index.ts file.

image

Examples

No response

Application/PAL

IBM Storage Fusion

Business priority

Medium Priority = upcoming release but is not pressing

Available extra resources

No response

Code of Conduct

Copy link
Contributor

github-actions bot commented Apr 9, 2024

Thank you for submitting a feature request. Your proposal is open and will soon be triaged by the Carbon team.

If your proposal is accepted and the Carbon team has bandwidth they will take on the issue, or else request you or other volunteers from the community to work on this issue.

@tay1orjones
Copy link
Member

tay1orjones commented Apr 10, 2024

Thanks for opening this, I think it's worthwhile to consider exporting these. I'm not sure if there's any downsides, but if I'm not mistaken, consumers have always been able to pluck proptypes off the exported components. Makes sense to enable the same through TypeScript.

This would be an excellent opportunity for a contribution from the community.

There are also some instances where a prop is missing an index.ts or .d.ts file

Majority of these cases are due to a sibling component not yet being migrated. Once all exports in the component(s) index.js have been migrated, index.js can be renamed to index.ts and then the .d.ts file will start being automatically generated.

If you notice any that have all their exports migrated, but it's still index.js, you can open a PR or an issue to track those individually. We have a couple of these issues already. Also all this should be fully resolved once everything has types, #12513, which we're really close to completing.

@tay1orjones tay1orjones moved this from Triage to Ready for community implementation in Roadmap Apr 10, 2024
@tay1orjones tay1orjones added proposal: accepted This request has gone through triaging and we are accepting PR's against it. needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. and removed status: needs triage 🕵️‍♀️ labels Apr 10, 2024
Copy link
Contributor

The Carbon team has accepted this proposal! Our team doesn't have the capacity to work on this now, so we are requesting community contributors. Please see the labels for roles that are needed. If you are willing to help out, comment below and we will get in touch!

@tay1orjones tay1orjones added status: help wanted 👐 good first issue 👋 Used by GitHub to elevate contribution opportunities package: @carbon/react @carbon/react labels Apr 10, 2024
@tay1orjones tay1orjones moved this to ⏱ Backlog in Design System Apr 10, 2024
@tay1orjones tay1orjones moved this from ⏱ Backlog to 🪆 Needs Refined in Design System Apr 10, 2024
@carbon-design-system carbon-design-system deleted a comment from github-actions bot Apr 10, 2024
@gillesdandrea
Copy link

As a work-around, in typescript it's possible to extract the type of a parameter of a function. So

type MyExtractedProps = Parameters<typeof MyFunctionalComponent>[0];

allow to use the Props of a functional components even if not exported.

@wkeese
Copy link
Contributor

wkeese commented Apr 21, 2024

I was going to suggest the same thing, except with ComponentProps, ex:

ComponentProps<typeof TableToolbarSearch>

It's a bit easier to use since you don't need the [0], and IIRC it works for old-school class components too.

Having said that, I have no objection to exporting all the properties explicitly again. That's how it worked in Carbon 10.

@tay1orjones tay1orjones added the planning: umbrella Umbrella issues, surfaced in Projects views label Apr 24, 2024
@tay1orjones tay1orjones self-assigned this Apr 24, 2024
@tay1orjones tay1orjones moved this from 🪆 Needs Refined to ⏱ Backlog in Design System Apr 24, 2024
@tay1orjones tay1orjones moved this from ⏱ Backlog to 🪆 Needs Refined in Design System Apr 24, 2024
@tay1orjones
Copy link
Member

tay1orjones commented May 1, 2024

@Sam-Wool What do your type imports usually look like? Could you confirm that you're not able to import ModalProps? It's currently exported. It isn't exported from the root but is in the Modal module.

// So I believe this won't work:
import { type ModalProps } from "@carbon/react";

// whereas this should work
import { type ModalProps } from "@carbon/react/es/components/Modal";

I don't know that we'll be able to export these from the root until everything is converted and the root index.js goes to index.tsx and can export types.

@mbarrer
Copy link
Contributor

mbarrer commented May 16, 2024

@tay1orjones with regards to ModalProps, this should be easily raised to the @carbon/react bucket if you exported it as well from the /Modal/index.ts bucket. Since the main index.ts of carbon uses the export * from pattern it will pick up those type exports and add them to the bucket.

We've been working a lot this past week on moving things over to carbon11, writing ambient modules to help ease the transition and fill in the missing prop exports. The team was wondering what the expectation is for the end goal of the prop exports. Should we ultimately expect to be able to pull types from the main @carbon/react bucket or should we expect to grab typings from the component files directly?

@haschek
Copy link

haschek commented Aug 28, 2024

The workarounds mentioned by @gillesdandrea and @wkeese may work to get type info for the transpiler but the comments are completely lost, no IDE or tool like Storybook can include that documentation anymore.

Also the import approach mentioned by @tay1orjones does not work in any case because there are still components without exported interfaces, e.g. AccordionItem.

So, let's hope we can get it soon directly from the root @carbon/react.

@tay1orjones
Copy link
Member

@haschek we're tracking it in #16436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: typescript good first issue 👋 Used by GitHub to elevate contribution opportunities needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. package: @carbon/react @carbon/react planning: umbrella Umbrella issues, surfaced in Projects views proposal: accepted This request has gone through triaging and we are accepting PR's against it. status: help wanted 👐 type: enhancement 💡
Projects
Archived in project
Status: Done
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants