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

[Epic] TypeScript refactor #970

Closed
53 tasks done
colebemis opened this issue Jan 20, 2021 · 6 comments
Closed
53 tasks done

[Epic] TypeScript refactor #970

colebemis opened this issue Jan 20, 2021 · 6 comments

Comments

@colebemis
Copy link
Contributor

colebemis commented Jan 20, 2021

Objective

Rewrite Primer React components in TypeScript.

📄 TypeScript ADR
📄 TypeScript notes google doc

Team

Tasks

Phase 1: Setup

Phase 2: Migrate

  • Migrate components¹

In Review or Done

Phase 3: Clean up

Postponed

¹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
  import styled from 'styled-components'
+ import {COMMON, FLEX, LAYOUT, SystemCommonProps, SystemFlexProps, LayoutProps} from './constants'
+ import sx, {SxProp} from './sx'
+ import {ComponentProps} from './utils/types'


+ const Box = styled.div<CommonProps & FlexProps & LayoutProps & SxProp>`
    ${COMMON}
    ${FLEX}
    ${LAYOUT}
    ${sx}
  `
+ export type BoxProps = 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.

3. Change the file extension of tests

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

👉 See the TypeScript notes google doc for more information on TypeScript migration.

Exit criteria

  • All JavaScript files in the src directory of primer/components have been rewritten in TypeScript

/cc @philipbremer

@colebemis colebemis changed the title [Tracking] Primer React TypeScript refactor [Tracking] TypeScript refactor Jan 20, 2021
@colebemis colebemis changed the title [Tracking] TypeScript refactor [Epic] TypeScript refactor Jan 25, 2021
@colebemis colebemis assigned tebriel and colebemis and unassigned tebriel Jan 25, 2021
@VanAnderson VanAnderson self-assigned this Jan 27, 2021
@emplums emplums added the epic label Jan 27, 2021
@colebemis
Copy link
Contributor Author

colebemis commented Mar 4, 2021

Status

💚 Green

Ships

We removed propTypes from all our components in favor of TypeScript types.

Blockers

We’re getting as prop-related errors when testing the generated type definitions in Memex which is blocking us from merging #1067

Up Next

  • I'll be meeting with @T-Hugs next week to discuss the as prop errors and brainstorm possible workarounds.
  • I'll be adding TypeScript guidelines to the contribution guidelines

@colebemis
Copy link
Contributor Author

Status

💛 Yellow

Blockers

The final step of this TypeScript refactor is to replace our manually maintained type definitions with type definitions generated by TypeScript. However, after testing in Memex, I found that the generated type definitions were causing errors because of the way the as prop is defined.

I figured out a way to correctly define types for the as prop but it will require us to update all our components:

Here's an example of how our components will need to be updated
import styled from 'styled-components'
import {COMMON, FLEX, LAYOUT, SystemCommonProps, SystemFlexProps, SystemLayoutProps} from './constants'
import sx, {SxProp} from './sx'
- import {ComponentProps} from './utils/types'
+ import {ForwardRefComponent} from './utils/polymorphic'

+ const defaultElement = 'div'

+ export type BoxProps = SystemCommonProps &
+  SystemFlexProps &
+  SystemLayoutProps &
+  SxProp

+ type BoxComponent = ForwardRefComponent<typeof defaultElement, BoxProps>

+ const Box = styled(defaultElement)<BoxProps>`
- const Box = styled.div<BoxProps>`
  ${COMMON}
  ${FLEX}
  ${LAYOUT}
  ${sx};
+ ` as BoxComponent
- `

- export type BoxProps = ComponentProps<typeof Box>
export default Box

I haven't had time to completely implement this solution because of Primer React theming work. I'll reach out to @VanAnderson to see if he as the bandwidth to take this on.

Up Next

  • Fix the as prop types so we can replace our manually maintained type definitions with type definitions generated by TypeScript
  • Add TypeScript guidelines to the contribution guidelines

@colebemis
Copy link
Contributor Author

Status

💛 Yellow

No updates on TypeScript this week because @VanAnderson and I have been focused on the Primer React theming epic.

Up Next

  • Fix the as prop types so we can replace our manually maintained type definitions with type definitions generated by TypeScript
  • Add TypeScript guidelines to the contribution guidelines

@colebemis
Copy link
Contributor Author

Status

💛 Yellow

No updates on TypeScript this week because @VanAnderson and I have been focused on the Primer React theming epic. I'll have the bandwidth to finish up TypeScript work next week.

Up Next

  • Fix the as prop types so we can replace our manually maintained type definitions with type definitions generated by TypeScript
  • Add TypeScript guidelines to the contribution guidelines

@colebemis
Copy link
Contributor Author

4/1 Update

Status 💚

Ships

This week we replaced our hand-written type definitions with type definitions generated by the TypeScript compiler (#1147). This means consumers get more accurate type definitions and maintainers no longer have to manually keep the type definitions in sync with the code! This is the culmination of a quarter-long effort to rewrite Primer React in TypeScript 🎉 (#970). These changes will be included in the next release of Primer React.

Up Next

  • Write guidelines for authoring components in TypeScript.

@colebemis
Copy link
Contributor Author

The exit criteria has been met 🎉 I'm going to go ahead and close this issue. We can track follow-up tasks in separate issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants