use react-is for checking valid element type, update primer/octicons-react #3437

mattcosta7 · 2023-06-21T08:46:16Z

This change allows for trailing/leading components to properly accept React component types other than function components (memo, lazy, forwardRef, etc).

This also bumps the version of primer/octicons-react which could be done separately, but why not just land this together.

This change is backward compat, since function components are still allowed, but properly allows for more types to be considered

Closes #3430

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2023-06-21T08:46:20Z

🦋 Changeset detected

Latest commit: 67f9ea3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-06-21T08:49:34Z

size-limit report 📦

Path	Size
dist/browser.esm.js	101.81 KB (+0.79% 🔺)
dist/browser.umd.js	102.36 KB (+0.78% 🔺)

mattcosta7 · 2023-06-21T08:49:36Z

src/TextInput/TextInput.tsx

@@ -24,11 +25,11 @@ export type TextInputNonPassthroughProps = {
  /**
   * A visual that renders inside the input before the typing area
   */
-  leadingVisual?: string | React.ComponentType<React.PropsWithChildren<{className?: string}>>
+  leadingVisual?: React.ElementType | React.ReactNode


we don't pass props internally, so we don't need to actually care what the props the element can take are

This might a be novice ts question but I assumed that only passing React.ReactNode will make ts happy (it doesn't!) because as far as I understand React.ReactNode includes React.ElementType as well? So I am curious why we need to explicitly type React.ElementType here as well 🤔

ooo interesting. I didn't realize ReactNode included that, and i'm suprised it does (since ElementType isn't renderable as children directly). I think i'd prefer this since it's a bit more explicit that this could be a Component, but could also be renderable content?

mattcosta7 · 2023-06-21T08:49:59Z

src/__tests__/TextInput.test.tsx

@@ -57,10 +57,56 @@ describe('TextInput', () => {

  it('renders leadingVisual', () => {
    expect(render(<TextInput name="search" placeholder={'Search'} leadingVisual={SearchIcon} />)).toMatchSnapshot()
+    expect(render(<TextInput name="search" placeholder={'Search'} leadingVisual={<SearchIcon />} />)).toMatchSnapshot()


just a bunch of tests that validate rendered content given a bunch of different element types

mattcosta7 · 2023-06-21T08:53:49Z

src/TextInputWithTokens/TextInputWithTokens.tsx

@@ -296,7 +297,7 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo
        visualPosition="leading"
        showLoadingIndicator={showLeadingLoadingIndicator}
      >
-        {typeof LeadingVisual === 'function' ? <LeadingVisual /> : LeadingVisual}
+        {typeof LeadingVisual !== 'string' && isValidElementType(LeadingVisual) ? <LeadingVisual /> : LeadingVisual}


we have to check for string explicitly here, because things like 'div' is a valid element type, but we don't want to try and render an empty div, but instead just pass through that text.

We may want to consider whether accepting raw element types makes sense longterm (I don't think there's a good reason to accept a raw Component instead of just React.ReactNode).

<Component leadingVisual={SearchIcon} /> // imo this shouldn't be allowed <Component leadingVisual={<SearchIcon />} /> // imo this should be the way

but that might be a later breaking change at this point?

@mattcosta7 I think you're dead-on and I bet we could look to introduce this behavior in v36 and deprecate the raw element type and remove support for it in v37

Oh great point! Thanks for the note as well, that was one of my questions until I came to that point!

➕ 1 for using ReactNodes instead of raw components

mattcosta7 · 2023-06-21T08:56:52Z

package.json

@@ -99,12 +99,13 @@
    "@github/relative-time-element": "^4.1.2",
    "@lit-labs/react": "1.1.1",
    "@primer/behaviors": "1.3.4",
-    "@primer/octicons-react": "18.3.0",
+    "@primer/octicons-react": "^19.3.0",


I moved from an exact dep, to avoid version conflicts in dotcom, which this would cause eventually otherwise (if dotcom installed, for instance, 19.4.0).

Should octicons-react just be a peer dependency and not a direct dep? This would likely help consumer bundlesize some

mattcosta7 · 2023-06-21T09:03:59Z

this will superceed #3428 which I should have looked for last night before starting this work, and only saw today - however this PR already fixes some issues with that one, so may be easier to just push through

mattcosta7 · 2023-06-21T16:58:57Z

package.json

@@ -118,6 +119,7 @@
    "lodash.isempty": "4.4.0",
    "lodash.isobject": "3.0.2",
    "react-intersection-observer": "9.4.3",
+    "react-is": "18.2.0",


going to loosen this and types so they can be managed better in dotcom

broccolinisoup

Thank you so much for this PR! and thanks for the notes as well, super helpful. Just had one question - other than that. looks great to me!

broccolinisoup · 2023-06-22T00:44:18Z

src/TextInput/TextInput.tsx

@@ -24,11 +25,11 @@ export type TextInputNonPassthroughProps = {
  /**
   * A visual that renders inside the input before the typing area
   */
-  leadingVisual?: string | React.ComponentType<React.PropsWithChildren<{className?: string}>>
+  leadingVisual?: React.ElementType | React.ReactNode


This might a be novice ts question but I assumed that only passing React.ReactNode will make ts happy (it doesn't!) because as far as I understand React.ReactNode includes React.ElementType as well? So I am curious why we need to explicitly type React.ElementType here as well 🤔

broccolinisoup · 2023-06-22T01:53:42Z

src/TextInputWithTokens/TextInputWithTokens.tsx

@@ -296,7 +297,7 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo
        visualPosition="leading"
        showLoadingIndicator={showLeadingLoadingIndicator}
      >
-        {typeof LeadingVisual === 'function' ? <LeadingVisual /> : LeadingVisual}
+        {typeof LeadingVisual !== 'string' && isValidElementType(LeadingVisual) ? <LeadingVisual /> : LeadingVisual}


Oh great point! Thanks for the note as well, that was one of my questions until I came to that point!

➕ 1 for using ReactNodes instead of raw components

broccolinisoup · 2023-06-22T02:18:25Z

I closed my PR and we can continue with this. The only thing is that because the original PR is reverted, we should individually test this at dotcom as a part of the re-introducing strategy. Are you happy to do that @mattcosta7 or do you prefer for me to do it? Either way works for me 🙂

mattcosta7 · 2023-06-22T07:16:23Z

I closed my PR and we can continue with this. The only thing is that because the original PR is reverted, we should individually test this at dotcom as a part of the re-introducing strategy. Are you happy to do that @mattcosta7 or do you prefer for me to do it? Either way works for me 🙂

Opened: https://github.com/github/github/pull/277602

CI looks good there - let me know whehter I should merge this or y'all want to later?

@joshblack re deprecation - I'm not sure what the best way to note that is here, but I agree that sounds reasoable!

mattcosta7 · 2023-06-22T09:39:55Z

VRT failures seem related to the actual icon changing slightly - not sure how to update those

github-actions · 2023-06-22T09:40:09Z

Uh oh! @mattcosta7, the image you shared is missing helpful alt text. Check #3437 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

broccolinisoup · 2023-06-22T11:31:56Z

VRT failures seem related to the actual icon changing slightly - not sure how to update those

I added update snapshot label, hopefully that should take care of it 😊

broccolinisoup · 2023-06-22T11:32:34Z

Opened: https://github.com/github/github/pull/277602
CI looks good there - let me know whehter I should merge this or y'all want to later?

Yay! Once we get the vrt green, feel free to merge it. Thanks again 🎉

use react-is for checking valid element type

7416720

mattcosta7 mentioned this pull request Jun 21, 2023

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

Closed

mattcosta7 commented Jun 21, 2023

View reviewed changes

update primer

1923b6f

github-actions bot temporarily deployed to storybook-preview-3437 June 21, 2023 08:52 Inactive

mattcosta7 commented Jun 21, 2023

View reviewed changes

update primer

02c2e02

mattcosta7 changed the title ~~use react-is for checking valid element type~~ use react-is for checking valid element type, update primer/octicons-react Jun 21, 2023

mattcosta7 commented Jun 21, 2023

View reviewed changes

github-actions bot temporarily deployed to storybook-preview-3437 June 21, 2023 08:59 Inactive

mattcosta7 added 2 commits June 21, 2023 09:01

update octicon

492bca9

changeset

8bcf91a

mattcosta7 marked this pull request as ready for review June 21, 2023 09:03

mattcosta7 requested review from a team and siddharthkp June 21, 2023 09:03

github-actions bot temporarily deployed to storybook-preview-3437 June 21, 2023 09:04 Inactive

mattcosta7 temporarily deployed to github-pages June 21, 2023 09:08 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3437 June 21, 2023 09:09 Inactive

github-actions bot temporarily deployed to storybook-preview-3437 June 21, 2023 09:10 Inactive

joshblack approved these changes Jun 21, 2023

View reviewed changes

joshblack requested a review from broccolinisoup June 21, 2023 15:10

mattcosta7 commented Jun 21, 2023

View reviewed changes

mattcosta7 added 2 commits June 21, 2023 16:59

loosen react-is types

340f0a5

Merge branch 'main' into use-isValidElementType-for-visuals

4955d42

mattcosta7 temporarily deployed to github-pages June 21, 2023 17:06 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3437 June 21, 2023 17:06 Inactive

broccolinisoup approved these changes Jun 22, 2023

View reviewed changes

broccolinisoup mentioned this pull request Jun 22, 2023

Bump @primer/react-octicons to v19.0.1 and update Octicons to use React.forweardRef (re-introducing reverted PRs) #3428

Closed

7 tasks

Merge branch 'main' into use-isValidElementType-for-visuals

4ebc57c

mattcosta7 temporarily deployed to github-pages June 22, 2023 07:03 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3437 June 22, 2023 07:04 Inactive

update some types

16880e8

mattcosta7 temporarily deployed to github-pages June 22, 2023 07:42 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3437 June 22, 2023 07:42 Inactive

broccolinisoup added the update snapshots label Jun 22, 2023

test(vrt): update snapshots

67f9ea3

github-actions bot removed the update snapshots label Jun 22, 2023

primer bot deployed to github-pages June 22, 2023 11:43 View deployment

github-actions bot deployed to storybook-preview-3437 June 22, 2023 11:44 View deployment

mattcosta7 added this pull request to the merge queue Jun 22, 2023

Merged via the queue into main with commit b298c32 Jun 22, 2023

mattcosta7 deleted the use-isValidElementType-for-visuals branch June 22, 2023 17:01

primer-css mentioned this pull request Jun 22, 2023

Release tracking #3421

Merged

skemex mentioned this pull request May 24, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#45

Open

rbragaru mentioned this pull request Aug 14, 2024

[Snyk] Upgrade @primer/react from 35.15.1 to 35.32.2 rbragaru/rocketdex#4

Open

skemex mentioned this pull request Sep 24, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#55

Open

skemex mentioned this pull request Nov 10, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#56

Open

use react-is for checking valid element type, update primer/octicons-react #3437

use react-is for checking valid element type, update primer/octicons-react #3437

Uh oh!

Conversation

mattcosta7 commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Merge checklist

Uh oh!

changeset-bot bot commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcosta7 Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcosta7 commented Jun 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

broccolinisoup left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

broccolinisoup commented Jun 22, 2023

Uh oh!

mattcosta7 commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcosta7 commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 22, 2023

Uh oh!

broccolinisoup commented Jun 22, 2023

Uh oh!

broccolinisoup commented Jun 22, 2023

Uh oh!

Uh oh!

mattcosta7 commented Jun 21, 2023 •

edited

Loading

changeset-bot bot commented Jun 21, 2023 •

edited

Loading

github-actions bot commented Jun 21, 2023 •

edited

Loading

mattcosta7 Jun 21, 2023 •

edited

Loading

broccolinisoup left a comment •

edited

Loading

mattcosta7 commented Jun 22, 2023 •

edited

Loading

mattcosta7 commented Jun 22, 2023 •

edited

Loading