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.

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

chore: add typedefs for registerBlockType #18257

Open
wants to merge 2 commits into
base: master
from

Conversation

@chancestrickland
Copy link

chancestrickland commented Nov 3, 2019

Description

Adding JSDoc typedefs for registerBlockType.

As I'm a new contributor, I would love some feedback on this before spending too much time on the items marked TODO. I'm also unsure if there is a set standard for documenting function properties in typedefs (re: edit/save, JSDoc isn't super clear on a standard here and userland suggestions seem to be all over the place). There has to be a way to be more specific than just declaring Function!

How has this been tested?

Checked against other typedefs in the project and my approach seems consistent and satisfies lint rules. Intellisense in VS Code works as expected.

Types of changes

Only JSDoc + a new .gitignore entry to avoid adding project-level editor settings.

@aduth aduth self-requested a review Nov 4, 2019
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 4, 2019

cc @dsifford , as I wonder if you might have some insight to add here with regard to some of the questions (function documentation, enumerables).

I haven't personally seen any pattern (in this project or elsewhere) for detailed function-type documentation in JSDoc.

.gitignore Outdated
@@ -11,6 +11,7 @@ gutenberg.zip
phpcs.xml
yarn.lock
/wordpress
.vscode

This comment has been minimized.

Copy link
@aduth

aduth Nov 4, 2019

Member

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

This comment has been minimized.

Copy link
@aduth

aduth Nov 4, 2019

Member

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

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

This comment has been minimized.

Copy link
@aduth

aduth Nov 4, 2019

Member

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.

This comment has been minimized.

Copy link
@chancestrickland

chancestrickland Nov 4, 2019

Author

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.

This comment has been minimized.

Copy link
@aduth

aduth Nov 4, 2019

Member

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.

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 5, 2019

Contributor

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

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

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

This comment has been minimized.

Copy link
@aduth

aduth Nov 4, 2019

Member

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.

This comment has been minimized.

Copy link
@chancestrickland

chancestrickland Nov 4, 2019

Author

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

Copy link
Contributor

dsifford left a comment

Looks like a good start to me! Nice work @chancestrickland!

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

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 5, 2019

Contributor

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

This comment has been minimized.

Copy link
@chancestrickland

chancestrickland Nov 5, 2019

Author

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.

This comment has been minimized.

Copy link
@aduth

aduth Nov 5, 2019

Member

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.

This comment has been minimized.

Copy link
@aduth

aduth Nov 5, 2019

Member

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

* @type {Object<string,Function>}

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

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

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

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

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

* @type {Object<string,string>}

* @type {Object<string,MediaQueryList>}

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 5, 2019

Contributor

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

(also I agree no dot is better)

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

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 5, 2019

Contributor

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

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

* developer can target the class
* name for the style variation
* if it is selected.
* @property {Function} edit TODO: ...

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 5, 2019

Contributor

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

(same for save below)

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 5, 2019

Contributor

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!

@chancestrickland chancestrickland marked this pull request as ready for review Nov 5, 2019
@chancestrickland

This comment has been minimized.

Copy link
Author

chancestrickland commented Nov 5, 2019

@aduth @dsifford I believe this should be ready for review. I went ahead and implemented most of the prior feedback, but let me know if I missed anything or if something looks off.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 8, 2019

Two surface-level observations:

  • Currently, the generated documentation resides in the repository, and needs to be re-generated when associated code comments are changed. Can you run npm run docs:build and commit the resulting changes? This will allow the failing "Build artifacts" Travis job to pass.
  • There is a conflict in the branch.

In the meantime, I'll give a look over the specific changes.

*
* @typedef {(string|WPElement|WPComponent)} WPBlockTypeIconRender
* @callback FnEditBlock

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

Hm, is it fair to call this a callback? It's a component, which in many cases will be something of a callback (function component), but not always. And while technically a Component class is a function, I'm not sure "callback" can apply to a value which needs to be constructed?

https://reactjs.org/docs/components-and-props.html

I suppose the main purpose here is in providing a type for the props it can expect to receive? Ultimately that seems to go back to the purpose propTypes is meant to serve. I could imagine if we adopted propTypes, we might be able to integrate this into the docs tooling to extract documentation for component props.

Then again, with the direct React is taking, we seem to be reaching a point where there's few reasons anyone should ever create a new component that's not a function component, in which case the documentation as you've proposed here might be the most reasonable approach. And above all, it would be good to have something available to document the expected props when used this way.

This comment has been minimized.

Copy link
@chancestrickland

chancestrickland Nov 8, 2019

Author

I would agree in theory, unfortunately I couldn't get JSDoc to understand using any other syntax for documenting functions and their arguments in a detailed manner. I tried @function and just adding @param tags to the typedefs but the parser was not satisfied AFAICT. Perhaps it's just a limitation we accept, use a generic Function and just explain the parameters and return type in the comment block?

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

I would agree in theory, unfortunately I couldn't get JSDoc to understand using any other syntax for documenting functions and their arguments in a detailed manner.

In a side project, I've had luck with the following:

/**
 * @typedef {function(SLAuth): Promise<SLStream[]>} SLProviderGetStreams
 */

(specifically the {function(SomeArgumentType): SomeReturnType} format)

The TypeScript checking verifies the parameter:

image

...and has Intellisense for the result:

image

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 8, 2019

Contributor

Anything between the brackets in a typedef is parsed identically to how typescript type definitions are parsed...

So if you have a function that looks like this...

function foo(someString) {
  return Promise.resolve(someString);
}

You could make a typedef like this...

/**
 * @typedef {(someString: string) => Promise<string>} FooFunc
 */

Or you could reference it inline like this...

/**
 * Function that takes foo as a parameter.
 * @prop {(someString: string) => Promise<string>} fooFunc
 */
function takeFooFunc(fooFunc) { /* ... */ }

This comment has been minimized.

Copy link
@chancestrickland

chancestrickland Nov 8, 2019

Author

That's good to know about TS compatibility, thanks! I did get arrow function notation to work with VS Code at least but not seeing a single example anywhere in the JSDoc spec made me nervous about committing to that approach 😅

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

Thanks @dsifford , I learned something new!

@chancestrickland : If it's compatible with (a) TypeScript, (b) eslint-plugin-jsdoc, and (c) our documentation generator†, I think it should be fine. I like in the alternate forms @dsifford presents that it gives an option to name the arguments (in my prior comment, it would show as arg0).

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 8, 2019

Contributor

@chancestrickland Yep, I guess I should have mentioned that the way I describe is not strictly aligned with the rules of jsdoc proper. But the editor features and code hints and all that good stuff that are parsed from these type definitions are done so using Microsoft's tsdoc standards, which is an extension of jsdoc.

So, I suppose it should also be mentioned that doing it this was would probably not pair well with a documentation generator that uses the vanilla jsdoc parser.

*
* @typedef {Object} WPBlockAttributeOptions
*
* @property {string} attribute Use attribute to extract the

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

Should we / do we need to document that all of these properties are optional?

For documentation, it could be useful to describe how they're associated, even if by types it might not be possible to enforce.

e.g.

  • A value for selector is optionally supported when source is one of the HTML-based sources
  • A value for attribute is required when source: 'attribute'
  • A value for meta is required when source: 'meta'
/**
* Editor block alignment options.
*
* @typedef {('left' | 'center' | 'right' | 'wide' | 'full')} WPBlockAlign

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

Per consistency with below, and some previous conversations about minimizing line length, I assume we might want to remove the spaces around |.

* @callback FnSetAttributes
*
* @param {WPBlockAttributes} attributes Attributes to set.
* @return {void}

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

FWIW, many conventions support omitting @return when the return value is undefined (including TypeScript?), and I think this is the general approach we've taken in the rest of the project.

If a function that is not in externs has no return value, you can omit the @return tag, and the compiler will assume that the function returns undefined.

https://developers.google.com/closure/compiler/docs/js-for-compiler#tag-return

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 8, 2019

Contributor

Just chiming in here for a bit of added distinction...

As far as typescript is concerned, a function that returns undefined must return either undefined or simply return; for all code paths.

A function that returns void may or may not return undefined somewhere, but does not need to return in all code paths.

*/

/**
*

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

Should there be a description?

*/

/**
* Editor block category options.

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

This description is inaccurate (I believe it's copy-pasted from the prior typedef).

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

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

I don't think all of these options are accurate?

'number', 'string', and 'integer' are not options for an attribute source, and should be removed.

Conversely, there's a few missing, though they're not any I think we should really be recommending: 'children', 'node', 'property', 'tag'.

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

This comment has been minimized.

Copy link
@aduth

aduth Nov 8, 2019

Member

Since a plugin or theme can add their own custom options, I think we can't assume it would be part of this limited set.

https://developer.wordpress.org/block-editor/developers/filters/block-filters/#managing-block-categories

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.