Improve JSDoc comments #1115

andrewiggins · 2018-05-21T02:45:59Z

Update Preact's JSDoc comments to better reflect the current code and improve editor experience (if using editor with TypeScript support/extension). I make use of @typedefs to define a couple of key complex types used by Preact. Each complex type is described below.

Complex Types

Options: Documents all the public options a consumer of Preact can define

VNode: Documents the properties of a Preact Virtual DOM Node

PreactElement: Documents all of the properties Preact adds to the DOM elements it creates

What about Component?

I haven't exhaustively documented Component in this PR because that is a much more complex type to document in JSDoc and I'm considering doing it in its own PR to discuss different ways of how to accomplish it. I've started experimenting with how to do this in my update-jsdoc-component-doc branch if you are curious.

Other Notes

These JSDoc comments rely on a new feature in Typescript 2.9, "allow import(...)-ing types at any location". I use this feature to import @typedefs across files. The next version of VSCode should include TS 2.9 by default so these comments will work by default in that version. If you'd like to check out the TS 2.9 experience now, check out this branch and set the VSCode workspace setting typescript.tsdk to node_modules\\typescript\\lib.

This PR does not eliminate all errors that appear in the editor. In order to do that Preact would likely need to embrace full TypeScript or Flow syntaxes to utilize the entirety of features those languages offer. It might be possible to remove all errors using only JSDoc syntax however I'd like to explore that possibility in a separate PR as it might require an additional significant amount of changes.

I've rewritten many JSDoc comments to the default style that VSCode uses when you add new JSDoc comments.

Fixes #1106

coveralls · 2018-05-21T02:48:40Z

Coverage remained the same at 100.0% when pulling 6869b75 on andrewiggins:update-jsdoc-preactelement into 2364da5 on developit:master.

marvinhagemeister

That's awesome, thanks for your PR 🎉 Admittedly most of my comments are just nitpicking. There are few instances where we need to decide if we should stick with jsdoc and only use TS definitions when necessary or if we should go full on in with TS only.

The best thing about this PR is that vscode can follow the types correctly. I'm really excited about this PR 👍

marvinhagemeister · 2018-05-21T05:55:29Z

src/dom/index.js

