TypeScript refactor

[colebemis]

Based off of @BinaryMuse's original TypeScript PR: #845

Summary

This PR lays the groundwork for an incremental TypeScript refactor of Primer React components.

Impact

From #953:

Refactoring our codebase in TypeScript will make our library less bug-prone by enforcing type safety in the implementation of our components, reduce the number of issues we face with the manual maintenance of our index.d.ts file, and make upstreaming components from other projects at GitHub written in TS much much faster allowing us to greatly improve our contributor experience.

Technical details

Here are the infrastructure changes that were needed to support TypeScript:

Compilation (Babel)

I configured Babel to use the @babel/preset-typescript preset, which allows Babel to compile .ts and .tsx files.

Linting (ESLint)

I configured ESLint to used the @babel/eslint-parser parser, which allows ESLint to parse .ts and .tsx files.

Testing (Jest)

I installed Jest type declarations (@types/jest), which provides type annotations in the test files.

Documentation (Gatsby)

I configured the documentation site to use gatsby-plugin-typescript, which allows us to import .ts and .tsx files into documentation site files.

Type checking and type declarations

I added a yarn dist:types script to type check and generate type declaration files (.d.ts) for every TypeScript file.

How to migrate a component to TypeScript

The migration process for each component may vary, but it will likely follow these high-level steps:

1. Change the file extension from .js to .tsx

src/Box.js → src/Box.tsx

2. Add type annotations to the component file

Here are type annotations for the Box component:

  // src/Box.tsx
+ const Box = styled.div<CommonProps & FlexProps & LayoutProps & SxProps>`
    ${COMMON}
    ${FLEX}
    ${LAYOUT}
    ${sx}
  `
+ export type BoxProps = React.ComponentProps<typeof Box>
  export default Box

Most Primer React components will follow a similar pattern. Each component should export a ___Props type in addition to the default export.

ℹ️ Note: After doing some research on types vs interfaces, I propose we use types (instead of interfaces) for all component props. In addition to the reasons discussed in this article, types allow us to use React.ComponentProps<> to generate prop type definitions for styled components.

❓ Question: How can we programmatically enforce that every component exports a ___Props type?

3. Change the file extension of tests

src/__tests__/Box.js → src/__tests__/Box.tsx

Next steps

Short term

• Rewrite all existing components in TypeScript

Long term (after TypeScript refactor is complete)

• Remove manually maintained index.d.ts
• Remove propTypes
• Generate prop documentation using react-docgen-typescript

[vercel]

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/dv9vvn1wh
✅ Preview: https://primer-components-git-colebemis-ts-refactor.primer.now.sh

[dmarcey]

Wow, @colebemis this is looking great! I like the incremental approach, so that we don't have to have a single, massive migration PR and can hopefully fan the work out a bit.

I left a few comments in-line and also have a few questions / suggestions here.

• Is the idea that we will eventually get rid of the index.d.ts file and just replace it with a top-level index.ts that exports all of our components / interfaces? If so, maybe we don't need to worry about

❓ Question: How can we programmatically enforce that every component exports a ___Props type?
too much? Because that index.ts would hopefully be a pretty straight-forward
export Box, {BoxProps from './Box';
and we could probably spot issues fairly quickly?
Another thought is somehow using some sort of doc auto-generation based on the *Props for the doc site. If these were somehow set up to target the *Props then it would ensure that they existed and were exported. I know this is kind of hand-wavy as I don't know exactly how to do this, but it was just a thought!

• I wonder if converting all of the tests to TS sooner rather than later would be a beneficial incremental step? Now that we have the infrastructure, it seems like it would be safe and straight-forward, and also might help anticipate any trickier gotchas we might encounter along the way.

Overall this is amazing and I'm super excited to see this work!

[dmarcey]

Is it worth re-considering using default exports as well? It is probably a breaking change if anyone was consuming directly from @primer/components/lib/Box or something like that, but if we're going to be making other breaking changes, it might be a fair time to consider it.

I'm not really aware of the main pros/cons of default exports, and it seems like in other code at GitHub we're inconsistent on this already, so maybe it's not even worth looking at 😅

For example, https://github.com/github/details-dialog-element/blob/main/src/index.ts uses a default export, and web components in the dotcom codebase itself seem to be about 50/50.

Just thought I'd mention it to see if it sparks thoughts for others.

[T-Hugs]

Generally I prefer not to use default exports. Here is a good article about it: https://basarat.gitbook.io/typescript/main-1/defaultisbad

[colebemis]

I'm open to moving away from default exports for our components. A few thoughts:

• If we decide to remove default exports, I'd prefer to do that in a separate PR that updates all the exports of all the components at once. This would allow consumers to update all their imports at once.
• How do we ensure consistency across the codebase? Is there an existing linting rule for this?

[T-Hugs]

https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-default-export.md

[dmarcey]

Great job, @colebemis ! Thanks for responding to all of the feedback!
I'm also on board with making the export default changes a separate scope of work, even if we possibly wrap it up in a similar major version release?

I think this is good to 🚢 !

[emplums]

Approving but lets make sure to resolve the TS errors we were seeing in Memex before merging :)

One question: will changing interfaces to types cause breaking changes in consuming applications? I know sometimes in Memex they import things like SelectMenuProps directly so they can extend them

[T-Hugs]

This is awesome - long time coming! Great job!

[colebemis]

@emplums and I found that the location of the Box import statement in index.d.ts was causing type errors in the Memex codebase. Specifically, importing before the module declaration was causing issues:

However, moving the import inside the module declaration causes an error in the Primer React components codebase:

@T-Hugs @dmarcey Any ideas about why this might be happening and how to fix it?

[T-Hugs]

My recommendation is to revert to shipping exclusively the manually-created index.d.ts until the full TypeScript conversion is complete, assuming that will be a relatively fast-follow.