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

Improved JSDoc comments for type correctness. #1161

Closed
wants to merge 45 commits into from
Closed
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
adf8dd5
Improved JSDoc comments for type correctness.
Wobbabits Jul 4, 2018
26ea308
Merge branch 'master' into master
developit Jul 10, 2018
e6573fa
Changes to pass tsc --allowJs --checkJs
Jul 11, 2018
7d21d23
typescript@^2.9.2 update
Jul 11, 2018
b6eb3c9
.gitignore ignore .*.swp
Jul 11, 2018
1e4140a
package.json add checkjs script
Jul 11, 2018
64e8503
Minor cleanup related to JSDoc fixes
Jul 11, 2018
e95c386
separate import for other JSDoc param items
Jul 11, 2018
d2b7141
Merge pull request #1 from brodybits/cb-tsc-fix1
Wobbabits Jul 11, 2018
c0ae1d4
Fixed problem to not require type cast for constructor.
Wobbabits Jul 16, 2018
05a7ddc
Fix JSDoc cast in buildComponentFromVNode
Jul 16, 2018
aca4d72
package.json use checkJs script name (camelCase)
Jul 15, 2018
7f5735e
src/vdom/component-recycler.js remove param import
Jul 15, 2018
5ebff59
src/component.js move JSDoc import
Jul 15, 2018
dd17907
minor cleanup of JSDoc type in internal idiff
Jul 15, 2018
0387927
use internal consts to cleanup inline JSDoc types
Jul 17, 2018
3a189fa
Improved JSDoc comments for type correctness.
Wobbabits Jul 4, 2018
1ed6764
Fixed problem to not require type cast for constructor.
Wobbabits Jul 16, 2018
37495b2
Changes to pass tsc --allowJs --checkJs
Jul 11, 2018
cc10b1c
typescript@^2.9.2 update
Jul 11, 2018
6125010
.gitignore ignore .*.swp
Jul 11, 2018
972d6b3
package.json add checkJs
Jul 11, 2018
976ac4a
Minor cleanup related to JSDoc fixes
Jul 11, 2018
78ea2ef
separate import for other JSDoc param items
Jul 11, 2018
bce5afa
Fix JSDoc cast in buildComponentFromVNode
Jul 16, 2018
5ac7548
minor cleanup of JSDoc type in internal idiff
Jul 15, 2018
9f82f5b
use internal consts to cleanup inline JSDoc types
Jul 17, 2018
0d2aea9
Merge branch 'master' into new-jsdoc-updates
Wobbabits Jul 18, 2018
170109b
Merge pull request #2 from brodybits/cb-tsc-fix2
Wobbabits Jul 18, 2018
decf628
Changed text node type cast from 'any' to Node.
Wobbabits Jul 18, 2018
57a66e2
Use JSDoc types instead of property strings
Jul 22, 2018
ab2359c
Fix JSDoc EventListener @typedef
Jul 22, 2018
59c4695
src/dom/index.js move JSDoc import of Component
Jul 22, 2018
67e5185
Inline some consts to reduce size
Jul 22, 2018
36f2a27
src/vdom/diff.js remove some inline JSDoc types
Jul 22, 2018
fb063cf
Merge branch 'master' into new-jsdoc-updates
Wobbabits Jul 25, 2018
b479244
Merge pull request #3 from brodybits/new-jsdoc-updates
Wobbabits Jul 25, 2018
d4abe42
Revert "jsdoc updates rebased on upstream master"
Wobbabits Jul 26, 2018
8cc588d
Merge pull request #5 from rbiggs/revert-3-new-jsdoc-updates
Wobbabits Jul 26, 2018
a2be058
Fixed union type for VNode children.
Wobbabits Jul 30, 2018
bfd9743
Merge branch 'master' of https://github.com/rbiggs/preact
Wobbabits Jul 30, 2018
c24eccf
src/vdom/diff.js fixes to preserve original JS
Jul 23, 2018
09f9690
src/vnode.js remove @ts-nocheck (not needed)
Jul 23, 2018
c90f388
Fix JSDoc return type of vdom.getNodeProps()
Wobbabits Jul 30, 2018
ce26426
Merge pull request #4 from brodybits/cb-tsc-fix3
Wobbabits Jul 31, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
/coverage
package-lock.json
/test/ts/**/*.js
.*.swp

