Fix TextInput types #1959

colebemis · 2022-03-14T21:15:12Z

Problem

The type for TextInput props was resolving to any, causing a bad developer experience for consumers.

After doing some digging, I found that the issue was caused by this line:

type TextInputInternalProps = Merge<React.ComponentPropsWithoutRef<typeof UnstyledTextInput>, NonPassthroughProps>
// any & React.RefAttributes & React.Attributes

Passing a styled component type to React.ComponentPropsWithoutRef yields an any type.

Solution

I fixed the type issue by rewriting the TextInputProps to avoid using React.ComponentPropsWithoutRef with a styled component.

Fixing the TextInput type revealed 50+ type errors in our codebase 😅 I fixed those errors in this PR as well.

Closes #1948

changeset-bot · 2022-03-14T21:15:15Z

🦋 Changeset detected

Latest commit: ac292b3

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 · 2022-03-14T21:18:27Z

size-limit report 📦

Path	Size
dist/browser.esm.js	63.19 KB (+0.01% 🔺)
dist/browser.umd.js	63.55 KB (+0.01% 🔺)

colebemis · 2022-03-15T22:56:02Z

src/TextInput.tsx

 >

-// Note: using ComponentProps instead of ComponentPropsWithoutRef here would cause a type issue where `css` is a required prop.
-type TextInputInternalProps = Merge<React.ComponentPropsWithoutRef<typeof UnstyledTextInput>, NonPassthroughProps>


This line was the root of the problem. Passing a styled component type to React.ComponentPropsWithoutRef yields an any type

import styled from 'styled-components' import React from 'react' const StyledComponent = styled.div`...` type Props = React.ComponentPropsWithoutRef<typeof StyledComponent> // any

Good to know so we don't make this mistake in the future.

colebemis · 2022-03-15T22:56:28Z

src/TextInput.tsx

+  | 'minWidth'
+  | 'variant'
+  | 'size'
+  | 'validationStatus'


This type was missing validationStatus

colebemis · 2022-03-15T22:57:14Z

src/TextInput.tsx

  /** @deprecated Use `leadingVisual` or `trailingVisual` prop instead */
  icon?: React.ComponentType<{className?: string}>
  leadingVisual?: string | React.ComponentType<{className?: string}>
  trailingVisual?: string | React.ComponentType<{className?: string}>
 } & Pick<
-  ComponentProps<typeof TextInputWrapper>,
-  'block' | 'contrast' | 'disabled' | 'monospace' | 'sx' | 'width' | 'maxWidth' | 'minWidth' | 'variant' | 'size'
+  StyledWrapperProps,


ComponentProps<> wasn't necessary here. We should avoid using ComponentProps<> when possible because it can yield unexpected results somtimes.

colebemis · 2022-03-15T22:58:26Z

src/TextInput.tsx

@@ -78,13 +87,12 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputInternalProps>(
      </TextInputWrapper>
    )
  }
-)
+) as PolymorphicForwardRefComponent<'input', TextInputProps>


Using the polymorphic type utility from Radix here to implement correct types for the as prop.

Eventually, we should deprecate the as prop for TextInput. I can't think of a good use case for it now that we have the Textarea component

colebemis · 2022-03-15T22:59:03Z

src/TextInputWithTokens.tsx

@@ -53,7 +52,7 @@ type TextInputWithTokensInternalProps<TokenComponentType extends AnyReactCompone
   * The number of tokens to display before truncating
   */
  visibleTokenCount?: number
-} & TextInputProps
+} & Omit<TextInputProps, 'size'>


The size prop from TextInputProps was clashing with the size prop defined above

colebemis · 2022-03-15T22:59:54Z

src/TextInputWithTokens.tsx

-  }: TextInputWithTokensInternalProps<TokenComponentType> & {
-    selectedTokenIndex: number | undefined
-    setSelectedTokenIndex: React.Dispatch<React.SetStateAction<number | undefined>>
-  },


I don't think selectedTokenIndex and setSelectedTokenIndex are props that we use in this component

You're right. I'm not sure how these types made it here 🤔

mperrotti

Thanks for fixing all of these. Code looks good, and everything in the preview deployment looks good

mperrotti · 2022-03-15T23:28:22Z

src/stories/Overlay.stories.tsx

