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
Merged

Data: Document WPDataRegistry properties #16693

merged 4 commits into from
Jul 26, 2019

Conversation

aduth
Copy link
Member

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

@aduth aduth added [Type] Developer Documentation Documentation for developers [Type] Code Quality Issues or PRs that relate to code quality [Package] Data /packages/data labels Jul 20, 2019
@aduth aduth requested review from nerrad and youknowriad as code owners July 20, 2019 10:08
@aduth aduth added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Jul 20, 2019
Copy link
Contributor

@dsifford dsifford left a comment

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@dsifford dsifford Jul 21, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
@aduth aduth deleted the update/data-doc branch July 26, 2019 14:55
@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Package] Data /packages/data [Type] Code Quality Issues or PRs that relate to code quality [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants