feat: Migrate button components to TypeScript #1017

smockle · 2021-02-04T21:21:03Z

This is classified as a “major” change, because I removed default exports from the individual button component files, following TypeScript best practices. I retained the default export in the barrel (index.ts) for backwards compatibility (we could do the same in other files, exporting both named and default exports).

Updated per the strategy @colebemis outlined below.

Part of #970

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 · 2021-02-04T21:21:05Z

🦋 Changeset detected

Latest commit: 7a24774

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

This PR includes changesets to release 1 package

Name	Type
@primer/components	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

vercel · 2021-02-04T21:21:06Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/c21svfo4m
✅ Preview: https://primer-components-git-smockle-button.primer.now.sh

colebemis

This is great, @smockle! Thanks for taking this on ❤️

A few thoughts:

propTypes

It looks like tests are failing because of missing propTypes. Let's not remove propTypes in this PR. Instead, I'd rather remove propTypes from all the components (and update tests) at once in one PR during the cleanup phase of this epic.

default exports

I'm on board with moving away from default exports. However, like propTypes, I think we should remove all the default exports (and add a linting rule) at once in one PR during the cleanup phase (I'll add that to our epic planning issue). That way we can batch breaking changes into one release instead of spreading them across multiple releases.

naming

I noticed you've named the types passed to styled-components ___InternalProps. In other components, we use the name Styled___Props for similar types. I don't have a strong opinion about what naming convention we should use (I actually prefer your naming) but I think we should be consistent. So let's either rename the types in this PR to Styled___Props or rename the types in other components to ___InternalProps.

smockle · 2021-02-05T18:02:10Z

This is great, @smockle! Thanks for taking this on ❤️

A few thoughts:

propTypes

It looks like tests are failing because of missing propTypes. Let's not remove propTypes in this PR. Instead, I'd rather remove propTypes from all the components (and update tests) at once in one PR during the cleanup phase of this epic.

default exports

I'm on board with moving away from default exports. However, like propTypes, I think we should remove all the default exports (and add a linting rule) at once in one PR during the cleanup phase (I'll add that to our epic planning issue). That way we can batch breaking changes into one release instead of spreading them across multiple releases.

naming

I noticed you've named the types passed to styled-components ___InternalProps. In other components, we use the name Styled___Props for similar types. I don't have a strong opinion about what naming convention we should use (I actually prefer your naming) but I think we should be consistent. So let's either rename the types in this PR to Styled___Props or rename the types in other components to ___InternalProps.

Great feedback, thanks! I appreciate you taking the time to review and to clearly state the next steps. I agree 100% re: scoping the PR down (i.e. keeping propTypes and default exports) and re: making its names consistent with what we use elsewhere. 👍

… styled components.

smockle · 2021-02-05T20:08:40Z

In a4ffc03, I did three things:

Restored propTypes
Restored default exports
Renamed type used for styled components props

This is ready for another review! 🚀

microsoft/TypeScript#27956 > These errors occur when you have subdirectories of a typeRoots directory…that do not contain index.d.ts files.

colebemis

Looks great! Just left a few minor comments

colebemis · 2021-02-05T21:12:19Z

src/Button/Button.tsx


-const Button = styled(ButtonBase)`
+const Button = styled(ButtonBase)<ButtonBaseProps>`


I think styled can infer the props accepted by ButtonBase. It's worth testing out though.

Suggested change

const Button = styled(ButtonBase)<ButtonBaseProps>`

const Button = styled(ButtonBase)`

Thanks, it looks like you’re right! Out of curiosity—when is an explicit type necessary? I noticed one is provided in Avatar, Grid, and StyledOcticon, so I thought it was a Primer code style convention and tried to follow it here. I’m now realizing there might be a specific need for it in those cases?

Removed <ButtonBaseProps> from Button and similar in 1d60b55

Note: An explicit type was restored in 7f8997e, which added ButtonSystemProps

Note: The explicit type restored in 7f8997e was amended in 80317da, because sx was apparently not composeable, so I applied it separately.

colebemis · 2021-02-05T21:14:28Z

src/Button/ButtonBase.tsx

+type StyledButtonBaseProps = {
+  as?: 'button' | 'a' | 'summary' | 'input' | string | React.ReactType
+  fontSize?: FontSizeProps['fontSize']
+  variant?: 'small' | 'medium' | 'large'
+} & SystemCommonProps &
+  SystemLayoutProps &
+  SxProp


Just curious, is there a reason you didn't write it this way?

Suggested change

type StyledButtonBaseProps = {

as?: 'button' | 'a' | 'summary' | 'input' | string | React.ReactType

fontSize?: FontSizeProps['fontSize']

variant?: 'small' | 'medium' | 'large'

} & SystemCommonProps &

SystemLayoutProps &

SxProp

type StyledButtonBaseProps = {

as?: 'button' | 'a' | 'summary' | 'input' | string | React.ReactType

variant?: 'small' | 'medium' | 'large'

} & FontSizeProps &

SystemCommonProps &

SystemLayoutProps &

SxProp

