[Typescript] Add category related types from DefinitelyTyped to the blocks package. #43686
Conversation
ryanwelcher
added
[Package] Blocks
|
I'd rather not separate types from the implementation to the point where they're in a separate directory. People will easily forget about updating them. Also, you don't need
|
Thanks for guidance here @adamziel ! I've updated the PR as you indicated. Please let me know if there is anything I should update. |
|
Size Change: 0 B Total Size: 1.24 MB ℹ️ View Unchanged
|
| export interface WPBlockCategory { | ||
| slug: string; | ||
| title: string; | ||
| icon?: JSX.Element | Dashicon.Icon | null | undefined; |
There was a problem hiding this comment.
The interesting part is the usage of Dashicon type from @wodrpress/components. It probably should be IconKey as defined in https://unpkg.com/browse/@wordpress/[email protected]/build-types/dashicon/types.d.ts. Although, it creates a dependency on the @wordpress/components package that we should rather avoid in @wordpress/blocks. I'm curious if we have any package with shared types and whether we could move the list of Dashicon names there.
There was a problem hiding this comment.
+1 for avoiding the dependency on @wordpress/components
There was a problem hiding this comment.
cc @mirka who's been also recently taking a look at Dashicons
There was a problem hiding this comment.
A couple things:
@wordpress/components currently does not officially "ship" its TypeScript type data with the package, so it is basically inaccessible outside of the components package itself. (Long story 😅)
So, any kind of import type { SomeType } from '@wordpress/components' or import { SomeComponent } from '@wordpress/component' is not going to come associated with type data. If you do see types in your IDE, it's likely due to your IDE auto-downloading types from DefinitelyTyped as a fallback.
Here is a test snippet to verify that type checking is working as expected:
setCategories( [
{
icon: 'INVALID ICON!' /* this should error */,
title: 'foo',
slug: 'bar',
},
] );As one of our priorities, we're actively working towards removing the blockers so we can ship the types with the components package. (ETA is still on the order of months though — maybe by end of year if we cut some corners.)
@gziolo Once ^ is complete, I believe types can be imported with import type { ... } which would make it a dev dependency. Would that be acceptable?
(I have no opinion on this PR, just wanted to add some context on what's going on in the components package side.)
There was a problem hiding this comment.
@mirka, development dependencies don't get installed together with the package. So this would create friction for people consuming @wordpress/blocks from npm. That's why we discussed how this type could be present without bringing the whole @wordpress/components package and all its dependencies to the project.
I believe the simplest way to move forward, for the time being, is to copy the type and inline it in the package.
In the long run, my preferred approach would be to deprecate dashicons and list all the possible icon names from the @wordpress/icons package as an option for blocks. The challenge is that we need a wrapper component that could dynamically resolve the string provided to the actual icon like dashicon does it today.
There was a problem hiding this comment.
Gotcha, the Dashicon deprecation problem recently reappeared on our radar (#43725) as well. We'll have to see how feasible the dynamic loading wrapper is. Riad has floated this idea before, too.
I believe the simplest way to move forward, for the time being, is to copy the type and inline it in the package.
Agreed, and it shouldn't cause maintenance issues since the list of Dashicons aren't going to change anymore.
So this would create friction for people consuming
@wordpress/blocksfrom npm. That's why we discussed how this type could be present without bringing the whole@wordpress/componentspackage and all its dependencies to the project.
Interesting, thanks for explaining. I guess it isn't worth creating a dependency if the dashicon keys are the only things that wp-blocks needs to be re-exported from wp-components (or maybe in the future, from wp-icons).
| @@ -57,7 +62,7 @@ export function getCategories() { | |||
| * }; | |||
| * ``` | |||
| */ | |||
| export function setCategories( categories ) { | |||
| export function setCategories( categories: readonly WPBlockCategory[] ): void { | |||
There was a problem hiding this comment.
Do we need the void return type explicitly here? It's quite easy for TypeScript to infer it, so that we can avoid some noise.
|
@ryanwelcher, it looks like you don't work on this PR anymore. Should we focus instead on #48604 that @johnhooks opened after conversation with you? |
Yes, I'll defer to the work being done on #48604. I don't have the availability ( as much as I'd like to 😢 ) to work on this now and am happy to have @johnhooks take over. I'll close this out. |
What?
This PR adds the category-related types created in wordpress__blocks to the package.
Why?
Having types available in the package itself would be preferred over having to install an external one. DefinitelyTyped relies on a small number of contributors ( A HUGE THANK YOU TO THEM ) to ensure that the types are updated for each release and having them in the repository would allow that responsibility to be shared and keep them up to date.
How?
The types have been copied over from the external repo with the only difference being changing the name to match the JSDoc @typedef for
WPBlockCategory