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

compose: Add types to withInstanceId and corresponding hook #31341

Merged
merged 4 commits into from
May 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 5 additions & 9 deletions packages/compose/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -267,9 +267,9 @@ Provides a unique instance ID.

_Parameters_

- _object_ `Object`: Object reference to create an id for.
- _prefix_ `string`: Prefix for the unique id.
- _preferredId_ `string`: Default ID to use.
- _object_ `object`: Object reference to create an id for.
- _prefix_ `[string]`: Prefix for the unique id.
- _preferredId_ `[string]`: Default ID to use.

_Returns_

Expand Down Expand Up @@ -470,13 +470,9 @@ _Returns_
A Higher Order Component used to be provide a unique instance ID by
component.

_Parameters_

- _WrappedComponent_ `WPComponent`: The wrapped component.

_Returns_
_Type_

- `WPComponent`: Component with an instanceId prop.
- `PropInjectingHigherOrderComponent< { instanceId: string | number; } >`

<a name="withSafeTimeout" href="#withSafeTimeout">#</a> **withSafeTimeout**

Expand Down
20 changes: 0 additions & 20 deletions packages/compose/src/higher-order/with-instance-id/index.js

This file was deleted.

38 changes: 38 additions & 0 deletions packages/compose/src/higher-order/with-instance-id/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { ComponentType } from 'react';

/**
* Internal dependencies
*/
import createHigherOrderComponent from '../../utils/create-higher-order-component';
// eslint-disable-next-line no-duplicate-imports
import type { PropInjectingHigherOrderComponent } from '../../utils/create-higher-order-component';
import useInstanceId from '../../hooks/use-instance-id';

/**
* A Higher Order Component used to be provide a unique instance ID by
* component.
*/
const withInstanceId: PropInjectingHigherOrderComponent< {
instanceId: string | number;
} > = createHigherOrderComponent(
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to extract a InstanceIdProps or even export it? The type definition is used twice in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally did that but it made the extracted documentation worse because it made the type PropRemovingHigherOrderComponent< InstanceIdProps > instead of listing the props that are removed. Do you think it's still worth aliasing the type in that case?

Copy link
Member

Choose a reason for hiding this comment

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

I would personally do it and document the InstanceIdProps type. But I don't mind either way.

To make documentation clearer, I would also rename the HOC type to PropInjectingHigherOrderComponent. Before it's too late after the NPM package is published and backward compatibility becomes an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW we don't fully export the type from the package so there's no concern about change it immediately, but I do like that name better.

< TProps extends { instanceId: string | number } >(
WrappedComponent: ComponentType< TProps >
) => {
return ( props: Omit< TProps, 'instanceId' > ) => {
const instanceId = useInstanceId( WrappedComponent );
return (
<WrappedComponent
{ ...( props as TProps ) }
instanceId={ instanceId }
/>
);
};
},
'withInstanceId'
);

export default withInstanceId;
14 changes: 9 additions & 5 deletions packages/compose/src/hooks/use-instance-id/index.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
// Disable reason: Object and object are distinctly different types in TypeScript and we mean the lowercase object in thise case
// but eslint wants to force us to use `Object`. See https://stackoverflow.com/questions/49464634/difference-between-object-and-object-in-typescript
/* eslint-disable jsdoc/check-types */
/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';

/**
* @type {WeakMap<any, number>}
* @type {WeakMap<object, number>}
*/
const instanceMap = new WeakMap();

/**
* Creates a new id for a given object.
*
* @param {unknown} object Object reference to create an id for.
* @param {object} object Object reference to create an id for.
* @return {number} The instance id (index).
*/
function createId( object ) {
Expand All @@ -23,9 +26,9 @@ function createId( object ) {
/**
* Provides a unique instance ID.
*
* @param {Object} object Object reference to create an id for.
* @param {string} prefix Prefix for the unique id.
* @param {string} preferredId Default ID to use.
* @param {object} object Object reference to create an id for.
* @param {string} [prefix] Prefix for the unique id.
* @param {string} [preferredId=''] Default ID to use.
* @return {string | number} The unique instance id.
*/
export default function useInstanceId( object, prefix, preferredId = '' ) {
Expand All @@ -36,3 +39,4 @@ export default function useInstanceId( object, prefix, preferredId = '' ) {
return prefix ? `${ prefix }-${ id }` : id;
}, [ object ] );
}
/* eslint-enable jsdoc/check-types */
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ export type SimpleHigherOrderComponent = < TProps >(
Inner: ComponentType< TProps >
) => ComponentType< TProps >;

export type PropInjectingHigherOrderComponent< TRemovedProps > = <
TProps extends TRemovedProps
>(
Inner: ComponentType< TProps >
) => ComponentType< Omit< TProps, keyof TRemovedProps > >;

/**
* Given a function mapping a component to an enhanced component and modifier
* name, returns the enhanced component augmented with a generated displayName.
Expand Down
2 changes: 2 additions & 0 deletions packages/compose/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
"include": [
"src/higher-order/if-condition/**/*",
"src/higher-order/pure/**/*",
"src/higher-order/with-instance-id/**/*",
"src/hooks/use-instance-id/**/*",
"src/utils/**/*"
]
}