Hmm, also isn't the type incorrect because ButtonBase doesn't actually accept common, layout, and fontsize props?

Just curious, is there a reason you didn't write it this way?

It’s possible I thought (incorrectly) that FontSizeProps includes keys not used by ButtonBase (i.e. in addition to 'fontSize')—but it’s more likely the intersection simply didn’t occur to me 😅

…[I]sn't the type incorrect because ButtonBase doesn't actually accept common, layout, and fontsize props?

I’d defined it this way to avoid needing to add those to custom types for Button, ButtonDanger, ButtonInvisible, ButtonOutline and ButtonPrimary (which all use fontSize, COMMON, LAYOUT, and sx), in order to make ButtonBaseProps more useful as a base. It didn’t occur to me that—in doing so—I’d made ButtonBase’s prop type itself wrong, especially because ButtonBase is never (that I know of) rendered outside one of those components.

It definitely makes sense to clean that up (i.e. to type ButtonBase’s props correctly). Thanks for catching this! 👍

It definitely makes sense to clean that up…

Note: @colebemis gave a good suggestion for how to do this DRYly in #1017 (comment)

Sidebar: Is it true that “ButtonBase doesn't actually accept [a] fontsize prop”? The propTypes in the current published version includes fontSize.

Fixed in 7f8997e and 80317da

src/Button/ButtonGroup.tsx

src/Button/ButtonOutline.tsx

colebemis · 2021-02-05T21:23:03Z

src/Button/index.ts

@@ -0,0 +1,9 @@
+export {Button as default} from './Button'
+export {Button} from './Button'


Should the props types be exported here too?

There’s no right answer; it depends on the ergonomics we want to provide. It’s common for barrels to re-export all exports (which would include type exports), e.g. from the TypeScript Deep Dive: Barrel page:

// demo/index.ts export * from './foo'; // re-export all of its exports export * from './bar'; // re-export all of its exports export * from './baz'; // re-export all of its exports

(Though I think this specific syntax is incompatible with default exports, so this may be something we choose to revisit after we replace them with named exports.)

I think this specific syntax is incompatible with default exports

The answer to this isn’t clearer to me after reading the MDN export page and the ECMA-262 spec.

If you export * from two files which each have a default export, which one is bound to the barrel’s default export?

I tested this in a sandbox. I get two errors that Module '"/sandbox/src/module-aggregate"' has no default export. and 'export *' does not re-export a default., but it still works. 😱 That is, the first re-exported function appears to be bound to the aggregate (barrel)’s default, contra the error messages.

Setting export * aside, we can go ahead and export the named prop types explicitly. Added this via e3aed73.

colebemis · 2021-02-05T21:25:03Z

src/Button/ButtonBase.tsx

 import systemPropTypes from '@styled-system/prop-types'
+import {FontSizeProps} from 'styled-system'
+
+export const systemStyles = compose(fontSize, COMMON, LAYOUT)


Maybe let's give this a better name:

Suggested change

export const systemStyles = compose(fontSize, COMMON, LAYOUT)

export const buttonSystemProps = compose(fontSize, COMMON, LAYOUT)

Since this gets exported from this file instead of being applied to ButtonBase. Maybe we should export a ButtonSystemProps type that the other components can use instead of adding FontSizeProps & SystemCommonProps & SystemLayoutProps to the StyledButtonBaseProps type.

Maybe let's give this a better name:

Since systemStyles is the name used in the current published version, would renaming it (which I agree we should do, for the reasons you stated) be a breaking change?

Maybe we should export a ButtonSystemProps type that the other components can use instead of adding FontSizeProps & SystemCommonProps & SystemLayoutProps to the StyledButtonBaseProps type.

Great idea—this is an elegant way to address #1017 (comment). 👍

Fixed by 7f8997e and 80317da

Instead, share these via a different type ('ButtonSystemProps').

…m other 'buttonSystemProps'

smockle · 2021-02-06T19:18:15Z

From @colebemis in #1017 (review):

Looks great! Just left a few minor comments

🎊 Thanks for reviewing! I’ve addressed each of the comments, so this is ready for another review.

colebemis · 2021-02-08T18:29:41Z

I think this is pretty much good to go! 🎉 @smockle Can you update the test files and snapshots as well?

smockle · 2021-02-08T19:35:50Z

I think this is pretty much good to go! 🎉 @smockle Can you update the test files and snapshots as well?

Thanks! Done, in 6e411c4 👍

colebemis

Wooo! Nice work 🎉

feat: Migrate button components to TypeScript

a4dce5d

smockle marked this pull request as draft February 4, 2021 21:21

fix: Import named import to make 'npm run dist' pass

2402cd7

vercel bot temporarily deployed to Preview February 5, 2021 04:47 Inactive

smockle marked this pull request as ready for review February 5, 2021 04:47

colebemis requested changes Feb 5, 2021

View reviewed changes

colebemis mentioned this pull request Feb 5, 2021

[Epic] TypeScript refactor #970

Closed

53 tasks

Merge branch 'main' into smockle/button

14ac1ef

vercel bot temporarily deployed to Preview February 5, 2021 18:13 Inactive