@@ -311,7 +311,7 @@ export const NestedOverlays = () => {
                  Create a list to organize your starred repositories.
                </Text>
                <TextInput placeholder="Name this list" sx={{mb: 2}} />
-                <TextInput as="textarea" placeholder="Write a description" rows="3" sx={{mb: 2, textarea: {p: 2}}} />
+                <TextInput as="textarea" placeholder="Write a description" rows={3} sx={{mb: 2, textarea: {p: 2}}} />


Could we just change this to a Textarea?

Yes, but until we deprecate the as prop for TextInput I think consumers will still do this. If people do this in their apps, I like that we are type-checking this use-case in our codebase.

Ah ok. Let's keep it as-is then.

colebemis · 2022-03-16T21:29:00Z

@dusave tested these changes in Memex and didn't run into any build or type errors. Thanks, @dusave!

* Add failing TextInput type test * Create loud-schools-own.md * Update TextInput types * Add another TextInput type test * Update TextInput props type * Update TextInputWithTokens types * Fix incorrect uses of TextInputWithTokens * Update SelectMenu types test * Fix TextInput tests * Update snapshot tests * autocomplete -> autoComplete * Remove theme prop SelectMenuFilter * Fix TextInput in Overlay story * Fix TextInputWithTokens stories

Add failing TextInput type test

15f9e4b

Create loud-schools-own.md

abafd95

github-actions bot deployed to Preview March 14, 2022 21:23 View deployment

Merge branch 'main' into fix-textinput-types

30cf3f6

github-actions bot deployed to Preview March 15, 2022 17:13 View deployment

colebemis added 12 commits March 15, 2022 14:36

Update TextInput types

fcb5cd8

Add another TextInput type test

42d96ce

Update TextInput props type

2121744

Update TextInputWithTokens types

a22c7d1

Fix incorrect uses of TextInputWithTokens

97777ee

Update SelectMenu types test

e88fed1

Fix TextInput tests

cd292bb

Update snapshot tests

ea5c974

autocomplete -> autoComplete

594e842

Remove theme prop SelectMenuFilter

611a842

Fix TextInput in Overlay story

35110f8

Fix TextInputWithTokens stories

f0c2590

github-actions bot deployed to Preview March 15, 2022 22:43 View deployment

colebemis commented Mar 15, 2022

View reviewed changes

src/TextInput.tsx

| 'minWidth'

| 'variant'

| 'size'

| 'validationStatus'

Copy link

Contributor Author

colebemis Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type was missing validationStatus

colebemis commented Mar 15, 2022

View reviewed changes

Merge branch 'main' into fix-textinput-types

2678bbc

colebemis marked this pull request as ready for review March 15, 2022 23:08

colebemis requested review from a team and siddharthkp March 15, 2022 23:08

colebemis requested a review from mperrotti March 15, 2022 23:09

github-actions bot deployed to Preview March 15, 2022 23:09 View deployment

colebemis added the react label Mar 15, 2022

mperrotti approved these changes Mar 15, 2022

View reviewed changes

Merge branch 'main' into fix-textinput-types

d4d98cf

github-actions bot deployed to Preview March 16, 2022 17:27 View deployment

Merge branch 'main' into fix-textinput-types

ac292b3

github-actions bot deployed to Preview March 16, 2022 21:20 View deployment

colebemis merged commit 2025036 into main Mar 16, 2022

colebemis deleted the fix-textinput-types branch March 16, 2022 22:57

primer-css mentioned this pull request Mar 16, 2022

Release tracking #1966

Merged

siddharthkp mentioned this pull request Apr 13, 2022

[Accepted] ADR: Use Box + sx to author components #2020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TextInput types #1959

Fix TextInput types #1959

colebemis commented Mar 14, 2022 •

edited

Loading

changeset-bot bot commented Mar 14, 2022 •

edited

Loading

github-actions bot commented Mar 14, 2022 •

edited

Loading

colebemis Mar 15, 2022

mperrotti Mar 15, 2022

colebemis Mar 15, 2022

colebemis Mar 15, 2022

colebemis Mar 15, 2022

colebemis Mar 15, 2022

colebemis Mar 15, 2022

mperrotti Mar 15, 2022

mperrotti left a comment

mperrotti Mar 15, 2022

colebemis Mar 16, 2022

mperrotti Mar 16, 2022

colebemis commented Mar 16, 2022 •

edited

Loading

Fix TextInput types #1959

Fix TextInput types #1959

Conversation

colebemis commented Mar 14, 2022 • edited Loading

Problem

Solution

changeset-bot bot commented Mar 14, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Mar 14, 2022 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mperrotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colebemis commented Mar 16, 2022 • edited Loading

colebemis commented Mar 14, 2022 •

edited

Loading

changeset-bot bot commented Mar 14, 2022 •

edited

Loading

github-actions bot commented Mar 14, 2022 •

edited

Loading

colebemis commented Mar 16, 2022 •

edited

Loading