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

Normalize usage of types ReactNode and ReactNodeLike #16054

Closed
27 tasks done
cuppajoey opened this issue Mar 25, 2024 · 4 comments
Closed
27 tasks done

Normalize usage of types ReactNode and ReactNodeLike #16054

cuppajoey opened this issue Mar 25, 2024 · 4 comments
Assignees
Labels
planning: umbrella Umbrella issues, surfaced in Projects views severity: 3 https://ibm.biz/carbon-severity type: bug 🐛

Comments

@cuppajoey
Copy link
Contributor

cuppajoey commented Mar 25, 2024

Package

@carbon/react

Browser

No response

Package version

v1.53.0

React version

v16.140

Description

The types defined for TextArea.labelText and TextInput.labelText do not match. The former is typed as prop-types.ReactNodeLike while the latter is typed as React.ReactNode. In older versions of React, ReactNode included support for {}, but has since been removed. Consumers who haven't yet upgraded to React 18 can experience type errors (as demonstrated in the included StackBlitz demo) between two props which should accept the same data types.

Screenshot 2024-03-25 at 11 40 19 AM

While this issue can be worked around, I wanted to take this opportunity to highlight an inconsistent practice when writing types for props that accept a ReactNode. Currently many component props accept ReactNode while others accept ReactNodeLike, e.g., TextArea and TextInput. In some cases both types are being used within the same component (TextInput.labelText vs TextInput.slug).

From my understanding, ReactNodeLike is meant to match ReactNode without being dependent on React itself (See DefinitelyTyped/DefinitelyTyped#29194). However, since @carbon/react has a direct dependency on react, we should use ReactNode exclusively instead of relying on two packages for what is essentially the same type definition.

If the maintainers agree, I'm happy to contribute a fix for this myself 👍.

All the components that have the intertwining between the typescript interface and type annotations from the prop-types library:

Tasks

Preview Give feedback
  1. area: typescript role: dev 🤖 type: bug 🐛
    2nikhiltom
  2. area: typescript role: dev 🤖 severity: 3
    2nikhiltom
  3. area: typescript role: dev 🤖 type: bug 🐛
    2nikhiltom
  4. area: typescript role: dev 🤖 severity: 3 type: bug 🐛
    2nikhiltom
  5. area: typescript role: dev 🤖 severity: 3 type: bug 🐛
    2nikhiltom
  6. area: typescript role: dev 🤖 type: bug 🐛
    2nikhiltom
  7. area: typescript role: dev 🤖 type: bug 🐛
    2nikhiltom
  8. area: typescript role: dev 🤖 type: bug 🐛
    2nikhiltom
  9. area: typescript role: dev 🤖 type: bug 🐛
    2nikhiltom
  10. area: typescript role: dev 🤖 severity: 3 type: bug 🐛
    2nikhiltom
  11. area: typescript role: dev 🤖 type: bug 🐛
    2nikhiltom
  12. component: number-input severity: 3
    Gururajj77
  13. component: number-input severity: 3
    Gururajj77
  14. component: radio-button severity: 3
    Gururajj77
  15. component: select severity: 3
    Gururajj77
  16. component: slider severity: 3
    Gururajj77
  17. component: tags severity: 3
    Gururajj77
  18. component: tags severity: 3
    Gururajj77
  19. component: tags severity: 3
    Gururajj77
  20. component: tags severity: 3
    Gururajj77
  21. severity: 3
    Gururajj77
  22. component: text-area severity: 3
    Gururajj77
  23. component: password-input severity: 3
    Gururajj77
  24. component: text-input severity: 3
    Gururajj77
  25. component: tile severity: 3
    Gururajj77
  26. component: tile severity: 3
  27. area: typescript role: dev 🤖 severity: 3
    2nikhiltom

You can take up each component as a separate issue and work on it.

Reproduction/example

https://stackblitz.com/edit/github-bcs3rp?file=src%2FInput.tsx

Steps to reproduce

  1. Wait for the application to start and for Typescript to load.
  2. Open src/Input.tsx.
  3. Hover over labelText in the TextArea on line 12.

Suggested Severity

None

Application/PAL

No response

Code of Conduct

@tay1orjones
Copy link
Member

Thanks for opening this up. The mismatch here should definitely be fixed. Which way to fix it I'm unsure on. We state support for react 16, 17, and 18. It sounds like using ReactNode might cause typescript errors for 17 and 18 users in certain scenarios. Avoiding using prop-types within TypeScript interfaces seems reasonable despite the downsides.

I'm not opposed to consolidating on using ReactNode for now, and we'll keep an eye on feedback if it's problematic.

@tay1orjones tay1orjones added severity: 3 https://ibm.biz/carbon-severity and removed status: needs triage 🕵️‍♀️ labels Apr 18, 2024
@tay1orjones tay1orjones moved this from 🕵️‍♀️ Triage to ⏱ Backlog in Design System Apr 18, 2024
@cuppajoey
Copy link
Contributor Author

Thanks for opening this up. The mismatch here should definitely be fixed. Which way to fix it I'm unsure on. We state support for react 16, 17, and 18. It sounds like using ReactNode might cause typescript errors for 17 and 18 users in certain scenarios. Avoiding using prop-types within TypeScript interfaces seems reasonable despite the downsides.

Could you point me to some info regarding the potential errors with ReactNode with 17 and 18 users?

I'm not opposed to consolidating on using ReactNode for now, and we'll keep an eye on feedback if it's problematic.

Are you open to contributions on this? We could consolidate these incrementally as we work on other issues.

@tay1orjones
Copy link
Member

@cuppajoey Yes here's one example from slack

image

Are you open to contributions on this?

Yes absolutely, we can do them incrementally as we work on other stuff. @Gururajj77 could you add a list here of all components that need to be modified?

@Gururajj77 Gururajj77 moved this from 🏗 In Progress to 🚦 In Review in Design System May 10, 2024
@Gururajj77 Gururajj77 moved this from 🚦 In Review to 🏗 In Progress in Design System May 10, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in Design System May 16, 2024
@2nikhiltom 2nikhiltom reopened this May 16, 2024
@github-project-automation github-project-automation bot moved this from ✅ Done to ⏱ Backlog in Design System May 16, 2024
@2nikhiltom 2nikhiltom moved this from ⏱ Backlog to 🏗 In Progress in Design System May 16, 2024
@2nikhiltom 2nikhiltom moved this from 🏗 In Progress to 🚦 In Review in Design System May 16, 2024
@2nikhiltom
Copy link
Contributor

Hey ! All the components are covered via following PR's #16453, #16465, #16418, #16428 and #16449

@github-project-automation github-project-automation bot moved this from 🚦 In Review to ✅ Done in Design System May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
planning: umbrella Umbrella issues, surfaced in Projects views severity: 3 https://ibm.biz/carbon-severity type: bug 🐛
Projects
Archived in project
Development

No branches or pull requests

5 participants