Improved JSDoc comments for type correctness.

[rbiggs]

1. Improved JSDoc comments for better type check in VSCode with “implicitProjectConfig.checkJs": true” to enable type checking for JavaScript.
2. Introduced JSDoc type casting to solve problems with type coercion.
3. Escaped expando properties like “splitText” with [''] to resolve warning that property does not exist on object.
4. Updated JSDoc comment for VNode. Attribures property was set to type “object”. Unfortunately JSDoc types “object” and “Object” get treated as type “any” by TypeScript. To indicate a generic object you have to use this signature: Object.<string, any>.
5. Add type cast for Ctor being called with new keyword to avoid warning "Cannot use 'new' with an expression whose type lacks a call or construct signature".
6. Added type casting where type is a function, but original value could be a string as well. This tells type checker that the value can be called.
7. Added numerous type casts in diff.js to resolve type conflicts.
8. Many types had to be cast to type “any” in oder to remedy conflicts where type coercion would happen during runtime in browser, but static type checkers can’t foresee.
9. Type casting adds an extra pair of parens around the value being cast. But these are removed during minification.

[coveralls]

Coverage remained the same at 100.0% when pulling ce26426 on rbiggs:master into b8a117a on developit:master.

[developit]

Just a thought while reading through: where the original types were sortof declared globally, these are more inline. I worry about what that will mean for maintenance and contributions.

[rbiggs]

Well, you actually do have really good JSDoc comments already. The only way to deal with the type coercion is inline where the problem is. Would be the same problem though if you were writing TypeScript. However, if you turn on “implicitProjectConfig.checkJs": true” in the user settings for VSCode, you'll see any type problems right away with new additions. When I first opened Preact's sure code up, I thought there were going to be a lot more problems than there was.

That said, I can see your hesitance and don't necessarily consider that a bad thing. The change to type for VNode attributes is needed. object and Object do not accurately describe what attributes property is.

I used to do a lot of TypeScript. But now I just rely on JSDoc comments for the same benefits. No build and they don't affect final code size.

[developit]

Hi @rbiggs - I've been experimenting with checkJs a bit and I really do like what we get out of this (cross-module function arity checking is... amazing). I'm wondering though: for more complex types, do you think it would be possible to have the JSDoc annotations simply infer/import them from a .d.ts?

[rbiggs]

I'm not sure that's necessary, importing types from the .d.ts file. Already in your current JSDoc comments you are importing types from other files:

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

As a matter of fact, if you want to have a bunch of global types, you could make a dummy type module and import your types from there:

export function typeModule() {}
/**
 * @typedef {Object.<string, any>} FunnyObj
 * @property {string} name
 * // etc...
 */

The dummy function is required so that the file is recognized as a module. Define all your custom types in there so you can import them else where.
Then you can import funnyObject like this:

