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

Docs: Add Gutenberg-specific JSDoc recommendations #18920

Merged
merged 12 commits into from Dec 5, 2019

Conversation

@aduth
Copy link
Member

aduth commented Dec 4, 2019

Related: https://make.wordpress.org/core/2019/12/04/javascript-chat-summary-december-3-2019/
Related: #18838

This pull request seeks to add a new section to the Coding Guidelines documentation, describing specific extensions of the WordPress JavaScript Documentation Standards relevant for Gutenberg's distinct use of import semantics in organizing files, the use of TypeScript tooling for types validation, and automated documentation generation using @wordpress/docgen.

Read the documentation

Open Questions:

There's a few things I chose not to include here, though might be relevant for additional detailing:

  • Clarifying distinctions between nullable, undefined, and optional parameter types, and between undefined and "void" returns (Added in ee70ae3)
  • Discouraging some of the core guidelines which are relevant mostly for older single-file scripts (file headers, etc)
  • Clarifying correctness around @see and @link tags documented in the core guidelines (I feel this should be a revision made to those core guidelines, upon clarification)
  • Capitalization guidelines around {Object} vs. {object}, etc. Likewise to the above, this should be a revision made to the core guidelines. The current guidelines are consistent with what is enforced in Gutenberg with preferredTypes, but the recommendation is not made explicit.

Testing Instructions:

As these changes only include edits to documentation, there is no expected impact on the application runtime.

@aduth aduth requested review from gziolo and dsifford Dec 4, 2019
@aduth aduth requested review from ajitbohra and chrisvanpatten as code owners Dec 4, 2019
Copy link
Member

gziolo left a comment

This is awesome, thanks for opening this PR with all the detailed guidelines. I'll take a closer look tomorrow 😍

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 4, 2019

Clarifying distinctions between nullable, undefined, and optional parameter types, and between undefined and "void" returns

I added a new section for this in ee70ae3.

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Dec 4, 2019

This looks all really good to me.

One small consistency thing I noticed is the use of double quotes in string unions in the docs, but single quotes everywhere else. Is there a standard here?

Examples...

```js
/**
* Named breakpoint sizes.
*
* @typedef {"huge"|"wide"|"large"|"medium"|"small"|"mobile"} WPBreakpoint
*/

```js
/** @typedef {import('@wordpress/data').WPDataRegistry} WPDataRegistry */
```

Other than that, really nice work!

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 4, 2019

@dsifford Good call. I think subconsciously the import felt more like writing code inline, hence why I might have opted for the single quote, vs. tending to use double-quotes as a default in documentation more generally.

I agree it's probably best to be consistent here. I'd probably lean toward the single quote. What do you think?

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Dec 4, 2019

I'm on team single-quote as well, but I'll leave that to you all with more skin in the game to make the final call.

@gziolo
gziolo approved these changes Dec 5, 2019
Copy link
Member

gziolo left a comment

I had some minor comments but overall I love the proposal. I don't know what are the next steps planned, but I would be fine with merging it as soon as possible. We should follow the guidelines while enabling TypeScript validation for packages and revisit all proposals once we encounter any issues in the actual usage.

docs/contributors/coding-guidelines.md Show resolved Hide resolved
docs/contributors/coding-guidelines.md Show resolved Hide resolved
docs/contributors/coding-guidelines.md Show resolved Hide resolved
docs/contributors/coding-guidelines.md Outdated Show resolved Hide resolved
docs/contributors/coding-guidelines.md Outdated Show resolved Hide resolved
@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 5, 2019

Thanks for the reviews! I think there might still be room for some further tweaking, but this should serve as a good starting-point I think. Let's get it in.

@aduth aduth merged commit c6643dc into master Dec 5, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the add/typescript-jsdoc-guidelines branch Dec 5, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
scruffian added a commit to scruffian/gutenberg that referenced this pull request Dec 10, 2019
* Docs: Add Gutenberg-specific JSDoc recommendations

* Docs: JSDoc Guidelines: Include section "Nullable, Undefined, and Void Types"

* Docs: JSDoc Guidelines: Simply phrasing around function components

* Docs: JSDoc Guidelines: Include example key of named string set

* Docs: JSDoc Guidelines: Use single quote for string literal set

* Docs: JSDoc: Prescribe use of single quotes

* Docs: JSDoc: Simplify language around exposing types

* Docs: JSDoc: Use "entry point" instead of "entrypoint"

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

* Docs: JSDoc: Simplify language around type union custom types

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

* Docs: JSDoc: Add reference to TypeScript function type syntax

* Docs: JSDoc: Avoid own violations of record type guidelines

* Docs: JSDoc: Correct BlockTitle example props usage

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
@aduth aduth mentioned this pull request Jan 15, 2020
9 of 69 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.