chore(@wordpress/dom): improve accuracy of jsdoc #16352

dsifford · 2019-06-28T16:05:37Z

Description

This PR simply improves the JSDoc in @wordpress/dom for better accuracy and, in some cases, better autocompletion in editors that support it.

How has this been tested?

No code was touched (aside from changing nomenclature in 1 locatoin). Nevertheless, unit tests ran and passed.

Screenshots

N/A

Types of changes

Docs

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

dsifford · 2019-06-28T16:07:09Z

packages/dom/src/dom.js

 /**
 * Replaces the given node with a new node with the given tag name.
 *
- * @param {Element}  node    The node to replace
- * @param {string}   tagName The new tag name.
+ * @template {keyof HTMLElementTagNameMap} T


This is the location of where the JSDoc will improve autocompletion and catch typos.

This is the location of where the JSDoc will improve autocompletion and catch typos.

But it's not valid JSDoc ?

Not valid per eslint I guess.

Where is the @template tag to be found? I don't see it anywhere in the standard.

It's an extension of jsdoc that is used by the javascript/typescript language service (LSP) that is used by most editors.

You can find more on it here

dsifford · 2019-06-28T16:07:29Z

packages/dom/README.md

-   _node_ `Element`: The node to replace
-   _tagName_ `string`: The new tag name.
+-   _node_ `Node`: The node to replace.
+-   _tagName_ `T`: The new tag name.


Not sure the documentation generator is prepared for this.

It looks like this is generated by this part of the JSDoc which is not to be found in the JSDoc standard.

I'l defer this to @aduth but I was under the impression that we were not following the core jsdoc standard to the letter, but instead the one that is picked up by the language service (i.e. tsdoc)

I think if we want to have any hope of achieving tsc types checking (#18838), we'll need to target the TypeScript syntax extensions. Even something such as sharing types across files (import) would not be straight-forward otherwise.

We'll need some clarity and guidelines around how this is rolled out, though. I've included it in the agenda for tomorrow's #core-js meeting:

https://docs.google.com/document/d/1TJ06NiyLzThsplXdEhya51C13WE62nHN71kt997hXHw/edit#heading=h.quypt74go9w8

For example, some of the more advanced syntax like intersection types (often used for makeshift type inheritence) are both explicitly unsupported in JSDoc and flagged as invalid syntax by eslint-plugin-jsdoc (more specifically, jsdoctypeparser, related issue). Some of this is rather open-ended on whether it should be parsed correctly [1] [2], but in the meantime we'll need to decide whether to use it, and how to work around potential interoperability issues.

Thanks for the context of where this is coming from. Though, if we want to merge this particular PR before the changes that need to happen to our linting/docs tooling, we need to use the JSDoc syntax at the moment and revisit later.

dsifford · 2019-06-28T16:08:09Z

packages/dom/src/dom.js

 */
-export function getScrollContainer( node ) {
-	if ( ! node ) {
+export function getScrollContainer( element ) {


Changed to element since it expects an Element and not a Node

Changed to element since it expects an Element and not a Node

Technically Element implements the Node interface, so every element is a node (reference). But for clarity's sake, I'd agree with your assessment 👍

Correct. But clientHeight and scrollHeight are only available on the Element interface so only an Element would be valid to pass into this function.

dsifford · 2019-06-28T16:08:52Z

packages/dom/src/dom.js

@@ -661,8 +658,8 @@ export function replaceTag( node, tagName ) {
 /**
 * Wraps the given node with a new node with the given tag name.
 *
- * @param {Element} newNode       The node to insert.
- * @param {Element} referenceNode The node to wrap.
+ * @param {Node} newNode       The node to insert.


Changed to Node because it only uses Node-based methods/properties

aduth · 2019-11-29T23:59:11Z

packages/dom/src/dom.js

@@ -638,13 +633,15 @@ export function unwrap( node ) {
 	parent.removeChild( node );
 }

+// eslint-disable-next-line valid-jsdoc


I assume this will need to be updated, since we no longer use valid-jsdoc but rather eslint-plugin-jsdoc (which has its own validity rule).

Yeah sorry about that. What's left to be done here? Forgot I had this PR open. (We can close this if it's too far behind)

Yeah sorry about that. What's left to be done here? Forgot I had this PR open. (We can close this if it's too far behind)

I was going to ask the same 😄 It seems in pretty good shape otherwise, it would be good to get in. This one caught my eye though. There could also be some other more-recent changes to account for. Could do for a rebase.

aduth · 2020-03-31T17:08:06Z

packages/dom/src/focusable.js

@@ -69,7 +69,7 @@ function isValidFocusableArea( element ) {
 /**
 * Returns all focusable elements within a given context.
 *
- * @param {Element} context Element in which to search.
+ * @param {ParentNode} context Element in which to search.


ParentNode isn't a valid type as far as I can tell. It would be Node I believe?

aduth · 2020-03-31T17:08:49Z

I'm going to close this, as it's been left to linger as stale for some time now. I'd like to see it revisited again in the future, ideally as part of an effort to opt-in the package to type checking (doing so will help identify potential problems like #16352 (comment)).

chore(@wordpress/dom): improve accuracy of jsdoc

e302d02

dsifford requested review from aduth, ellatrix, gziolo, oandregal and youknowriad as code owners June 28, 2019 16:05

dsifford commented Jun 28, 2019

View reviewed changes

Soean added the [Type] Developer Documentation Documentation for developers label Jun 28, 2019

aduth requested changes Nov 29, 2019

View reviewed changes

aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 31, 2020

aduth reviewed Mar 31, 2020

View reviewed changes

aduth closed this Mar 31, 2020

dsifford deleted the fix/dom/improve-jsdoc branch March 31, 2020 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(@wordpress/dom): improve accuracy of jsdoc #16352

chore(@wordpress/dom): improve accuracy of jsdoc #16352

dsifford commented Jun 28, 2019

dsifford Jun 28, 2019

aduth Jul 1, 2019

dsifford Jul 2, 2019

oandregal Dec 2, 2019

dsifford Dec 2, 2019

dsifford Jun 28, 2019

oandregal Dec 2, 2019

dsifford Dec 2, 2019

aduth Dec 2, 2019

oandregal Dec 2, 2019

dsifford Jun 28, 2019 •

edited

Loading

aduth Jul 1, 2019

dsifford Jul 2, 2019

dsifford Jun 28, 2019

aduth Nov 29, 2019

dsifford Nov 30, 2019

aduth Nov 30, 2019

aduth Mar 31, 2020

aduth commented Mar 31, 2020

chore(@wordpress/dom): improve accuracy of jsdoc #16352

chore(@wordpress/dom): improve accuracy of jsdoc #16352

Conversation

dsifford commented Jun 28, 2019

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsifford Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Mar 31, 2020

dsifford Jun 28, 2019 •

edited

Loading