/**
 * @type {import('../typeModule').FunnyObj} SpecialObject

This lets you have all your custom types defined in one place, kinda, sorta like a .d.ts file.

Yup, checkJs in plain JavaScript is pretty amazing. You get 90%-95% of TypeScript's type safety inside JavaScript. I only write my JavaScript that way now. You catch funky things right away, you get great intellisense, renaming symbols across files, etc. in the source code, and no build step. Win win.I really feel this is a game changer for JavaScript developers.

[developit]

@rbiggs - are you on our slack chat?

[rbiggs]

I am now. Wobbabits

[brodycj]

I just raised rbiggs/preact#1 with a few more experimental ideas on top of these changes that pass lint and npm test including:

• minor JSDoc import fixes to pass tsc --checkJs
• minor cleanup of some JSDoc changes in src/vdom/diff.js
• add checkjs script (maybe should have been camelCase)
• move other JSDoc imports to the beginning, out of JSDoc @param items
• other minor updates

I would really love to see this change move forward. I gotta say that I find the idea of static type checking in JavaScript to be a revolutionary idea, getting ready to apply it to my own projects.

[marvinhagemeister]

ohh I like this one! Totally forgot about typedef 👍

[marvinhagemeister]

This seems strange to me. @rbiggs Can you elaborate on the thinking behind it?

[rbiggs]

That's a type cast. We're casting it to a basic function to avoid type error displayed in this image:

This is a common problem when dealing with constructor functions and TypeScript. A simple type cast resolve this problem.

[brodycj]

The following change I proposed in rbiggs/preact#2 should make this read a bit better:

--- a/src/vdom/component-recycler.js
+++ b/src/vdom/component-recycler.js
@@ -33,7 +33,9 @@ export function createComponent(Ctor, props, context) {
                inst;
 
        if (Ctor.prototype && Ctor.prototype.render) {
-               inst = new /** @type {(props, context) => void} */(Ctor)(props, context);
+               const ComponentCtor =
+                       /** @type {(props, context) => void} */(Ctor);
+               inst = new ComponentCtor (props, context);
                Component.call(inst, props, context);
        }
        else {

[marvinhagemeister]

The variables are fine, it's the typings that I have an issue with. It seems wrong and more like an issue with typescript if one has to define a class constructor as a plain function.

[rbiggs]

Actually, it's the wrong type being defined in the parameter:

/**
 * Create a component. Normalizes differences between PFC's and classful
 * Components.
 * @param {function} 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}
 */

The Ctor type is described as a generic function, which can't be used with the new keyword. Just need to change that to the same thing we used for the type cast:

/**
 * Create a component. Normalizes differences between PFC's and classful
 * Components.
 * @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}
 */

Then the type cast isn't necessary:

if (Ctor.prototype && Ctor.prototype.render) {
  inst = new Ctor(props, context);
  Component.call(inst, props, context);
}

[marvinhagemeister]

Seems like an unnecessary change here

[rbiggs]

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:

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.

[developit]

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?

[brodycj]

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.

[marvinhagemeister]

Seems like an unnecessary change

[rbiggs]

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

[brodycj]

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

[marvinhagemeister]

I love the @typedef declaration, that's a nice improvement 🎉 With the inline type comments I'm not sure I like them yet. Perhaps it needs to grow a bit on me.

[rbiggs]

I have to admit, this does take some getting used to. The way type casting is done feels a bit verbose, but there isn't any other way to resolve the problem of wrong types. Say you have an DOM object of type Node and you want to attach an event listener with addEventListener or set a value on it with setAttribute. No problem for a browser. But a type checker with flag this as a type error because those methods are found only on objects of type Element. Node has mostly positional properties to it. They are two very different interfaces. When you try to use an Element method on a Node, the browser calls the appropriate method from the Element interface so that you code just works, same as it does with type coercions. Type checkers cannot foresee this behavior, they just see that you are invoking properties that do not exist on the object. To get around this type problem you need to cast the type to something that makes sense to the type checker:

// The following object will be of type Node:
var p = document.getElementsByTagName('p')[0]
// Invoke type Element method on it (requires type casting):
/** @type {Element} */(p).addEventListener('click', handleClick)

Without the above type cast, you would get a warning that addEventListener does not exist on object of type Node. We need to cast it to type Element. Type casts require that the type be enclosed in parens. Those get removed during minification so they do not affect final output.

[brodycj]

I just raised rbiggs/preact#2 that fixes some things from rbiggs/preact#1, uses internal consts to clean up some of these inline JSDoc cast items, and other minor cleanups. Here is an example from src/vdom/diff.js:

@@ -235,8 +237,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 = /** @type {any[]} */(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--;

Should be less ugly though we still need the inline JSDoc casts in some form to benefit from checkJs in VSCode and tsc. The alternatives I can think of are port to TypeScript or lose the benefits of checkJs.

Advantages of using JSDoc comments over TypeScript I can think of include:

• no port needed
• easier build
• easier debugging
• easier for non-TypeScript user community members to get involved

[developit]

@brodybits the problem with hoisting c in that example is that it actually changes the bundled library size :(

This type stuff is really exciting and I'm eager to merge it, but because of the constraints we place on Preact's output we can't afford any code changes. It might be worth taking some of these things to the TypeScript team working on Salsa? They seem really open to suggestions.

[rbiggs]

Those changes, like c, were merely for better readability, reducing the complexity of a type cast. The original pull request made no changes to the code other than type information. This meant the output remained the same after the build :-)

@brodybits and I can take a look at keeping the code the same and getting the types right.

[brodycj]

I raised rbiggs/preact#4 to resolve some issues:

• use type instead of ugly expando strings in JavaScript
• inline some of the inline const declarations to reduce code size
• Transform JSDoc any -> object wherever possible (2 places)
• more src/vdom/diff.js fixes to completely preserve the original dist/preact*.js output

There are 2-3 places where the JavaScript keeps the intermediate results of array lookup. For a total of 9-byte increase of preact.min.js (8-byte increase of gzip size) this results in both cleaner JavaScript and probably a minor increase in performance. Here is a quick comparison of sizes: (table commented out, for now at least)

I would be happy to did remove the other inline results if so desired, wonder if there may be ways to save much more if we look hard enough?

I also raised GH-1171 with the changes proposed here and in rbiggs/preact#4 squashed and rebased on master.

[brodycj]

I just updated both rbiggs/preact#4 and GH-1171 (with proposed changes squashed and rebased) to preserve the generated JavaScript 100%, will raise separate PRs for items such as:

• store & use intermediate results such as array lookup
• cleanup const vs let declarations

[developit]

splitText is a property of Text nodes: https://developer.mozilla.org/en-US/docs/Web/API/Text/splitText

[rbiggs]

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.

[andrewiggins]

Thanks for doing this ❤️ Since Preact X has significantly changed the source of Preact, this PR will be hard to merge into master so I'm gonna close this PR. FWIW I think master has pretty good JSDoc comments too (and improved further by #2883). If you have any suggestions to make them better let us know what you find!

[brodycj]

Thanks! Unfortunately I do not have much time to take a look at PR #2883 now, we will see if @rbiggs will have any other input:)