fix: Restore propTypes. Restore default exports. Rename props used by…

a4ffc03

… styled components.

vercel bot temporarily deployed to Preview February 5, 2021 20:06 Inactive

chore: Downgrade non-breaking button changes to 'patch'

a17ff07

smockle requested a review from colebemis February 5, 2021 20:08

vercel bot temporarily deployed to Preview February 5, 2021 20:09 Inactive

fix: TS2688

942a11c

microsoft/TypeScript#27956 > These errors occur when you have subdirectories of a typeRoots directory…that do not contain index.d.ts files.

vercel bot temporarily deployed to Preview February 5, 2021 20:19 Inactive

fix: Fix types in Button story

9218fe4

vercel bot temporarily deployed to Preview February 5, 2021 20:34 Inactive

colebemis reviewed Feb 5, 2021

View reviewed changes

fix: Remove 'ButtonBaseProps' where it’s inferable

1d60b55

vercel bot temporarily deployed to Preview February 6, 2021 17:56 Inactive

fix: Don’t add types to 'ButtonBaseProps' that 'ButtonBase' doesn’t use

7f8997e

Instead, share these via a different type ('ButtonSystemProps').

vercel bot temporarily deployed to Preview February 6, 2021 18:24 Inactive

fix: Infer 'ButtonGroup'’s props type

8996990

vercel bot temporarily deployed to Preview February 6, 2021 18:29 Inactive

feat: Export prop types alongside components

e3aed73

vercel bot temporarily deployed to Preview February 6, 2021 19:05 Inactive

fix: Apparently 'sx' is not 'compose'able, so apply it separately fro…

80317da

…m other 'buttonSystemProps'

vercel bot temporarily deployed to Preview February 6, 2021 19:12 Inactive

smockle requested a review from colebemis February 6, 2021 19:14

chore: Change file extension (js -> tsx) for tests and snapshots

6e411c4

vercel bot temporarily deployed to Preview February 8, 2021 19:34 Inactive

Merge branch 'main' into smockle/button

7a24774

vercel bot temporarily deployed to Preview February 8, 2021 19:40 Inactive

colebemis approved these changes Feb 8, 2021

View reviewed changes

colebemis merged commit 7a73fe5 into main Feb 8, 2021

colebemis deleted the smockle/button branch February 8, 2021 20:06

github-actions bot mentioned this pull request Feb 8, 2021

Release Tracking #1023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Migrate button components to TypeScript #1017

feat: Migrate button components to TypeScript #1017

smockle commented Feb 4, 2021 •

edited

Loading

changeset-bot bot commented Feb 4, 2021 •

edited

Loading

vercel bot commented Feb 4, 2021 •

edited

Loading

colebemis left a comment

smockle commented Feb 5, 2021

propTypes

default exports

naming

smockle commented Feb 5, 2021

colebemis left a comment

colebemis Feb 5, 2021

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021

smockle Feb 6, 2021

smockle Feb 6, 2021

colebemis Feb 5, 2021

colebemis Feb 5, 2021

smockle Feb 6, 2021

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021

smockle Feb 6, 2021 •

edited

Loading

colebemis Feb 5, 2021

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021

smockle Feb 6, 2021

colebemis Feb 5, 2021

colebemis Feb 5, 2021

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021

smockle commented Feb 6, 2021

colebemis commented Feb 8, 2021

smockle commented Feb 8, 2021

colebemis left a comment


		const Button = styled(ButtonBase)`
		const Button = styled(ButtonBase)<ButtonBaseProps>`

		@@ -0,0 +1,9 @@
		export {Button as default} from './Button'
		export {Button} from './Button'

	export const systemStyles = compose(fontSize, COMMON, LAYOUT)
	export const buttonSystemProps = compose(fontSize, COMMON, LAYOUT)

feat: Migrate button components to TypeScript #1017

feat: Migrate button components to TypeScript #1017

Conversation

smockle commented Feb 4, 2021 • edited Loading

Screenshots

Merge checklist

changeset-bot bot commented Feb 4, 2021 • edited Loading

🦋 Changeset detected

vercel bot commented Feb 4, 2021 • edited Loading

colebemis left a comment

Choose a reason for hiding this comment

propTypes

default exports

naming

smockle commented Feb 5, 2021

propTypes

default exports

naming

smockle commented Feb 5, 2021

colebemis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smockle Feb 6, 2021 • edited Loading

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

smockle Feb 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smockle Feb 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smockle Feb 6, 2021 • edited Loading

Choose a reason for hiding this comment

smockle Feb 6, 2021 • edited Loading

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

smockle Feb 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smockle commented Feb 6, 2021

colebemis commented Feb 8, 2021

smockle commented Feb 8, 2021

colebemis left a comment

Choose a reason for hiding this comment

smockle commented Feb 4, 2021 •

edited

Loading

changeset-bot bot commented Feb 4, 2021 •

edited

Loading

vercel bot commented Feb 4, 2021 •

edited

Loading

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021 •

edited

Loading

smockle Feb 6, 2021 •

edited

Loading