chore(@wordpress/dom): improve accuracy of jsdoc #16352
Conversation
dsifford
requested review from
aduth,
ellatrix,
gziolo,
oandregal and
youknowriad
as code owners
| /** | ||
| * 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 |
There was a problem hiding this comment.
This is the location of where the JSDoc will improve autocompletion and catch typos.
There was a problem hiding this comment.
This is the location of where the JSDoc will improve autocompletion and catch typos.
But it's not valid JSDoc ?
There was a problem hiding this comment.
Not valid per eslint I guess.
There was a problem hiding this comment.
Where is the @template tag to be found? I don't see it anywhere in the standard.
There was a problem hiding this comment.
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
| - _node_ `Element`: The node to replace | ||
| - _tagName_ `string`: The new tag name. | ||
| - _node_ `Node`: The node to replace. | ||
| - _tagName_ `T`: The new tag name. |
There was a problem hiding this comment.
Not sure the documentation generator is prepared for this.
There was a problem hiding this comment.
It looks like this is generated by this part of the JSDoc which is not to be found in the JSDoc standard.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
| */ | ||
| export function getScrollContainer( node ) { | ||
| if ( ! node ) { | ||
| export function getScrollContainer( element ) { |
There was a problem hiding this comment.
Changed to element since it expects an Element and not a Node
There was a problem hiding this comment.
Changed to
elementsince it expects anElementand not aNode
Technically Element implements the Node interface, so every element is a node (reference). But for clarity's sake, I'd agree with your assessment 👍
There was a problem hiding this comment.
Correct. But clientHeight and scrollHeight are only available on the Element interface so only an Element would be valid to pass into this function.
| @@ -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. | |||
There was a problem hiding this comment.
Changed to Node because it only uses Node-based methods/properties
| @@ -638,13 +633,15 @@ export function unwrap( node ) { | |||
| parent.removeChild( node ); | |||
| } | |||
|
|
|||
| // eslint-disable-next-line valid-jsdoc | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
added
the
[Status] Stale
| @@ -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. | |||
There was a problem hiding this comment.
ParentNode isn't a valid type as far as I can tell. It would be Node I believe?
|
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)). |
Description
This PR simply improves the JSDoc in
@wordpress/domfor 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: