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

Data: Document WPDataRegistry properties #16693

Merged
merged 4 commits into from Jul 26, 2019

Conversation

@aduth
Copy link
Member

commented Jul 20, 2019

Extracted from #16761

This pull request seeks to improve the type usage in the data module to use the custom WPDataRegistry type more consistently, and to add proper descriptions for the data registry properties.

Testing Instructions:

This impacts only JSDoc code comments and thus should have no impact on runtime operation.

@nerrad

nerrad approved these changes Jul 20, 2019

@dsifford
Copy link
Contributor

left a comment

Just a few thoughts. I assume this is temporary until typescript checking makes its way here, so not super important.

In a nutshell: The types Object, Array, and Function = 😢 😭

* @param {Object} options Registered store options, with properties
* describing reducer, actions, selectors, and
* resolvers.
* @param {WPDataRegistry} registry Registry reference.

This comment has been minimized.

Copy link
@dsifford

dsifford Jul 21, 2019

Contributor

There's no way for editors to know how to interpret this without importing it in the file as its own typedef.

This comment has been minimized.

Copy link
@aduth

aduth Jul 24, 2019

Author Member

There's no way for editors to know how to interpret this without importing it in the file as its own typedef.

Hm, is this what the import is used for in your own pull requests? How does that work with @typedef (vs. your TypeScript definitions)?

This comment has been minimized.

Copy link
@dsifford

dsifford Jul 24, 2019

Contributor

Precisely.

In javascript, in order to reference types that are defined elsewhere (be it in a typescript definition file, or as standard JSDoc), the types must first be imported in the referencing file as a @typedef. Once imported, you can freely reference the type throughout that file.

Otherwise the editor will not be able to provide type hints or "intellisense" (in VS editors)

* @param {Object} options Contains reducer, actions, selectors, and resolvers.
* @param {Object} registry Registry reference.
* @param {string} key Unique namespace identifier.
* @param {Object} options Registered store options, with properties

This comment has been minimized.

Copy link
@dsifford

dsifford Jul 21, 2019

Contributor

Type Object is not useful IMHO. I assume this is temporary until the typescript checking makes its way into this file, yes?

This comment has been minimized.

Copy link
@aduth

aduth Jul 24, 2019

Author Member

Type Object is not useful IMHO. I assume this is temporary until the typescript checking makes its way into this file, yes?

I'd say temporary, yes. I'm not really changing anything here aside from alignment and description. WPDataRegistry is the only type being changed throughout this pull request.

* @property {Function} registerStore Given a namespace key and settings
* object, registers a new namespace
* store.
* @property {Function} subscribe Given a function callback, invokes

This comment has been minimized.

Copy link
@dsifford

dsifford Jul 21, 2019

Contributor

Using this property as an example because it's the easiest one to do. IMHO, it would be better to do one of the following options since the type Function is not really helpful or descriptive.

Option 1

/**
 * @callback Subscriber Description of subscriber
 * @param {() => void} callback Description of callback parameter.
 * @return {void}
 */

Option 2

/**
 * @typedef {(callback: () => void) => void} Subscriber Description of subscriber
 */

And then use it on this line as...

/**
 * [...]
 * @prop {Subscriber} registerGenericStore Given a namespace key and settings
 *                                         object, registers a new generic store.
 * [...]
 */

I assume that this is also temporary though until typescript checking makes its way here. So not super important.

This comment has been minimized.

Copy link
@aduth

aduth Jul 24, 2019

Author Member

Are those syntaxes standard in JSDoc? It would be a good thing to detail in the documentation standards, I think.

In general, I agree with you. I didn't aim to change the types here, however.

This comment has been minimized.

Copy link
@dsifford

dsifford Jul 24, 2019

Contributor

Yes.

I agree that the documentation standards should be combed through for accuracy and, ideally, these standards should be linted automatically. Pretty sure eslint-plugin-jsdoc should be able to do the brunt of this.

* @property {Function} subscribe
* @property {Function} select
* @property {Function} dispatch
* @property {Function} registerGenericStore Given a namespace key and settings

This comment has been minimized.

Copy link
@dsifford

dsifford Jul 21, 2019

Contributor

I'd suggest making it the standard for this entire project (and all of WordPress, if possible) to use the tag @prop instead of @property to save on precious line lengths.

It'll help somewhat with the smushed descriptions.

This comment has been minimized.

Copy link
@aduth

aduth Jul 24, 2019

Author Member

It'll help somewhat with the smushed descriptions.

In some respect I appreciate the constraints to encourage being brief with the names and descriptions.

I don't have a strong feeling about @property vs. @prop, but it needs to be made in the documentation standards, which currently lists @prop as unsupported. A topic for a future JavaScript meeting, perhaps?

@aduth aduth merged commit 32b7b34 into master Jul 26, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the update/data-doc branch Jul 26, 2019

@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 29, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Data: Document WPDataRegistry properties (WordPress#16693)
* Data: Document WPDataRegistry properties

* Data: Use WPDataRegistry type where applicable

* Data: Fix WPDataRegistry typedef syntax

* Data: Bring WPDataRegistry into scope for typedef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.