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

compose: Add types to useRefEffect and clipboard hooks #31603

Merged
merged 7 commits into from
May 27, 2021
+70 −28
Merged

compose: Add types to useRefEffect and clipboard hooks #31603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b4e9c8
compose: Add types to useRefEffect and clipboard hooks
sarayourfriend May 7, 2021
2d61794
Use `RefCallback` type
sarayourfriend May 17, 2021
508b1af
Node can be null
sarayourfriend May 21, 2021
b2ce994
Use HTMLElement for useRefCallback
sarayourfriend May 25, 2021
32e3c94
compose: Make `useRefEffect` generic; also fix docgen for default exp…
sarayourfriend May 25, 2021
1a4f6ef
Update CHANGELOG for docgen
sarayourfriend May 25, 2021
6f3c3ce
Mark timeout as optional
sarayourfriend May 26, 2021
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
6 changes: 6 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
"@testing-library/react": "11.2.2",
"@testing-library/react-native": "7.1.0",
"@types/classnames": "2.2.10",
"@types/clipboard": "2.0.1",
"@types/eslint": "6.8.0",
"@types/estree": "0.0.44",
"@types/highlight-words-core": "1.2.0",
Expand Down
14 changes: 7 additions & 7 deletions packages/compose/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,9 @@ Copies the text to the clipboard when the element is clicked.

_Parameters_

- _ref_ `Object`: Reference with the element.
- _ref_ `import('react').RefObject<string | Element | NodeListOf<Element>>`: Reference with the element.
- _text_ `string|Function`: The text to copy.
- _timeout_ `number`: Optional timeout to reset the returned state. 4 seconds by default.
- _timeout_ `[number]`: Optional timeout to reset the returned state. 4 seconds by default.

_Returns_

Expand All @@ -180,12 +180,12 @@ Copies the given text to the clipboard when the element is clicked.

_Parameters_

- _text_ `text|Function`: The text to copy. Use a function if not already available and expensive to compute.
- _text_ `string | (() => string)`: The text to copy. Use a function if not already available and expensive to compute.
- _onSuccess_ `Function`: Called when to text is copied.

_Returns_

- `RefObject`: A ref to assign to the target element.
- `import('react').Ref<HTMLElement>`: A ref to assign to the target element.

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

Expand Down Expand Up @@ -398,12 +398,12 @@ callback will be called multiple times for the same node.

_Parameters_

- _callback_ `Function`: Callback with ref as argument.
- _dependencies_ `Array`: Dependencies of the callback.
- _callback_ `( node: TElement ) => ( () => void ) | undefined`: Callback with ref as argument.
- _dependencies_ `DependencyList`: Dependencies of the callback.

_Returns_

- `Function`: Ref callback.
- `RefCallback< TElement | null >`: Ref callback.

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

Expand Down
20 changes: 15 additions & 5 deletions packages/compose/src/hooks/use-copy-on-click/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,32 +9,40 @@ import Clipboard from 'clipboard';
import { useRef, useEffect, useState } from '@wordpress/element';
import deprecated from '@wordpress/deprecated';

