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.

Sign up for GitHub

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

Components: enhancements to TypeScript migration guidelines #41669

Merged
merged 6 commits into from
Jun 14, 2022
Merged

Components: enhancements to TypeScript migration guidelines #41669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f071cc5
Add link to TypeScript migration guide in TOC
ciampo Jun 10, 2022
00e17b1
Add point about props destructuring
ciampo Jun 10, 2022
6425db7
Expand point about `WordPressComponentProps`
ciampo Jun 10, 2022
be4c8d0
CHANGELOG
ciampo Jun 10, 2022
1d425ed
Add more info re. ref forwarding
ciampo Jun 13, 2022
da279b7
Do not recommend using the `@example` JSDoc keyword
ciampo Jun 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
- `CustomGradientBar` updated to satisfy `react/exhuastive-deps` eslint rule ([#41463](https://github.com/WordPress/gutenberg/pull/41463))
- `TreeSelect`: Convert to TypeScript ([#41536](https://github.com/WordPress/gutenberg/pull/41536)).
- `FontSizePicker`: updated to satisfy `react/exhuastive-deps` eslint rule ([#41600](https://github.com/WordPress/gutenberg/pull/41600)).
- Enhance the TypeScript migration guidelines ([#41669](https://github.com/WordPress/gutenberg/pull/41669)).


## 19.12.0 (2022-06-01)
Expand Down
106 changes: 94 additions & 12 deletions packages/components/CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ For an example of a component that follows these requirements, take a look at [`
- [Documentation](#documentation)
- [README example](#README-example)
- [Folder structure](#folder-structure)
- [TypeScript migration guide](#refactoring-a-component-to-typescript)

## Compatibility

Expand Down Expand Up @@ -489,13 +490,99 @@ Given a component folder (e.g. `packages/components/src/unit-control`):
2. Resume work on this component once all dependencies have been refactored.
2. Alternatively:
1. For each of those files, add `// @ts-nocheck` at the start of the file.
2. Add the folders to the `tsconfig.json` file.
3. If you’re still getting errors about a component’s props, the easiest way is to slightly refactor this component and perform the props destructuring inside the component’s body (as opposed as in the function signature) — this is to prevent TypeScript from inferring the types of these props.
4. Continue with the refactor of the current component (and take care of the refactor of the dependent components at a later stage).
2. If the components in the ignored files are destructuring props directly from the function's arguments, move the props destructuring to the function's body (this is to avoid TypeScript errors from trying to infer the props type):

```jsx
// Before:
function MyComponent( { myProp1, myProp2, ...restProps } ) { /* ... */ }

// After:
function MyComponent( props ) {
const { myProp1, myProp2, ...restProps } = props;
Comment on lines +496 to +501
Copy link
Member

Choose a reason for hiding this comment

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

Good clarification with the example 👍


/* ... */
}
```

3. Add the folders to the `tsconfig.json` file.
Copy link
Contributor Author

@ciampo ciampo Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Points 3, 4 and 5 here are unchanged apart from their index, that shiftet by 1 after adding a new point (now no. 2)

4. If you’re still getting errors about a component’s props, the easiest way is to slightly refactor this component and perform the props destructuring inside the component’s body (as opposed as in the function signature) — this is to prevent TypeScript from inferring the types of these props.
5. Continue with the refactor of the current component (and take care of the refactor of the dependent components at a later stage).
6. Create a new `types.ts` file.
7. Slowly work your way through fixing the TypeScript errors in the folder:
1. Try to avoid introducing any runtime changes, if possible. The aim of this refactor is to simply rewrite the component to TypeScript.
2. Make sure you have a **named** export for the component, not just the default export ([example](https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/divider/component.tsx)). This ensures that the docgen can properly extract the types data. The naming should be so that the connected/forwarded component has the plain component name (`MyComponent`), and the raw component is prefixed (`UnconnectedMyComponent` or `UnforwardedMyComponent`). This makes the component's `displayName` look nicer in React devtools and in the autogenerated Storybook code snippets.
2. Extract props to `types.ts`, and use them to type components. The README can be of help when determining a prop’s type.
3. Use existing HTML types when possible? (e.g. `required` for an input field?)
4. Use the `CSSProperties` type where it makes sense.
5. Extend existing components’ props if possible, especially when a component internally forwards its props to another component in the package.
6. If the component forwards its `...restProps` to an underlying element/component, you should use the `WordPressComponentProps` type for the component's props:

```jsx
import type { WordPressComponentProps } from '../ui/context';
import type { ComponentOwnProps } from './types';

function UnconnectedMyComponent(
// The resulting type will include:
Copy link
Member

Choose a reason for hiding this comment

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

Should we add the stuff about the ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed some changes in cb34385, let me know what you think!

Main changes:

  • moved down the point re. making sure that there's a name export
  • added a new point re. ref forwarding (either via forwardRef or via contextConnect)
  • updated index of remaining points as needed

// - all props defined in `ComponentOwnProps`
// - all HTML props/attributes from the component specified as the second
// parameter (`div` in this example)
// - the special `as` prop (which marks the component as polymorphic),
// unless the third parameter is `false`
props: WordPressComponentProps< ComponentOwnProps, 'div', true >
) { /* ... */ }
```

7. If the component doesn't forwards its ref yet, wrap the component in a `forwardRed` call. Alternatively, if you want to take advantage of the [Context system](#context-system), you can use the `contextConnect` utility function (which also takes care of adding ref forwarding)
ciampo marked this conversation as resolved.
Show resolved Hide resolved

```jsx
// With `forwardRef`
import type { ForwardedRef } from 'react';
import { forwardRef } from '@wordpress/element';
import type { WordPressComponentProps } from '../ui/context';
import type { ComponentOwnProps } from './types';

function UnforwardedMyComponent(
props: WordPressComponentProps< ComponentOwnProps, 'div', true >,
forwardedRef: ForwardedRef< any >
) { /* ... */ }


/**
* MyComponent JSDoc
*/
export const MyComponent = forwardRef( UnforwardedMyComponent );

export default MyComponent;
```

```jsx
// With `contextConnect`
import type { ForwardedRef } from 'react';
import {
contextConnect,
useContextSystem,
WordPressComponentProps,
} from '../ui/context';
import type { ComponentOwnProps } from './types';

function UnconnectedMyComponent(
props: WordPressComponentProps< ComponentOwnProps, 'div', true >,
forwardedRef: ForwardedRef< any >
) {
const contextProps = useContextSystem( props, 'MyComponent' );

/* ... */
}


/**
* MyComponent JSDoc
*/
export const MyComponent = contextConnect( UnconnectedMyComponent, 'MyComponent' );

export default MyComponent;
```

8. As shown in the previous examples, make sure you have a **named** export for the component, not just the default export ([example](https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/divider/component.tsx)). This ensures that the docgen can properly extract the types data. The naming should be so that the connected/forwarded component has the plain component name (`MyComponent`), and the raw component is prefixed (`UnconnectedMyComponent` or `UnforwardedMyComponent`). This makes the component's `displayName` look nicer in React devtools and in the autogenerated Storybook code snippets.

```jsx
function UnconnectedMyComponent() { /* ... */ }
Expand All @@ -505,14 +592,9 @@ Given a component folder (e.g. `packages/components/src/unit-control`):
export default MyComponent;
```

3. Extract props to `types.ts`, and use them to type components. The README can be of help when determining a prop’s type.
4. Use existing HTML types when possible? (e.g. `required` for an input field?)
5. Use the `CSSProperties` type where it makes sense.
6. Extend existing components’ props if possible, especially when a component internally forwards its props to another component in the package.
7. Use `WordPressComponent` type if possible.
8. Use JSDocs syntax for each TypeScript property that is part of the public API of a component. The docs used here should be aligned with the component’s README. Add `@default` values where appropriate.
9. Prefer `unknown` to `any`, and in general avoid it when possible.
8. On the component's main export, add a JSDoc comment that includes the main description and `@example` code snippet from the README ([example](https://github.com/WordPress/gutenberg/blob/943cec92f21fedcd256502ea72d9903941f3b05a/packages/components/src/unit-control/index.tsx#L290-L306))
9. Use JSDocs syntax for each TypeScript property that is part of the public API of a component. The docs used here should be aligned with the component’s README. Add `@default` values where appropriate.
10. Prefer `unknown` to `any`, and in general avoid it when possible.
8. On the component's main named export, add a JSDoc comment that includes the main description and the example code snippet from the README ([example](https://github.com/WordPress/gutenberg/blob/43d9c82922619c1d1ff6b454f86f75c3157d3de6/packages/components/src/date-time/date-time/index.tsx#L193-L217)). _At the time of writing, the `@example` JSDoc keyword is not recognized by StoryBook's docgen, so please avoid using it_.
9. Make sure that:
1. tests still pass;
2. storybook examples work as expected.
Expand Down