Skip to content

chore: add typedefs for registerBlockType #18257

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

Closed
chaance wants to merge 2 commits into from
Closed

chore: add typedefs for registerBlockType #18257

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ebfc0b
chore: add typedefs for registerBlockType
chaance Nov 3, 2019
8714780
chore: add more typedefs for registerBlockType
chaance Nov 5, 2019
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ gutenberg.zip
phpcs.xml
yarn.lock
/wordpress
.vscode
Copy link
Member

@aduth aduth Nov 4, 2019

Choose a reason for hiding this comment

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

Historically we've advocated that this be included in a developer's own global .gitignore to avoid a proliferation of user-specific entries in this file, but this has definitely come up on multiple occasions and I'm curious to potentially consider again (for developer experience' sake) whether to make exceptions at least for the most common cases.

cc @gziolo

Copy link
Member

@aduth aduth Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's a bit outside the scope of what you're trying to accomplish here, and not of huge consequence one way or the other, but perhaps a topic worth covering in tomorrow's weekly JavaScript chat in Slack.

Edit: I added a topic to the agenda


playground/dist
.cache
Expand Down
99 changes: 97 additions & 2 deletions packages/blocks/src/api/registration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,101 @@ import { select, dispatch } from '@wordpress/data';
import { isValidIcon, normalizeIconObject } from './utils';
import { DEPRECATED_ENTRY_KEYS } from './constants';

/**
* TODO: Editor block attributes object.
*
* @typedef {Object} WPBlockAttributeOptions
*
* @property {WPBlockAttributeType} type ...
* @property {string} source ...
* @property {string} selector ...
* @property {string} attribute ...
* @property {string} meta Attributes may be obtained from a post’s meta rather than from the block’s representation in saved post content. For this, an attribute is required to specify its corresponding meta key under the meta key
Copy link
Member

@aduth aduth Nov 4, 2019

Choose a reason for hiding this comment

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

As minor points of code style convention (not currently enforced at this level of detail):

  • Consider to wrap at 80-100 line length (reference guideline
  • Consider to align respective "sections" of a grouping (i.e. horizontally aligned types, names, descriptions within a @property grouping) (reference guideline, though it should be noted the document in general has fallen somewhat out of date)
Suggested change
* @property {string} meta Attributes may be obtained from a post’s meta rather than from the block’s representation in saved post content. For this, an attribute is required to specify its corresponding meta key under the meta key
* @property {string} meta Attributes may be obtained from post
* meta rather than from the block’s
* representation in saved post content.
* For this, an attribute is required to
* specify its corresponding meta key
* under the meta key.

Copy link
Author

@chaance chaance Nov 4, 2019

Choose a reason for hiding this comment

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

Noted, thanks! This entire block is marked as TODO but I will format it accordingly after adding all of the descriptions and correct types.

*/

/**
* Editor block category options.
*
* @typedef {Object.<string, WPBlockAttributeOptions>} WPBlockAttributes
Copy link
Contributor

@dsifford dsifford Nov 4, 2019

Choose a reason for hiding this comment

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

Is this type valid? I haven't seen any written like this before.

There are other variants of this that are valid so this may be a matter of style...

Examples:

// Using a `Record` type
/**
 * @typedef {Record<string, WPBlockAtributeOptions>} WPBlockAttributes
 */

// Using an indexed type
/**
 * @typedef { {[k: string]: WPBlockAttributeOptions} } WPBlockAttributes
 */

Copy link
Author

@chaance chaance Nov 5, 2019

Choose a reason for hiding this comment

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

https://jsdoc.app/tags-type.html > third row under syntax examples uses the same syntax. I don't have a style preference though so if we're using a different syntax elsewhere I am happy to conform.

Copy link
Member

@aduth aduth Nov 5, 2019

Choose a reason for hiding this comment

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

I find it to be pretty intuitive at it's proposed in this pull request, and it's a syntax I'm familiar with using in my own projects.

Copy link
Member

@aduth aduth Nov 5, 2019

Choose a reason for hiding this comment

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

Some prior art (omitting ., which upon reflection I am uncertain whether to be valid):

gutenberg/bin/packages/build-worker.js

Line 83 in 711f1f7

* @type {Object<string,Function>}

gutenberg/packages/block-editor/src/store/reducer.js

Line 1274 in 711f1f7

* @param {Object<string,Object>} state Current state.

gutenberg/packages/block-library/src/columns/utils.js

Line 87 in 711f1f7

* @return {Object<string,number>} Column widths.

gutenberg/packages/block-library/src/columns/utils.js

Line 106 in 711f1f7

* @return {Object<string,number>} Redistributed column widths.

gutenberg/packages/blocks/src/api/serializer.js

Lines 147 to 150 in 711f1f7

* @param {Object<string,*>} blockType Block type.
* @param {Object<string,*>} attributes Attributes from in-memory block data.
*
* @return {Object<string,*>} Subset of attributes for comment serialization.