# Additional bundles
/devtools.js
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"minified:main": "dist/preact.min.js",
"types": "dist/preact.d.ts",
"scripts": {
"checkJs": "tsc --allowJs --checkJs --noEmit --target ES6 src/*.js src/**/*.js",
"clean": "rimraf dist/ devtools.js devtools.js.map debug.js debug.js.map test/ts/**/*.js",
"copy-flow-definition": "copyfiles -f src/preact.js.flow dist",
"copy-typescript-definition": "copyfiles -f src/preact.d.ts dist",
Expand Down Expand Up @@ -119,7 +120,7 @@
"rollup-plugin-node-resolve": "^3.0.0",
"sinon": "^4.4.2",
"sinon-chai": "^3.0.0",
"typescript": "^2.9.0-rc",
"typescript": "^2.9.2",
"uglify-js": "^2.7.5",
"webpack": "^4.3.0"
},
Expand Down
8 changes: 6 additions & 2 deletions src/clone-element.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { extend } from './util';
import { h } from './h';

/**
* @typedef {import('./vnode').VNode} VNode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh I like this one! Totally forgot about typedef 👍

*/

/**
* Clones the given VNode, optionally adding attributes/props and replacing its
* children.
* @param {import('./vnode').VNode} vnode The virtual DOM element to clone
* @param {VNode} vnode The virtual DOM element to clone
* @param {object} props Attributes/props to add when cloning
* @param {Array<import('./vnode').VNode>} [rest] Any additional arguments will be used as replacement
* @param {Array<VNode>} [rest] Any additional arguments will be used as replacement
* children.
*/
export function cloneElement(vnode, props) {
Expand Down
7 changes: 6 additions & 1 deletion src/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ import { FORCE_RENDER } from './constants';
import { extend } from './util';
import { renderComponent } from './vdom/component';
import { enqueueRender } from './render-queue';

/**
* @typedef {import('./vnode').VNode} VNode
*/

/**
* Base Component class.
* Provides `setState()` and `forceUpdate()`, which trigger rendering.
Expand Down Expand Up @@ -79,7 +84,7 @@ extend(Component.prototype, {
* @param {object} state The component's current state
* @param {object} context Context object, as returned by the nearest
* ancestor's `getChildContext()`
* @returns {import('./vnode').VNode | void}
* @returns {VNode | void}
*/
render() {}

Expand Down
8 changes: 6 additions & 2 deletions src/render-queue.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,19 @@ import options from './options';
import { defer } from './util';
import { renderComponent } from './vdom/component';

/**
* @typedef {import('./component').Component} Component
*/

/**
* Managed queue of dirty components to be re-rendered
* @type {Array<import('./component').Component>}
* @type {Array<Component>}
*/
let items = [];

/**
* Enqueue a rerender of a component
* @param {import('./component').Component} component The component to rerender
* @param {Component} component The component to rerender
*/
export function enqueueRender(component) {
if (!component._dirty && (component._dirty = true) && items.push(component)==1) {
Expand Down
11 changes: 8 additions & 3 deletions src/render.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import { diff } from './vdom/diff';

/**
* @typedef {import('./vnode').VNode} VNode
* @typedef {import('./dom/index.js').PreactElement} PreactElement
*/

/**
* Render JSX into a `parent` Element.
* @param {import('./vnode').VNode} vnode A (JSX) VNode to render
* @param {import('./dom').PreactElement} parent DOM element to render into
* @param {import('./dom').PreactElement} [merge] Attempt to re-use an existing DOM tree rooted at
* @param {VNode} vnode A (JSX) VNode to render
* @param {PreactElement} parent DOM element to render into
* @param {PreactElement} [merge] Attempt to re-use an existing DOM tree rooted at
* `merge`
* @public
*
Expand Down
6 changes: 3 additions & 3 deletions src/vdom/component-recycler.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ export function collectComponent(component) {
/**
* Create a component. Normalizes differences between PFC's and classful
* Components.
* @param {function} Ctor The constructor of the component to create
* @param {(props, context) => void} Ctor The constructor of the component to create
* @param {object} props The initial props of the component
* @param {object} context The initial context of the component
* @returns {import('../component').Component}
* @returns {Component}
*/
export function createComponent(Ctor, props, context) {
let list = components[Ctor.name],
Expand All @@ -39,7 +39,7 @@ export function createComponent(Ctor, props, context) {
else {
inst = new Component(props, context);
inst.constructor = Ctor;
inst.render = doRender;
inst['render'] = doRender;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an unnecessary change here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that render is being dynamically tagged on to the inst object. Since it doesn't exist on the object, any type checker will give the following error:

render

You can get around this in one of two ways: type casting the inst object to something more generic, such as any (worst choice). Or simply escaping the expando property. During minification the [''] gets removed and the property is inst.render. So it doesn't affect the final output. The brackets and quotes basically hide the expando property from the type checker.

This is a very common problem with dealing with expando properties and type systems. TypeScript users prefer to escape them as I have done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reeeally like to see if we can avoid code changes for the doc annotations. This was something I had run into in another project when trying to use checkJs.

Thought: is there a way for us to use a typedef to create a ficticious render() method on Component.prototype?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be resolved here and in other places by changes proposed in rbiggs/preact#4, according to the first recommended alternative:

type casting the inst object to something more generic

I find it interesting to note his second alternative:

During minification the [''] gets removed and the property is inst.render. So it doesn't affect the final output.

}


Expand Down
23 changes: 16 additions & 7 deletions src/vdom/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,15 @@ import { diff, mounts, diffLevel, flushMounts, recollectNodeTree, removeChildren
import { createComponent, collectComponent } from './component-recycler';
import { removeNode } from '../dom/index';

/**
* @typedef {import('../component').Component} Component
* @typedef {import('../dom/index.js').PreactElement} PreactElement
* @typedef {import('../vnode').VNode} VNode
*/

/**
* Set a component's `props` and possibly re-render the component
* @param {import('../component').Component} component The Component to set props on
* @param {Component} component The Component to set props on
* @param {object} props The new props
* @param {number} renderMode Render options - specifies how to re-render the component
* @param {object} context The new context
Expand Down Expand Up @@ -60,7 +66,7 @@ export function setComponentProps(component, props, renderMode, context, mountAl
/**
* Render a Component, triggering necessary lifecycle events and taking
* High-Order Components into account.
* @param {import('../component').Component} component The component to render
* @param {Component} component The component to render
* @param {number} [renderMode] render mode, see constants.js for available options.
* @param {boolean} [mountAll] Whether or not to immediately mount all components
* @param {boolean} [isChild] ?
Expand Down Expand Up @@ -212,11 +218,11 @@ export function renderComponent(component, renderMode, mountAll, isChild) {

/**
* Apply the Component referenced by a VNode to the DOM.
* @param {import('../dom').PreactElement} dom The DOM node to mutate
* @param {import('../vnode').VNode} vnode A Component-referencing VNode
* @param {PreactElement} dom The DOM node to mutate
* @param {VNode} vnode A Component-referencing VNode
* @param {object} context The current context
* @param {boolean} mountAll Whether or not to immediately mount all components
* @returns {import('../dom').PreactElement} The created/mutated element
* @returns {PreactElement} The created/mutated element
* @private
*/
export function buildComponentFromVNode(dom, vnode, context, mountAll) {
Expand All @@ -240,7 +246,10 @@ export function buildComponentFromVNode(dom, vnode, context, mountAll) {
dom = oldDom = null;
}

c = createComponent(vnode.nodeName, props, context);
const f =
/** @type {(props, context) => void} */(vnode.nodeName);

c = createComponent(f, props, context);
if (dom && !c.nextBase) {
c.nextBase = dom;
// passing dom/oldDom as nextBase will recycle it if unused, so bypass recycling on L229:
Expand All @@ -262,7 +271,7 @@ export function buildComponentFromVNode(dom, vnode, context, mountAll) {

/**
* Remove a component from the DOM and recycle it.
* @param {import('../component').Component} component The Component instance to unmount
* @param {Component} component The Component instance to unmount
* @private
*/
export function unmountComponent(component) {
Expand Down
67 changes: 44 additions & 23 deletions src/vdom/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@ import { unmountComponent } from './component';
import options from '../options';
import { removeNode } from '../dom/index';

/**
* @typedef {import('../component').Component} Component
* @typedef {import('../dom/index.js').PreactElement} PreactElement
* @typedef {import('../vnode').VNode} VNode
*/

/**
* Queue of components that have been mounted and are awaiting componentDidMount
* @type {Array<import('../component').Component>}
* @type {Array<Component>}
*/
export const mounts = [];

Expand All @@ -33,21 +39,23 @@ export function flushMounts() {

/**
* Apply differences in a given vnode (and it's deep children) to a real DOM Node.
* @param {import('../dom').PreactElement} dom A DOM node to mutate into the shape of a `vnode`
* @param {import('../vnode').VNode} vnode A VNode (with descendants forming a tree) representing
* @param {PreactElement} dom A DOM node to mutate into the shape of a `vnode`
* @param {VNode} vnode A VNode (with descendants forming a tree) representing
* the desired DOM structure
* @param {object} context The current context
* @param {boolean} mountAll Whether or not to immediately mount all components
* @param {Element} parent ?
* @param {boolean} componentRoot ?
* @returns {import('../dom').PreactElement} The created/mutated element
* @returns {PreactElement} The created/mutated element
* @private
*/
export function diff(dom, vnode, context, mountAll, parent, componentRoot) {
// diffLevel having been 0 here indicates initial entry into the diff (not a subdiff)
if (!diffLevel++) {
// when first starting the diff, check if we're diffing an SVG or within an SVG
isSvgMode = parent!=null && parent.ownerSVGElement!==undefined;
isSvgMode =
parent != null &&
/** @type{SVGElement} */(parent).ownerSVGElement !== undefined;

// hydration is indicated by the existing element to be diffed not having a prop cache
hydrating = dom!=null && !(ATTR_KEY in dom);
Expand All @@ -71,8 +79,8 @@ export function diff(dom, vnode, context, mountAll, parent, componentRoot) {

/**
* Internals of `diff()`, separated to allow bypassing diffLevel / mount flushing.
* @param {import('../dom').PreactElement} dom A DOM node to mutate into the shape of a `vnode`
* @param {import('../vnode').VNode} vnode A VNode (with descendants forming a tree) representing the desired DOM structure
* @param {PreactElement} dom A DOM node to mutate into the shape of a `vnode`
* @param {VNode} vnode A VNode (with descendants forming a tree) representing the desired DOM structure
* @param {object} context The current context
* @param {boolean} mountAll Whether or not to immediately mount all components
* @param {boolean} [componentRoot] ?
Expand All @@ -83,22 +91,25 @@ function idiff(dom, vnode, context, mountAll, componentRoot) {
prevSvgMode = isSvgMode;

// empty values (null, undefined, booleans) render as empty Text nodes
if (vnode==null || typeof vnode==='boolean') vnode = '';
if (vnode == null || typeof vnode === 'boolean')
/** @type {any} */(vnode) = '';


// Fast case: Strings & Numbers create/update Text nodes.
if (typeof vnode==='string' || typeof vnode==='number') {

// update if it's already a Text node:
if (dom && dom.splitText!==undefined && dom.parentNode && (!dom._component || componentRoot)) {
if (dom && dom['splitText']!==undefined && dom.parentNode && (!dom._component || componentRoot)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an unnecessary change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same problem as with inst.render as described above.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also be resolved by changes proposed in rbiggs/preact#4

/* istanbul ignore if */ /* Browser quirk that can't be covered: https://github.com/developit/preact/commit/fd4f21f5c45dfd75151bd27b4c217d8003aa5eb9 */
if (dom.nodeValue!=vnode) {
dom.nodeValue = vnode;
}
}
else {
// it wasn't a Text node: replace it with one and recycle the old Element
out = document.createTextNode(vnode);
/** @type {Node} */(out) =
document.createTextNode(vnode);

if (dom) {
if (dom.parentNode) dom.parentNode.replaceChild(out, dom);
recollectNodeTree(dom, true);
Expand Down Expand Up @@ -150,14 +161,19 @@ function idiff(dom, vnode, context, mountAll, componentRoot) {
}

// Optimization: fast-path for elements containing a single TextNode:
if (!hydrating && vchildren && vchildren.length===1 && typeof vchildren[0]==='string' && fc!=null && fc.splitText!==undefined && fc.nextSibling==null) {
if (fc.nodeValue!=vchildren[0]) {
fc.nodeValue = vchildren[0];
if (!hydrating && vchildren && vchildren.length===1 && typeof vchildren[0]==='string' && fc!=null && fc['splitText']!==undefined && fc.nextSibling==null) {
const vchild =
/** @type {string} */(vchildren[0]);
if (fc.nodeValue!=vchild) {
fc.nodeValue = vchild;
}
}
// otherwise, if there are existing or new children, diff them:
else if (vchildren && vchildren.length || fc!=null) {
innerDiffNode(out, vchildren, context, mountAll, hydrating || props.dangerouslySetInnerHTML!=null);
const vnodeChildren =
/** @type {VNode[]} */(vchildren);
innerDiffNode(out, vnodeChildren,
context, mountAll, hydrating || props.dangerouslySetInnerHTML!=null);
}


Expand All @@ -174,8 +190,8 @@ function idiff(dom, vnode, context, mountAll, componentRoot) {

/**
* Apply child and attribute changes between a VNode and a DOM Node to the DOM.
* @param {import('../dom').PreactElement} dom Element whose children should be compared & mutated
* @param {Array<import('../vnode').VNode>} vchildren Array of VNodes to compare to `dom.childNodes`
* @param {PreactElement} dom Element whose children should be compared & mutated
* @param {Array<VNode>} vchildren Array of VNodes to compare to `dom.childNodes`
* @param {object} context Implicitly descendant context object (from most
* recent `getChildContext()`)
* @param {boolean} mountAll Whether or not to immediately mount all components
Expand All @@ -191,19 +207,19 @@ function innerDiffNode(dom, vchildren, context, mountAll, isHydrating) {
len = originalChildren.length,
childrenLen = 0,
vlen = vchildren ? vchildren.length : 0,
j, c, f, vchild, child;
j, f, vchild, child;

// Build up a map of keyed children and an Array of unkeyed children:
if (len!==0) {
for (let i=0; i<len; i++) {
let child = originalChildren[i],
props = child[ATTR_KEY],
key = vlen && props ? child._component ? child._component.__key : props.key : null;
key = vlen && props ? child['_component'] ? child['_component'].__key : props.key : null;
if (key!=null) {
keyedLen++;
keyed[key] = child;
}
else if (props || (child.splitText!==undefined ? (isHydrating ? child.nodeValue.trim() : true) : isHydrating)) {
else if (props || (child['splitText']!==undefined ? (isHydrating ? child.nodeValue.trim() : true) : isHydrating)) {
children[childrenLen++] = child;
}
}
Expand All @@ -226,7 +242,9 @@ function innerDiffNode(dom, vchildren, context, mountAll, isHydrating) {
// attempt to pluck a node of the same type from the existing children
else if (min<childrenLen) {
for (j=min; j<childrenLen; j++) {
if (children[j]!==undefined && isSameNodeType(c = children[j], vchild, isHydrating)) {
const c =
/** @type {any[]} */(children)[j];
if (c !== undefined && isSameNodeType(c, vchild, isHydrating)) {
child = c;
children[j] = undefined;
if (j===childrenLen-1) childrenLen--;
Expand Down Expand Up @@ -262,15 +280,18 @@ function innerDiffNode(dom, vchildren, context, mountAll, isHydrating) {

// remove orphaned unkeyed children:
while (min<=childrenLen) {
if ((child = children[childrenLen--])!==undefined) recollectNodeTree(child, false);
const c =
/** @type { any } */(children[childrenLen--]);
if (c !== undefined)
recollectNodeTree(c, false);
}
}



/**
* Recursively recycle (or just unmount) a node and its descendants.
* @param {import('../dom').PreactElement} node DOM node to start
* @param {PreactElement} node DOM node to start
* unmount/removal from
* @param {boolean} [unmountOnly=false] If `true`, only triggers unmount
* lifecycle, skips removal
Expand Down Expand Up @@ -312,7 +333,7 @@ export function removeChildren(node) {

/**
* Apply differences in attributes from a VNode to the given DOM Element.
* @param {import('../dom').PreactElement} dom Element with attributes to diff `attrs` against
* @param {PreactElement} dom Element with attributes to diff `attrs` against
* @param {object} attrs The desired end-state key-value attribute pairs
* @param {object} old Current/previous attributes (from previous VNode or
* element's prop cache)
Expand Down
Loading