compose: Add types to useRefEffect and clipboard hooks #31603
Conversation
sarayourfriend
added
[Type] Code Quality
|
Size Change: +5 B (0%) Total Size: 1.86 MB
ℹ️ View Unchanged
|
| * @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. |
There was a problem hiding this comment.
Shouldn't we import from @wordpress/element?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I also prefer importing the types in a comment above to leave more room for parameter descriptions
There was a problem hiding this comment.
Ah, I think there's lots of places where we import from @wordpress/element. Why don't we export the patterns there?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -59,7 +67,9 @@ export default function useCopyOnClick( ref, text, timeout = 4000 ) { | |||
| } ); | |||
|
|
|||
| return () => { | |||
| clipboard.current.destroy(); | |||
| if ( clipboard.current ) { | |||
There was a problem hiding this comment.
Could we pull clipboard this inside the effect?
There was a problem hiding this comment.
Sorry I'm not sure what you are suggesting 😞
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
The good thing would be that you wouldn't have to check if it exists
There was a problem hiding this comment.
Ahhh gotcha. Let me tinker around with it and see if I can get it to work.
There was a problem hiding this comment.
Well, given it's actually deprecated I feel weird trying to refactor it 😅 Do you think it's still worth it?
There was a problem hiding this comment.
I don't have a strong opinion on this, it's just a generally suggestion. It's totally fine to leave it :)
| * | ||
| * @return {Function} Ref callback. | ||
| * @return {(node: Node) => void} Ref callback. |
There was a problem hiding this comment.
There's a RefCallback type for this I think.
There was a problem hiding this comment.
RefCallback isn't generic though and in this case we want specifically to return the Node => void function, not just any old callback function.
If you remove the return type annotation and inspect the inferred type of the return from this function you'll see that it's what's written there.
There was a problem hiding this comment.
not just any old callback function
What do you mean?
There was a problem hiding this comment.
Ah I see what you meant now, I guess we can use the RefCallback type in this case... but why would we when (node: Node) => void is more straightforward to read and matches the literal return type of useCallback in this case?
There was a problem hiding this comment.
Ah, I just thought is was easier to use RefCallback, because it's a common thing. How do we add that node can be null too?
There was a problem hiding this comment.
Done! You can do that just by passing Node | null to RefCallback.
sarayourfriend
force-pushed
the
add/types-compose-copy
branch
3 times, most recently
from
eaab6c1 to
82d13e5
Compare
| @@ -37,11 +44,11 @@ export default function useCopyToClipboard( text, onSuccess ) { | |||
| const onSuccesRef = useUpdatedRef( onSuccess ); | |||
| return useRefEffect( ( node ) => { | |||
| // Clipboard listens to click events. | |||
| const clipboard = new Clipboard( node, { | |||
| const clipboard = new Clipboard( /** @type {Element} */ ( node ), { | |||
There was a problem hiding this comment.
Is the cast from Node to Element safe?
There was a problem hiding this comment.
Actually I think the type of node should just be HTMLElement to begin with, making it unnecessary.
There was a problem hiding this comment.
Okay actually I went ahead and made useRefEffect generic, defaulting to Node but making it possible to make it HTMLElement for this function. This should be the safest option 🙂
|
@ciampo this PR now also includes a change to |
sarayourfriend
added
[Tool] Docgen
sarayourfriend
force-pushed
the
add/types-compose-copy
branch
from
e83f9e4 to
628b096
Compare
| * @param {number} timeout Optional timeout to reset the returned | ||
| * @param {import('react').RefObject<string | Element | NodeListOf<Element>>} ref Reference with the element. | ||
| * @param {string|Function} text The text to copy. | ||
| * @param {number} timeout Optional timeout to reset the returned |
There was a problem hiding this comment.
If it's optional, should it become [timeout] ?
There was a problem hiding this comment.
Apart from this last pending comment, this PR LGTM!
this PR now also includes a change to docgen to fix an issue with default exports 😞
Just to confirm, I ran npx docgen packages/compose/src/hooks/use-ref-effect/index.ts and successfully generated the index-api.md file. Was this the way of testing the docgen fix?
Sure that's one way. |
sarayourfriend
force-pushed
the
add/types-compose-copy
branch
from
628b096 to
a45dd1d
Compare
sarayourfriend
force-pushed
the
add/types-compose-copy
branch
from
5bdb77f to
74f22f7
Compare
sarayourfriend
force-pushed
the
add/types-compose-copy
branch
from
74f22f7 to
6f3c3ce
Compare
| @@ -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(); | |||
There was a problem hiding this comment.
I know this has been merged already, but just curious: why did you wrap trigger in parenthesis here?
There was a problem hiding this comment.
It's how you do a type cast of a variable in JSDoc flavored TypeScript: microsoft/TypeScript#5158
| const cleanup = useRef(); | ||
| return useCallback( ( node ) => { | ||
| export default function useRefEffect< TElement = Node >( | ||
| callback: ( node: TElement ) => ( () => void ) | undefined, |
There was a problem hiding this comment.
@sarayourfriend: can we use EffectCallback now? Like:
callback: ( node: TElement ) => EffectCallback,There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
Description
Adds types to the useRefEffect, useCopyOnClick and useCopyToClipboard hooks.
Only minimal runtime changes were necessary, but some are kind of scary so we'll want to make sure to test this correctly.
How has this been tested?
Go to the block settings dropdown menu and ensure that copying the menu items works.
Screenshots
Types of changes
New feature (types!)
Checklist:
*.native.jsfiles for terms that need renaming or removal).