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 all 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
9 changes: 7 additions & 2 deletions src/dom/index.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { IS_NON_DIMENSIONAL } from '../constants';
import options from '../options';

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

/**
* A DOM event listener
* @typedef {(e: Event) => void} EventListner
* @typedef {(e: Event) => void} EventListener
*/

/**
Expand All @@ -15,8 +19,9 @@ import options from '../options';
* Properties Preact adds to elements it creates
* @typedef PreactElementExtensions
* @property {string} [normalizedNodeName] A normalized node name to use in diffing
* @property {string} [splitText]
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I'm aware of that. The problem is that in the cases where splitText is getting used, it's on data that sometimes might be an object of some type, or a number, or a string. That's fine for JavaScript because it will try to coerce the data. Take this example:

export function isSameNodeType(node, vnode, hydrating) {
  if (typeof vnode==='string' || typeof vnode==='number') {
    return node.splitText !== undefined;
  }
  if (typeof vnode.nodeName==='string') {
    return !node._componentConstructor && isNamedNode(node, vnode.nodeName);
  }
  return hydrating || node._componentConstructor===vnode.nodeName;
}

The above function's first argument is a node. This could be of type 1 or 3, or just a number. A type checker will look at the use of splitText and think, node might be a number or might be a string. I don't know. Because type checking happens during code or build time, the checker can't tell what the actual value of node is. It knows you can't call splitText on a number or a DOM node, so it will complain about the use not being valid for them. In contrast, during runtime in the browser, when you call splitText on a number and the browser goes, So, this is a number, but you what to use a string function. Let me see if it has a toString method. It does. I'll convert it to a string. And everything is fine.

The only way to prevent these types of errors is to either escape code with [""] or do a conversion of number to string with something like:

if (typeof vnode==='string' || typeof vnode==='number') {
  // Manually convert numbers to string:
  if (typeof node === 'number') node = node.toString()
  return node.splitText !== undefined;
}

This makes type checkers happy because there is no question that the value using splitText is indeed a string. Hope that's clear. Making JavaScript code type safe can be a PITA because its forgiving nature encourages letting the browser handle coercion for us.

* @property {EventListenerMap} [_listeners] A map of event listeners added by components to this DOM node
* @property {import('../component').Component} [_component] The component that rendered this DOM node
* @property {Component} [_component] The component that rendered this DOM node
* @property {function} [_componentConstructor] The constructor of the component that rendered this DOM node
*/

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
11 changes: 8 additions & 3 deletions src/vdom/component-recycler.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@ export function collectComponent(component) {
(components[name] || (components[name] = [])).push(component);
}

/**
* @typedef {Component} CustomComponent
* @property {function} render
*/

/**
* 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 +43,8 @@ export function createComponent(Ctor, props, context) {
else {
inst = new Component(props, context);
inst.constructor = Ctor;
inst.render = doRender;
/** @type {CustomComponent} */(inst).render =
doRender;
}


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);
c = createComponent(
/** @type {(props, context) => void} */(vnode.nodeName),
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
Loading