+
+/**
+ * A DOM element that has been extended with Preact properties
+ * @typedef {Element & ElementCSSInlineStyle & PreactElementExtensions} PreactElement


This line here makes me really happy 👍👍

TS love 😄

marvinhagemeister · 2018-05-21T05:57:33Z

src/component.js

+	 * @param {object} props Props (eg: JSX attributes) received from parent element/component
+	 * @param {object} state The component's current state
+	 * @param {object} context Context object (if a parent component has provided context)
+	 * @returns {import('./vnode').VNode | void}


At least according to our TypeScript definitions we don't support undefined as a return type here. Have to check which one is more correct.

The lib should support returning any value from render, including undefined. IIRC undefined is treated identically to null.

marvinhagemeister · 2018-05-21T06:02:34Z

src/dom/index.js

+
+/**
+ * A mapping of event types to event listeners
+ * @typedef {{ [eventType: string]: EventListener}} EventListenerMap


I'm unsure if we should go full on with TS here or use plain jsdoc `Object.<string, EventListener>.

@andrewiggins Are there any difference that you are aware why TS made the change here? Or do they just allow writing arbitrary TS declarations inside the brackets with TS 2.9?

TS parses JSDoc types using the full TS types parser so I believe we can use any TS type syntax here.

They also support JSDoc syntax Object.<string, EventListener> (search for Index signatures to find it), as well as *.

So it is entirely up to us on what syntax to use. Full disclosure, I write TypeScript for my full time job, hence my obvious bias, but I'm happy rewrite these comments if people prefer the JSDoc syntax.

I'm ambivalent here. We might find the full-on TS syntax a bit heavy, which could hurt contribution. But, it likely provides better docs (which I think is our end goal here).

Sounds like we are learning towards JSDoc syntax so I've updated the types to use JSDoc type syntax where possible (though I could use an extra set of eyes to make sure I didn't miss anything).

3 places where I think we should (or have to in some cases) use TS syntax:

(should) Use TS function syntax (e.g. (arg: string) => string) in some places so we can specify the parameter types

(must) Use TS type operations (namely & and |) because I don't know of a JSDoc equivalent

(must) Use TS 2.9 import type syntax (e.g. import('./vnode').VNode) because I don't know of a JSDoc equivalent

marvinhagemeister · 2018-05-21T06:07:22Z

src/dom/index.js

+ * If `value` is `null`, the attribute/handler will be removed.
+ * @param {PreactElement} node An element to mutate
+ * @param {string} name The name/key to set, such as an event or attribute name
+ * @param {any} old The last value that was set for this name/node pair


Perhaps we just need to cast a quick vote if we'll use TS syntax in general or jsdoc. I'm fine with either. For the latter the any type is *.

Depends on what we're using the comments for. If we're generating docs from them and * doesn't give the necessary output, we kinda have to go with any. If either work, my preference would be slightly in favor of the JSDoc-compatible types.

Per my reply on the other comment, I've updated this to *. TS correctly parses and understands the *.

marvinhagemeister · 2018-05-21T06:07:35Z

src/dom/index.js

+ * IE & FF throw for certain property-value combinations.
+ * @param {Node} node The node to set a property on
+ * @param {string} name The name of the property to set
+ * @param {any} value The value to set


same as above

Updated to *

marvinhagemeister · 2018-05-21T06:14:18Z

src/util.js

+ * Copy all properties from `props` onto `obj`.
+ * @param {object} obj Object onto which properties should be copied.
+ * @param {object} props Object from which to copy properties.
+ * @returns {object} obj


@returns doesn't have a paramenter name, see http://usejsdoc.org/tags-returns.html

marvinhagemeister · 2018-05-21T06:15:15Z

src/vdom/component-recycler.js

+/**
+ * Retains a pool of Components for re-use, keyed on component name.
+ * Note: since component names are not unique or even necessarily available, these are primarily a form of sharding.
+ * @type {Record<string, Component[]>}


Record<string, Component[]> vs Object.<string, Component[]>. Same as above, I think we just need to settle on a syntax and be consistent with it :)

Updated to Object.<string, Component[]>

marvinhagemeister · 2018-05-21T06:16:34Z

src/vdom/component.js

+ * Set a component's `props` and possibly re-render the component
+ * @param {import('../component').Component} component The Component to set props on
+ * @param {object} props The new props
+ * @param {number} opts Render options - specifies how to re-render the component


Makes we wonder if we should rename opts to something like render mode or something similar.

Yea, agreed. Renamed to renderMode.

marvinhagemeister · 2018-05-21T06:18:19Z

src/vnode.js

+ * @typedef VNode
+ * @property {string | Function} nodeName The string of the DOM node to create or Component constructor to render
+ * @property {Array<VNode | string>} children The children of node
+ * @property {string | number | undefined} key The key used to identify this VNode in a list


We should use either undefined or void throughout all our docs

Technically the types mean two different things when used as function return types in the TS type-checker (at least for now).

For example (TS playground),

function (): void {}

The above code uses void, instructing TypeScript that the the function does not need to have a return statement.

However, in this next example,

function (): undefined { return undefined; }

instructs TypeScript that all code paths in the function should lead to a return statement that returns undefined.

So in summary, I suggest use undefined, unless you want to specify that a function doesn't have to have a return statement, then use void. For Preact, the default implementation is just render() {}, so void is the right type to use there. Everywhere else should use undefined.

marvinhagemeister · 2018-05-21T06:19:10Z

src/vdom/component.js

+/**
+ * Render a Component, triggering necessary lifecycle events and taking High-Order Components into account.
+ * @param {import('../component').Component} component The component to render
+ * @param {number} [opts] A number indicating the kind of render to execute (e.g. sync, async, force)


Perhaps we should rename opts to better reflect its meaning

Renamed to renderMode

marvinhagemeister · 2018-05-21T16:13:50Z

@andrewiggins Thanks for addressing the comments. Hope you don't mind that I was rather nitpicky about the types. Only thing left is wether we use jsdoc or full on TS types syntax. I pinged everyone on slack and we should have an answer soon 👍

developit · 2018-05-21T16:52:23Z

src/clone-element.js

- * @param {VNode} vnode		The virtual DOM element to clone
- * @param {Object} props	Attributes/props to add when cloning
- * @param {VNode} rest		Any additional arguments will be used as replacement children.
+ * @param {import('./vnode').VNode} vnode The virtual DOM element to clone


Would it be easier to kick this import to the top of the file? It'll get stripped out anyway.

Oh true! I'll do that.

FYI, Had to add VNode to the eslint no-unused-var exception

I feel like we should be consistent with this where possible. The latest changes have a top level import here and all other places use import inside jsdoc.

Although I think the top level imports are easier to understand for new contributors, I'm a bit worried about reducing usefulness of no-unused-var-exception. After thinking a bit more about it I think I'll actually prefer the inline imports. What are your thoughts about this? @developit @andrewiggins

Yea, that's fair. Consistency would be good. I don't have a strong preference for either pattern but since Preact is not TypeScript, I like the clean separation of type-related "code" (i.e. JSDoc) and real code. Putting the unused import at the top of the file may seem like a bug to new contributors since no real code uses it.

@marvinhagemeister I've gone ahead and reverted the change back to importing the type inside the JSDoc comment to be consistent with the rest of the changes in this PR. I suggest if we want to use ES6 imports purely for JSDoc comments, we can do that in another PR that applies it consistently across the codebase. Doing that we can evaluate what it does to the codebase and better consider the maintenance tradeoffs, etc.

developit · 2018-05-21T16:53:30Z

src/component.js

+	 * @param {object} props Props (eg: JSX attributes) received from parent element/component
+	 * @param {object} state The component's current state
+	 * @param {object} context Context object (if a parent component has provided context)
+	 * @returns {import('./vnode').VNode | void}


The lib should support returning any value from render, including undefined. IIRC undefined is treated identically to null.

developit · 2018-05-21T16:54:13Z

src/component.js

 	 */
 	this.state = this.state || {};
 }


 extend(Component.prototype, {

-	/** Returns a `boolean` indicating if the component should re-render when receiving the given `props` and `state`.


Is there a way for us to keep documentation for these methods without them being defined on the prototype? Ideally I'd love to have the whole Component API documented as JSDoc annotations here, but only setState(), forceUpdate() and render() actually implemented.

Yea, I agree - documenting all the lifecycles here in JSDoc without having to define them on the prototype would be ideal. I'm experimenting with some techniques in a branch in my fork (a commmit with one experiement), but so far they all have some compromises. If I get to a good place with a good approach I'll open a PR.

developit · 2018-05-21T16:55:35Z

src/component.js

-	 *	@param {object} state		A hash of state properties to update with new values
-	 *	@param {function} callback	A function to be called once component state is updated
+	/**
+	 * Update component state by copying properties from `state` to `this.state`.


We can change this description to be more user-facing:

Update component state and schedule a re-render.

developit · 2018-05-21T16:57:44Z

src/component.js

+	 * Virtual DOM is generally constructed via [JSX](http://jasonformat.com/wtf-is-jsx).
+	 * @param {object} props Props (eg: JSX attributes) received from parent element/component
+	 * @param {object} state The component's current state
+	 * @param {object} context Context object (if a parent component has provided context)


For context, this description became incorrect in Preact 4. We could change it to:

Context object, as returned by the nearest ancestor's `getChildContext()`

developit · 2018-05-21T16:58:17Z

src/constants.js

@@ -1,13 +1,17 @@
 // render modes

+/** Do not recursively re-render a component */


IIRC this flag disables re-rendering entirely (s/recursively //)

Yea that's right, I believe

developit · 2018-05-21T16:59:09Z

src/constants.js

 export const NO_RENDER = 0;
+/** Recursively re-render a component and it's children */


"Synchronously re-render a component and its children"

developit · 2018-05-21T16:59:36Z

src/constants.js

 export const SYNC_RENDER = 1;
+/** Force a re-render of a component */


Synchronously re-render a component, even if its lifecycle methods attempt to prevent it.

developit · 2018-05-21T17:00:50Z

src/dom/index.js

+
+/**
+ * A mapping of event types to event listeners
+ * @typedef {{ [eventType: string]: EventListener}} EventListenerMap


I'm ambivalent here. We might find the full-on TS syntax a bit heavy, which could hurt contribution. But, it likely provides better docs (which I think is our end goal here).

developit · 2018-05-21T17:45:21Z

src/dom/index.js

+ * If `value` is `null`, the attribute/handler will be removed.
+ * @param {PreactElement} node An element to mutate
+ * @param {string} name The name/key to set, such as an event or attribute name
+ * @param {any} old The last value that was set for this name/node pair


Depends on what we're using the comments for. If we're generating docs from them and * doesn't give the necessary output, we kinda have to go with any. If either work, my preference would be slightly in favor of the JSDoc-compatible types.

andrewiggins · 2018-05-26T20:53:58Z

@marvinhagemeister @developit I believed I've resolved all the comments on this PR. When you have time, let me know what you think. Thanks for taking the time to leave your thoughts.

@marvinhagemeister No worries about the "nitpicky"-ness 😄 I thought your comments were were helpful.

marvinhagemeister

LGTM, this is really awesome! Being able to click through the types is something that has been missing from our code for a while. Again solid work, and love the thoughtfulness. Congrats to another successful PR 🎉 👍

Update Preact's JSDoc comments to better reflect the current code and improve editor experience (if using editor with TypeScript support/extension). I make use of `@typedef`s to define a couple of key complex types used by Preact. Each complex type is described below. ## Complex Types **Options**: Documents all the public options a consumer of Preact can define **VNode**: Documents the properties of a Preact Virtual DOM Node **PreactElement**: Documents all of the properties Preact adds to the DOM elements it creates ## What about Component? I haven't exhaustively documented Component in this PR because that is a much more complex type to document in JSDoc and I'm considering doing it in its own PR to discuss different ways of how to accomplish it. I've started experimenting with how to do this in my [update-jsdoc-component-doc branch](https://github.com/andrewiggins/preact/blob/update-jsdoc-component-doc/src/component.js) if you are curious. ## Other Notes These JSDoc comments rely on a new feature in Typescript 2.9, "[allow `import(...)-ing` types at any location](https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#allow-import-ing-types-at-any-location)". I use this feature to import `@typedef`s across files. The next version of VSCode should include TS 2.9 by default so these comments will work by default in that version. If you'd like to check out the TS 2.9 experience now, check out this branch and set the VSCode workspace setting `typescript.tsdk` to `node_modules\\typescript\\lib`. This PR does not eliminate all errors that appear in the editor. In order to do that Preact would likely need to embrace full TypeScript or Flow syntaxes to utilize the entirety of features those languages offer. It might be possible to remove all errors using only JSDoc syntax however I'd like to explore that possibility in a separate PR as it might require an additional significant amount of changes. I've rewritten many JSDoc comments to the default style that VSCode uses when you add new JSDoc comments. Fixes preactjs#1106

andrewiggins added 16 commits May 9, 2018 13:52

WIP - some jsdoc improvements

ebe7b9b

Make progress on jsdocs

c53d59e

Update to typescript 2.9 rc

030096a

Reformat JSDoc comments in dom and vdom folders

a92225b

Reformat JSDoc's of remaining files

0a0c1a9

Use object and boolean in vdom & dom jsdoc comments

a0ef059

Add JSDoc for diff

236149d

Fix up Component class JSDoc comments

6a4b62a

Add comments to some constants

bca3b38

Fix bug in VNode typings related to key

a2ddb33

Introduce basic PreactElement type

7e59893

Extend PreactElement type with custom Preact Element props

24859a9

Remove most typedef imports

3d01b76

Fill out some descriptions

3d779b3

Define createComponent return type

35a183d

Fix ASYNC_RENDER comment

8e86a87

Improve component callback types

c5d1d68

andrewiggins changed the title ~~Update JSDoc comments~~ Improve JSDoc comments May 21, 2018

Merge branch 'master' into update-jsdoc-preactelement

4e8af06

marvinhagemeister requested changes May 21, 2018

View reviewed changes

developit reviewed May 21, 2018

View reviewed changes

andrewiggins added 7 commits May 21, 2018 23:14

Respond to some PR comments

f1da9d7

Merge branch 'master' into update-jsdoc-preactelement

26c582c

Update to JSDoc type syntax

78e8b9e

Rename opts param to renderMode

fb96222

Update doc for context

42d4477

Fix ESLint errors & warnings

21ee88f

Merge branch 'master' into update-jsdoc-preactelement

1775021

andrewiggins added 2 commits May 29, 2018 21:24

Undo using JS imports for JSDocs

ebd4027

Merge branch 'master' into update-jsdoc-preactelement

6869b75

marvinhagemeister approved these changes May 30, 2018

View reviewed changes

marvinhagemeister merged commit 2399c49 into preactjs:master May 30, 2018

andrewiggins deleted the update-jsdoc-preactelement branch May 30, 2018 14:15

		@@ -1,13 +1,17 @@
		// render modes

		/** Do not recursively re-render a component */

		export const NO_RENDER = 0;
		/** Recursively re-render a component and it's children */

		export const SYNC_RENDER = 1;
		/** Force a re-render of a component */

Improve JSDoc comments #1115

Improve JSDoc comments #1115

Conversation

andrewiggins commented May 21, 2018 • edited Loading

Complex Types

What about Component?

Other Notes

coveralls commented May 21, 2018 • edited Loading

marvinhagemeister left a comment • 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

andrewiggins May 21, 2018 • 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

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

marvinhagemeister commented May 21, 2018

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

andrewiggins May 26, 2018 • 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

Choose a reason for hiding this comment

andrewiggins commented May 26, 2018

marvinhagemeister left a comment

Choose a reason for hiding this comment

andrewiggins commented May 21, 2018 •

edited

Loading

coveralls commented May 21, 2018 •

edited

Loading

marvinhagemeister left a comment •

edited

Loading

andrewiggins May 21, 2018 •

edited

Loading

andrewiggins May 26, 2018 •

edited

Loading