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

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

Open
wants to merge 1 commit into
base: master
from

Conversation

@dsifford
Copy link
Contributor

dsifford commented Jun 28, 2019

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.
/**
* 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 comment has been minimized.

Copy link
@dsifford

dsifford Jun 28, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@aduth

aduth Jul 1, 2019

Member

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

But it's not valid JSDoc ?

This comment has been minimized.

Copy link
@dsifford

dsifford Jul 2, 2019

Author Contributor

Not valid per eslint I guess.

This comment has been minimized.

Copy link
@nosolosw

nosolosw Dec 2, 2019

Member

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

This comment has been minimized.

Copy link
@dsifford

dsifford Dec 2, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@dsifford

dsifford Jun 28, 2019

Author Contributor

Not sure the documentation generator is prepared for this.

This comment has been minimized.

Copy link
@nosolosw

nosolosw Dec 2, 2019

Member

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

This comment has been minimized.

Copy link
@dsifford

dsifford Dec 2, 2019

Author Contributor

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)

This comment has been minimized.

Copy link
@aduth

aduth Dec 2, 2019

Member

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.

This comment has been minimized.

Copy link
@nosolosw

nosolosw Dec 2, 2019

Member

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

This comment has been minimized.

Copy link
@dsifford

dsifford Jun 28, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@aduth

aduth Jul 1, 2019

Member

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 👍

This comment has been minimized.

Copy link
@dsifford

dsifford Jul 2, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@dsifford

dsifford Jun 28, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@aduth

aduth Nov 29, 2019

Member

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

This comment has been minimized.

Copy link
@dsifford

dsifford Nov 30, 2019

Author Contributor

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)

This comment has been minimized.

Copy link
@aduth

aduth Nov 30, 2019

Member

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.

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.