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

Icon component: Add types #26216

Closed
wants to merge 2 commits into from
Closed

Icon component: Add types #26216

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
616d2bf
Type icon component
sirreal Oct 14, 2020
23ca3dc
Apply suggestions from code review
sirreal Oct 30, 2020
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
50 changes: 43 additions & 7 deletions packages/components/src/icon/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,39 @@ import { SVG } from '@wordpress/primitives';
*/
import Dashicon from '../dashicon';

/* eslint-disable jsdoc/valid-types */
/**
* @template T
* @typedef {T extends import('react').ComponentType<infer U> ? U : T extends string ? import('react').ComponentPropsWithoutRef<'span'> : {}} AdditionalProps
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the final default be never rather than {}? When would we end up in that final branch?

Also an aside, the formatting like this is so frustrating to read... I'm assuming there's no way to split this across multiple lines with JSDoc right? Is that another limitation we could add to the list of reasons why full TS is preferred?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should the final default be never rather than {}? When would we end up in that final branch?

🤔 This probably isn't perfect, I don't think it captures the case where icon is an element but this is a good start.

We want something that we can intersect (&) with another type to just keep the other type (an identity element). That's {} (I don't know whether there are other types that would work) where A & {} is the same as A. A & never is never (I had to play a bit to test this out).

Also an aside, the formatting like this is so frustrating to read... I'm assuming there's no way to split this across multiple lines with JSDoc right? Is that another limitation we could add to the list of reasons why full TS is preferred?

💯 It's very unpleasant. I don't believe TypeScript + JSDoc work properly when we break these on multiple lines. It would be much nicer in TypeScript. For reference, this is equivalent TypeScript:

export type AdditionalProps<T> =
    T extends React.ComponentType<infer U>
    ? U
    : T extends string
    ? React.ComponentPropsWithoutRef<'span'>
    : {};

Fortunately, although it's exported (JSDoc typedefs are always exported, but this likely isn't easily accessible from the package) it's really an internal type helper.

*/
/* eslint-enable jsdoc/valid-types */

/**
* @template P
* @typedef {string | import('react').ComponentType<P>|ReactElement} IconType
sirreal marked this conversation as resolved.
Show resolved Hide resolved
*/

/**
* @template P
* @typedef BaseProps
*
* @property {IconType<P>|null} icon The icon to render. Supported values are: Dashicons (specified as strings), functions, WPComponent instances and `null`.
sirreal marked this conversation as resolved.
Show resolved Hide resolved
* @property {number} [size] The size (width and height) of the icon.
*/

/**
* @template {{size?: number}} P
* @param {BaseProps<P> & AdditionalProps<IconType<P>>} props
* @return {JSX.Element|null} Element
sirreal marked this conversation as resolved.
Show resolved Hide resolved
*/
function Icon( { icon = null, size, ...additionalProps } ) {
if ( 'string' === typeof icon ) {
return <Dashicon icon={ icon } { ...additionalProps } />;
}

if ( icon && Dashicon === icon.type ) {
return cloneElement( icon, {
// Type Assertion: We know `icon` is defined and we can check the `.type` property.
if ( icon && Dashicon === /** @type {ReactElement} */ ( icon ).type ) {
return cloneElement( /** @type {ReactElement} */ ( icon ), {
...additionalProps,
} );
}
Expand All @@ -29,13 +55,21 @@ function Icon( { icon = null, size, ...additionalProps } ) {
const iconSize = size || 24;
if ( 'function' === typeof icon ) {
if ( icon.prototype instanceof Component ) {
return createElement( icon, {
size: iconSize,
...additionalProps,
} );
return createElement(
icon,
/* eslint-disable jsdoc/no-undefined-types */
/** @type {P} */ ( {
size: iconSize,
...additionalProps,
} )
/* eslint-enable jsdoc/no-undefined-types */
);
}

return icon( { size: iconSize, ...additionalProps } );
return /** @type {import('react').FunctionComponent} */ ( icon )( {
size: iconSize,
...additionalProps,
} );
}

if ( icon && ( icon.type === 'svg' || icon.type === SVG ) ) {
Expand All @@ -60,3 +94,5 @@ function Icon( { icon = null, size, ...additionalProps } ) {
}

export default Icon;

/** @typedef {import('react').ReactElement} ReactElement */
1 change: 1 addition & 0 deletions packages/components/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"include": [
"src/base-control/**/*",
"src/dashicon/**/*",
"src/icon/**/*",
"src/tip/**/*",
"src/utils/**/*",
"src/visually-hidden/**/*"
Expand Down