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

[Typescript] Add category related types from DefinitelyTyped to the blocks package. #43686

Closed
wants to merge 6 commits into from
Closed

[Typescript] Add category related types from DefinitelyTyped to the blocks package. #43686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f11b3b7
Add the types property to package.json.
ryanwelcher Aug 2, 2022
4ef74b9
types directory and add files to match structure of src.
ryanwelcher Aug 2, 2022
76b9d51
Import WPBlockCategory type from types/api
ryanwelcher Aug 2, 2022
29d3d14
Merge branch 'trunk' into feature/add-block-category-types
ryanwelcher Aug 29, 2022
2c41899
Merge branch 'trunk' into feature/add-block-category-types
ryanwelcher Aug 30, 2022
3cfaae0
Remove types directory and move all types into categories.ts.
ryanwelcher Aug 30, 2022
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
5 changes: 3 additions & 2 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 packages/blocks/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"@wordpress/autop": "file:../autop",
"@wordpress/blob": "file:../blob",
"@wordpress/block-serialization-default-parser": "file:../block-serialization-default-parser",
"@wordpress/components": "file:../components",
"@wordpress/data": "file:../data",
"@wordpress/deprecated": "file:../deprecated",
"@wordpress/dom": "file:../dom",
Expand Down
16 changes: 12 additions & 4 deletions packages/blocks/src/api/categories.js → packages/blocks/src/api/categories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,18 @@
* WordPress dependencies
*/
import { dispatch, select } from '@wordpress/data';
import { Dashicon } from '@wordpress/components';

/**
* Internal dependencies
*/
import { store as blocksStore } from '../store';

/** @typedef {import('../store/reducer').WPBlockCategory} WPBlockCategory */
export interface WPBlockCategory {
slug: string;
title: string;
icon?: JSX.Element | Dashicon.Icon | null | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for avoiding the dependency on @wordpress/components

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @mirka who's been also recently taking a look at Dashicons

Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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/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.

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).

}

/**
* Returns all the block categories.
Expand All @@ -18,7 +23,7 @@ import { store as blocksStore } from '../store';
*
* @return {WPBlockCategory[]} Block categories.
*/
export function getCategories() {
export function getCategories(): WPBlockCategory[] {
return select( blocksStore ).getCategories();
}

Expand Down Expand Up @@ -57,7 +62,7 @@ export function getCategories() {
* };
* ```
*/
export function setCategories( categories ) {
export function setCategories( categories: readonly WPBlockCategory[] ): void {
Copy link

Choose a reason for hiding this comment

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

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.

dispatch( blocksStore ).setCategories( categories );
}

Expand Down Expand Up @@ -87,6 +92,9 @@ export function setCategories( categories ) {
* };
* ```
*/
export function updateCategory( slug, category ) {
export function updateCategory(
slug: string,
category: Partial< WPBlockCategory >
): void {
dispatch( blocksStore ).updateCategory( slug, category );
}