/* eslint-disable jsdoc/no-undefined-types */
/**
* Copies the text to the clipboard when the element is clicked.
*
* @deprecated
*
* @param {Object} ref Reference with the element.
* @param {string|Function} text The text to copy.
* @param {number} timeout Optional timeout to reset the returned
* @param {import('react').RefObject<string | Element | NodeListOf<Element>>} ref Reference with the element.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we import from @wordpress/element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wordpress/element doesn't export all the types, just the functions. We import the types directly from react everywhere else as well so it's just following the already established pattern.

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer importing the types in a comment above to leave more room for parameter descriptions

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think there's lots of places where we import from @wordpress/element. Why don't we export the patterns there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Frankly there are a lot of types to export if we were to export them from @wordpress/element. This would increase the maintenance cost of that already quite non-standard package. We've also already accepted importing them directly from react in this case, so it would be a departure from what's already been going on.

I think @sirreal had some opinions about this that he might want to chime in with.

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 also prefer importing the types in a comment above to leave more room for parameter descriptions

Doing this pollutes the exported types of a module, which is why I avoid it.

Copy link
Member

Choose a reason for hiding this comment

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

Re-exporting React types from @wordpress/element was discussed here: #21767

From memory:

  • Re-exporting the types in JSDoc is extremely costly. Remember that we'd need to completely reproduce in JSDoc anything that accepts a type argument, e.g.
  • We could re-evaluate that now that some TypeScript language is allowed. However, I don't believe we have a "full" re-export of React, so we'd still need to re-export every single type we want.
  • Using types from React is "free". I don't see downsides to using it for the types especially considering the costs of the alternative.

* @param {string|Function} text The text to copy.
* @param {number} [timeout] Optional timeout to reset the returned
* state. 4 seconds by default.
*
* @return {boolean} Whether or not the text has been copied. Resets after the
* timeout.
*/
export default function useCopyOnClick( ref, text, timeout = 4000 ) {
/* eslint-enable jsdoc/no-undefined-types */
deprecated( 'wp.compose.useCopyOnClick', {
since: '10.3',
plugin: 'Gutenberg',
alternative: 'wp.compose.useCopyToClipboard',
} );

/** @type {import('react').MutableRefObject<Clipboard | undefined>} */
const clipboard = useRef();
const [ hasCopied, setHasCopied ] = useState( false );

useEffect( () => {
/** @type {number | undefined} */
let timeoutId;

if ( ! ref.current ) {
return;
}

// Clipboard listens to click events.
clipboard.current = new Clipboard( ref.current, {
text: () => ( typeof text === 'function' ? text() : text ),
Expand All @@ -48,7 +56,7 @@ export default function useCopyOnClick( ref, text, timeout = 4000 ) {

// Handle ClipboardJS focus bug, see https://github.com/zenorocha/clipboard.js/issues/680
if ( trigger ) {
trigger.focus();
/** @type {HTMLElement} */ ( trigger ).focus();
Copy link
Member

Choose a reason for hiding this comment

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

I know this has been merged already, but just curious: why did you wrap trigger in parenthesis here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's how you do a type cast of a variable in JSDoc flavored TypeScript: microsoft/TypeScript#5158

}

if ( timeout ) {
Expand All @@ -59,7 +67,9 @@ export default function useCopyOnClick( ref, text, timeout = 4000 ) {
} );

return () => {
clipboard.current.destroy();
if ( clipboard.current ) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we pull clipboard this inside the effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I'm not sure what you are suggesting 😞

Copy link
Member

Choose a reason for hiding this comment

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

It seems clipboard doesn't need to be a ref and can be pulled into useEffect, but maybe that's out of scope for this PR. :)

Copy link
Member

@ellatrix ellatrix May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The good thing would be that you wouldn't have to check if it exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh gotcha. Let me tinker around with it and see if I can get it to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, given it's actually deprecated I feel weird trying to refactor it 😅 Do you think it's still worth it?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, it's just a generally suggestion. It's totally fine to leave it :)

clipboard.current.destroy();
}
clearTimeout( timeoutId );
};
}, [ text, timeout, setHasCopied ] );
Expand Down
21 changes: 12 additions & 9 deletions packages/compose/src/hooks/use-copy-to-clipboard/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ import { useRef } from '@wordpress/element';
*/
import useRefEffect from '../use-ref-effect';

/** @typedef {import('@wordpress/element').RefObject} RefObject */

/**
* @template T
* @param {T} value
* @return {import('react').RefObject<T>} The updated ref
*/
function useUpdatedRef( value ) {
const ref = useRef( value );
ref.current = value;
Expand All @@ -24,24 +27,24 @@ function useUpdatedRef( value ) {
/**
* Copies the given text to the clipboard when the element is clicked.
*
* @param {text|Function} text The text to copy. Use a function if not
* @param {string | (() => string)} text The text to copy. Use a function if not
* already available and expensive to compute.
* @param {Function} onSuccess Called when to text is copied.
* @param {Function} onSuccess Called when to text is copied.
*
* @return {RefObject} A ref to assign to the target element.
* @return {import('react').Ref<HTMLElement>} A ref to assign to the target element.
*/
export default function useCopyToClipboard( text, onSuccess ) {
// Store the dependencies as refs and continuesly update them so they're
// fresh when the callback is called.
const textRef = useUpdatedRef( text );
const onSuccesRef = useUpdatedRef( onSuccess );
const onSuccessRef = useUpdatedRef( onSuccess );
return useRefEffect( ( node ) => {
// Clipboard listens to click events.
const clipboard = new Clipboard( node, {
text() {
return typeof textRef.current === 'function'
? textRef.current()
: textRef.current;
: textRef.current || '';
},
} );

Expand All @@ -54,8 +57,8 @@ export default function useCopyToClipboard( text, onSuccess ) {
// https://github.com/zenorocha/clipboard.js/issues/680
node.focus();

if ( onSuccesRef.current ) {
onSuccesRef.current();
if ( onSuccessRef.current ) {
onSuccessRef.current();
}
} );

Expand Down
21 changes: 15 additions & 6 deletions ...compose/src/hooks/use-ref-effect/index.js → ...compose/src/hooks/use-ref-effect/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { DependencyList, RefCallback } from 'react';

/**
* WordPress dependencies
*/
Expand All @@ -17,14 +23,17 @@ import { useCallback, useRef } from '@wordpress/element';
* to be removed. It *is* necessary if you add dependencies because the ref
* callback will be called multiple times for the same node.
*
* @param {Function} callback Callback with ref as argument.
* @param {Array} dependencies Dependencies of the callback.
* @param callback Callback with ref as argument.
* @param dependencies Dependencies of the callback.
*
* @return {Function} Ref callback.
* @return Ref callback.
*/
export default function useRefEffect( callback, dependencies ) {
const cleanup = useRef();
return useCallback( ( node ) => {
export default function useRefEffect< TElement = Node >(
callback: ( node: TElement ) => ( () => void ) | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

@sarayourfriend: can we use EffectCallback now? Like:

callback: ( node: TElement ) => EffectCallback,

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 don't believe so because that would imply that the callback's return is also able to return a desctructor function which will go one level too deep I think?

Here's the definition for EffectCallback:

type EffectCallback = () => (void | (() => void | undefined));

If we used EffectCallback then the cleanup function, which is the current optional return of the callback parameter, would itself also be allowed to return a function, but we completely ignore the return type of the cleanup function.

If EffectCallback was a generic type that allowed us to add an input parameter type, then we could use it like this:

callback: EffectCallback<TElement>

But effects take no input for their callback so that will never be the case.

I could be confused here though, let me know if you're seeing something I'm not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you're totally right, I was the one confused. I misread some type definitions in a different module and thought we were missing an opportunity to use a predefined type. Thanks for humouring me. :)

dependencies: DependencyList
): RefCallback< TElement | null > {
const cleanup = useRef< ( () => void ) | undefined >();
return useCallback( ( node: TElement | null ) => {
if ( node ) {
cleanup.current = callback( node );
} else if ( cleanup.current ) {
Expand Down
3 changes: 3 additions & 0 deletions packages/compose/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@
"src/hooks/use-async-list/**/*",
"src/hooks/use-constrained-tabbing/**/*",
"src/hooks/use-debounce/**/*",
"src/hooks/use-copy-on-click/**/*",
"src/hooks/use-copy-to-clipboard/**/*",
"src/hooks/use-focus-return/**/*",
"src/hooks/use-ref-effect/**/*",
"src/hooks/use-instance-id/**/*",
"src/hooks/use-isomorphic-layout-effect/**/*",
"src/hooks/use-keyboard-shortcut/**/*",
Expand Down
4 changes: 4 additions & 0 deletions packages/docgen/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Bug Fixes

- Fix getting param annotations for default exported functions. ([#31603](https://github.com/WordPress/gutenberg/pull/31603))

## 1.17.0 (2021-04-29)

### New Features
Expand Down
8 changes: 7 additions & 1 deletion packages/docgen/lib/get-type-annotation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ function getFunctionTypeAnnotation( typeAnnotation, returnIndicator ) {
typeAnnotation.typeAnnotation.typeAnnotation
);

return `( ${ params } )${ returnIndicator }${ returnType }`;
const paramsWithParens = params.length ? `( ${ params } )` : `()`;

return `${ paramsWithParens }${ returnIndicator }${ returnType }`;
}

/**
Expand Down Expand Up @@ -376,6 +378,10 @@ function getTypeAnnotation( typeAnnotation ) {
*/
function getFunctionToken( token ) {
let resolvedToken = token;
if ( babelTypes.isExportDefaultDeclaration( resolvedToken ) ) {
resolvedToken = resolvedToken.declaration;
}

if ( babelTypes.isExportNamedDeclaration( resolvedToken ) ) {
resolvedToken = resolvedToken.declaration;
}
Expand Down