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

Primer leading/trailingVisual and Icons cannot be lazy, memo, forwardRef #3430

Closed
mattcosta7 opened this issue Jun 20, 2023 · 2 comments · Fixed by #3437
Closed

Primer leading/trailingVisual and Icons cannot be lazy, memo, forwardRef #3430

mattcosta7 opened this issue Jun 20, 2023 · 2 comments · Fixed by #3437
Assignees
Labels
bug Something isn't working react

Comments

@mattcosta7
Copy link
Collaborator

mattcosta7 commented Jun 20, 2023

Description

related potentially, but not exactly - #2457

This would not be necessary if we passed rendered elements as props instead of passing components (as the linked PR above suggests)

Primer defines some props that accept function components however they don't accept things like forwardRef components, memoized components or lazy components (at least)

Where does this show up?

Places like this one -

{typeof LeadingVisual === 'function' ? <LeadingVisual /> : LeadingVisual}

this should render anything that's component-like, and not just function components, but it only handles function components.

something like this using react-is might be what we want here, with type updates to match

import {isValidElementType} from 'react-is'

 {isValidElementType(LeadingVisual) ? <LeadingVisual /> : LeadingVisual} 

When using primer/octicons-react version 19+, these components break because icons are all forwardRef components in that version

Attempting to use v19+ in github/github also trips these failures - https://github.com/github/github/pull/276931

Steps to reproduce

Attempt to pass a memoized, lazy or ref forwarding component to a leadingVisual

Version

latest

Browser

No response

@mattcosta7 mattcosta7 added the bug Something isn't working label Jun 20, 2023
@broccolinisoup
Copy link
Member

Hello @mattcosta7!👋🏻 Thanks for raising this issue. Bumping the octicon version was one of the reverted PRs and I did a very poor job on updating these function type checkings on the bump PR 🙈 I already have a PR to re-introducing the bump and function type checks, #3428, now I am using the is-react library as you suggested. It is still wip but I probably will reach out again due to type updates, I am struggling a bit with them.

@broccolinisoup broccolinisoup self-assigned this Jun 21, 2023
@mattcosta7
Copy link
Collaborator Author

@broccolinisoup something like this should do it? #3437

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.

3 participants