gutenberg/packages/compose/src/higher-order/with-global-events/index.js

Line 32 in 711f1f7

* @param {Object<string,string>} eventTypesToHandlers Object with keys of DOM

gutenberg/packages/e2e-tests/config/setup-test-framework.js

Line 31 in 711f1f7

* @type {Object<string,string>}

gutenberg/packages/viewport/src/listener.js

Line 28 in 711f1f7

* @type {Object<string,MediaQueryList>}

Copy link
Contributor

@dsifford dsifford Nov 5, 2019

Choose a reason for hiding this comment

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

Cool. Looks good to me assuming the typescript compiler is cool with that.

(also I agree no dot is better)

*/

/**
* TODO: Editor block style object.
*
* @typedef {Object} WPBlockStyle
*
* @property {string} name Unique key name for the style definition.
* @property {string} label Label shown to the user in the editor.
* @property {boolean} isDefault Whether or not the style is selected by
* default.
*/

/**
* Editor block category options.
*
* @typedef {('common'|'formatting'|'layout'|'widgets'|'embed')} WPBlockCategories
Copy link
Member

@aduth aduth Nov 4, 2019

Choose a reason for hiding this comment

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

Do you have a reference for this syntax of defining options of a set that you can share?

I can't find anything about it on the @type documentation, and my experience with similar options like @enum are they aren't quite what we're looking for for what you're expressing here.

Copy link
Author

@chaance chaance Nov 4, 2019

Choose a reason for hiding this comment

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

I had to dig for it, but this came from jsdoc/jsdoc#629 (comment). So it's not documented as the "official" solution but it is parsed correctly, and this seemed more appropriate than an enum here.

Copy link
Member

@aduth aduth Nov 4, 2019

Choose a reason for hiding this comment

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

Makes sense to me!

The only thing I might suggest is that if a block is to be registered with a single category, it would make sense that this be singular "Category" as well, vs. "Categories"? Similar to WPBlockAttributeType and WPBlockAttributeSource.

Copy link
Contributor

@dsifford dsifford Nov 4, 2019

Choose a reason for hiding this comment

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

Yep, this is valid... String union type 👍

The surrounding parens aren't required though. But that's a matter of style.

*/

/**
* Editor block category options.
*
* @typedef {('null'|'boolean'|'object'|'array'|'number'|'string'|'integer')} WPBlockAttributeType
*/

/**
* Editor block category options.
*
* @typedef {('text'|'html'|'query'|'meta'|'number'|'string'|'integer')} WPBlockAttributeSource
*/

/**
* Editor block settings object.
*
* @typedef {Object} WPBlockSettings
*
* @property {string} title This is the display title for
* your block, which can be
* translated with our
* translation functions. The
* block inserter will show this
* name.
* @property {WPBlockCategories} [category] Blocks are grouped into
* categories to help users
* browse and discover them.
* @property {string[]} [keywords] Sometimes a block could have
* aliases that help users
* discover it while searching.
* For example, an `image` block
* could also want to be
* discovered by `photo`. You can
* do so by providing an array of
* terms (which can be
* translated).
* @property {WPBlockTypeIconRender} [icon] An icon property should be
* specified to make it easier to
* identify a block. These can be
* any of the WordPress
* Dashicons, or a custom `svg`
* element.
* @property {WPBlockAttributes} [attributes] Attributes provide the
* structured data needs of a
* block. They can exist in
* different forms when they are
* serialized, but they are
* declared together under a
* common interface.
* @property {WPBlockStyle[]} [style] Block styles can be used to
* provide alternative styles to
* block. It works by adding a
* class name to the block’s
* wrapper. Using CSS, a theme
* developer can target the class
* name for the style variation
* if it is selected.
* @property {Function} edit TODO: ...
Copy link
Contributor

@dsifford dsifford Nov 5, 2019

Choose a reason for hiding this comment

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

I see that this is marked todo, but see here for what I'd recommend typing this as....

(same for save below)

Copy link
Contributor

@dsifford dsifford Nov 5, 2019

Choose a reason for hiding this comment

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

I guess I should say that I do understand that generics aren't as well supported in JSDoc, but ComponentType<any> should work at the very least!

* @property {Function} save TODO: ...
*/

/**
* Render behavior of a block type icon; one of a Dashicon slug, an element,
* or a component.
Expand Down Expand Up @@ -110,8 +205,8 @@ export function unstable__bootstrapServerSideBlockDefinitions( definitions ) { /
* behavior. Once registered, the block is made available as an option to any
* editor interface where blocks are implemented.
*
* @param {string} name Block name.
* @param {Object} settings Block settings.
* @param {string} name Block name.
* @param {WPBlockSettings} settings Block settings.
*
* @return {?WPBlock} The block, if it has been successfully registered;
* otherwise `undefined`.
Expand Down