-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
/** | ||
* Editor block category options. | ||
* | ||
* @typedef {('common'|'formatting'|'layout'|'widgets'|'embed')} WPBlockCategories |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
* @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 |
There was a problem hiding this comment.
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)
* @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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start to me! Nice work @chancestrickland!
/** | ||
* Editor block category options. | ||
* | ||
* @typedef {Object.<string, WPBlockAttributeOptions>} WPBlockAttributes |
There was a problem hiding this comment.
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
*/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>} |
* @param {Object<string,Object>} state Current state. |
* @return {Object<string,number>} Column widths. |
* @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. |
* @param {Object<string,string>} eventTypesToHandlers Object with keys of DOM |
* @type {Object<string,string>} |
* @type {Object<string,MediaQueryList>} |
There was a problem hiding this comment.
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)
/** | ||
* Editor block category options. | ||
* | ||
* @typedef {('common'|'formatting'|'layout'|'widgets'|'embed')} WPBlockCategories |
There was a problem hiding this comment.
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.
* developer can target the class | ||
* name for the style variation | ||
* if it is selected. | ||
* @property {Function} edit TODO: ... |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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!
Two surface-level observations:
In the meantime, I'll give a look over the specific changes. |
* | ||
* @typedef {(string|WPElement|WPComponent)} WPBlockTypeIconRender | ||
* @callback FnEditBlock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
...and has Intellisense for the result:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { /* ... */ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 whensource
is one of the HTML-based sources - A value for
attribute
is required whensource: 'attribute'
- A value for
meta
is required whensource: 'meta'
/** | ||
* Editor block alignment options. | ||
* | ||
* @typedef {('left' | 'center' | 'right' | 'wide' | 'full')} WPBlockAlign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 returnsundefined
.
https://developers.google.com/closure/compiler/docs/js-for-compiler#tag-return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
*/ | ||
|
||
/** | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a description?
*/ | ||
|
||
/** | ||
* Editor block category options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I understand there may not be a desire or availability to continue work here, and at the time some of the points around specific syntaxes were still rather unclear in the project. That said, I would very much like to see someone pick up this effort again in the future, especially if it can be integrated into documentation to help clarify available options. There's been quite a bit of work since this was originally opened to improve on how we author and use type definitions:
|
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 declaringFunction
!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.