[Proposal] Provide TS type checking based on JSDoc annotations

[ma2ciek]

About half a year ago TS started parsing JSDoc comments inside JS, so the JS code can be statically type checked during the development without the need to write in TS. During the last hackathon, I've created a sample based on our codebase showing what can be achieved by introducing such a tool.

Here are some screenshots with examples of what the typed JS is capable of:

• Deep hints with proper docs:

• Advanced type checking:

• Support for generic types:

• Support for iterators:

A full list of all supported JSDoc tags so far is available here and the list is still growing. For now, the valuable missing ones for us are @property and @method - microsoft/TypeScript#28730, @impements (there's a workaround), @public, @protected and @private.

But there's not going to be all roses. TS doesn't directly support mixins. It supports only inheritance with one base class. This could be somehow workaround, but it would need to be thought through.

/**
 * @extends B
 * @extends C
 */
class A extends combineMixins( B, C ) {}

How to test it?

1. Add tsconfig.json file in the main repository

{
	"compilerOptions": {
		"allowJs": true,
		"checkJs": true,
		"resolveJsonModule": true,
		"lib": [
			"es2015",
			"es2016",
			"es2017",
			"dom"
		],
		"target": "es6",
		"moduleResolution": "node",
		"noEmit": true
	},
	"include": [
		"packages/ckeditor5-utils/src/**/*.js"
	]
}

2. Install TS locally or globally (npm install typescript).

3. To have IDE support, make sure that the typescript plugin is installed in your IDE (VS Code has built-in support).

4. Try to find if there's a enforce checkJs option in your IDE

If the above option isn't possible, try to add // @ts-check at the beginning of the file, check if that works for you.

5. Run tsc command.

I've created a branch for testing that has ~30 errors - https://github.com/ckeditor/ckeditor5-utils/tree/typechecking (mixins on this branch are temporarily removed)

Pros:

• The codebase is typed and most typos or issues like incorrect interface implementations can be found during the development.
• There's a strong connection between docs and code, a bi-directional check.
• Support for all advanced TS types, generic, mapped types, conditional types, strong JS code type inference, type casting.
• Type checking can be part of a CI with the tsc command.
• There're typings for packages that we use already - e.g. lodash, mocha.
• There's an option to add support for TS checking incrementally, there are // @ts-check, //@ts-nocheck and // @ts-ignore flags
• There's (an open ticket about generation Typescript Declaration files (.d.ts))[https://github.com/Allow --declaration with --allowJs microsoft/TypeScript#7546] - it will solve the Typings for TypeScript #504 out of the box.

Cons:

• All module:... references becomes invalid (so the codebase would need a massive search&replace refactoring).
• The tool for docs' generation needs some changes and refactoring - it would need to support relative references, import() references and relative references that comes from imports to name few.
• No support for @implements for classes (there's a workaround), @method and @property in classes doesn't work (methods an properties work out of the box if they are provided, so this issue touches only not defined ones).
• Mixins are a big unknown, it needs an investigation on how we could solve that issue.
• New tool that needs support.
• Not everyone is familiar with how the TS works (Although the deep knowledge isn't needed to start).
• Additional dependencies and config files.

Unknown:

• IDE support - this works in the VSCode, not sure how it works in others.

I'm interested in hearing your opinions.

[jodator]

I would love to see this working with CKE5 code. For me I see problems with:

All module:... references becomes invalid

which implies this (AFAICS):

The tool for docs' generation needs some changes and refactoring - it would need to support relative references, import() references and relative references that comes from imports to name few.

So it means that instead of: import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
we need to write import Plugin from '../../ckeditor5-core/src/plugin'; ?

No support for @implements, @method and @Property in classes.

Also is sad :(

[ma2ciek]

So it means that instead of: import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
we need to write import Plugin from '../../ckeditor5-core/src/plugin'; ?

No, it means that instead of @type {module:core/plugin~Plugin} you need to import it and write {@type Plugin} or write @type {import('@ckeditor/ckeditor5-core/src/plugin')}. The module:... is a custom absolute path so that' why TS and our IDEs don't understand them.

No support for @implements, @method and @property in classes.

For the usage of @implements for classes there's a workaround as I told earlier.

For methods and properties, all work for defined ones, but it's harder to use them with not defined.

E.g. in this scenario there's no need for anything extra:

class A {
  constructor() {
     this.b = new B();
  }

  someMethod() {
  }
}

But the following won't work.

class A {
  constructor() {
     /**
      * @member {String} prop
      */
      this.set( 'prop', 0 );
  }
}

It'd need to be changed to at least something like this:

class A {
  constructor() {
     /**
      * @type {String}
      */
      this.prop;

      this.set( 'prop', 0 );
  }
}

[ma2ciek]

• @implements support: JSDoc: support @implements tag on class microsoft/TypeScript#17498
• @property and @method support: JSDoc : add support for @method (and @property) microsoft/TypeScript#28730

[ma2ciek]

I didn't make it clear, but the TS team still works actively on the support for JSDoc: https://github.com/Microsoft/TypeScript/issues?q=is%3Aopen+is%3Aissue+label%3A%22Domain%3A+JSDoc%22

[pjasiun]

I think that since our code is already statically typed and considering the number of effort we put to describe all parameters in docs the most complicated part of the work is already done. Not using TypeScript at this point would we a huge waste of the potencial. The only question is when we will have time to work on it.

[pjasiun]

We recently found a bug which would be noticed automatically by TS: https://github.com/ckeditor/ckeditor5-engine/pull/1613/files/6c630fd38a634cd46c9433a896606cb814f719b0#r242981678

[Reinmar]

I've been asked about my thoughts so...

It looks extremely promising. Migrating our source code to TS doesn't seem to be a good option (see this thread – https://twitter.com/reinmarpl/status/1082199078463782912), but a lighter integration with TS makes much more sense. We'll be type safe, future safe and inclusive.

[jodator]

but a lighter integration with TS makes much more sense

Probably the most sane thing to do now :) Looking forward to it.

[ma2ciek]

I've started to work on some solution to the mixins problem but I've hit that issue - microsoft/TypeScript#18732

The idea of mixins was pretty promising though as mixins could be a part of the inheritance system which TS can understand - http://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/.

The corresponding TS code would look like this:

type Constructor<T> = new (...args: any[]) => T;

function Mixin1<T extends Constructor<{}>>(SuperClass: T = class {}) {
  return class extends SuperClass {
    foo() {}
  };
}

function Mixin2<T extends Constructor<{}>>(SuperClass: T = class {}) {
  return class extends SuperClass {
    bar() {}
  };
}

class C extends Mixin1(Mixin2()) {
  baz() {
    this.foo();
	this.bar();
  }
}

In short, the problem in the JS + JSDoc code is that JSDoc template types can't be narrowed, while TS requires to know here that a type is a constructor type.

[pjasiun]

The question is how often we do have 2 mixins for a single class. All I can find is DomEmitterMixin + ObservableMixin in some UI components (FocusTracker and View) and DataApiMixin + ElementApiMixin for editors. This is something we could solve if it would be the only blocker. However, there might be more combinations like extends + mixin. Or we might have other TypeScript blockers, not related to mixins.

[jodator]

This is something we could solve if it would be the only blocker. However, there might be more combinations like extends + mixin

I think that this is main reason that causes problem. But I'm not sure if we couldn't live with interafaces instead of mixins (in the docs)? oh.. microsoft/TypeScript#17498 So they don't support @implements annotation :/

[ma2ciek]

The number of such problems isn't big, but that number can grow at any time.

This is a list of files that use mixins and inheritance or many mixins at the same time

Observable plugins:

• packages/ckeditor5-autosave/src/autosave.js
• packages/ckeditor5-upload/src/filerepository.js

Emittable classes that inherit over basic classes:

• packages/ckeditor5-engine/src/model/liverange.js
• packages/ckeditor5-engine/src/model/liveposition.js
• packages/ckeditor5-ui/src/viewcollection.js
• packages/ckeditor5-engine/src/view/editableelement.js

Editors that mix DataApiMixin, ElementApiMixin? and inherit over the base Editor class:

• packages/ckeditor5-editor-balloon/src/ballooneditor.js
• packages/ckeditor5-editor-inline/src/inlineeditor.js
• packages/ckeditor5-editor-classic/src/classiceditor.js
• packages/ckeditor5-editor-decoupled/src/decouplededitor.js

Others:

• packages/ckeditor5-ui/src/view.js: Mixes DomEmitterMixin and ObservableMixin
• packages/ckeditor5-utils/src/focustracker.js: Mixes DomEmitterMixin and ObservableMixin.

I think that this is main reason that causes problem. But I'm not sure if we couldn't live with interfaces instead of mixins (in the docs)? oh.. microsoft/TypeScript#17498 So they don't support @implements annotation :/

Yep, for now, the only way to check that a class implements an interface is to check whether the instance of that class in the constructor is applicable to the given type (interface).

[ma2ciek]

To sum up the needed work to migrate the code and fix some problems, I'm creating a list of tasks needed to align our codebase and tools for this change:

• Mixins need to be changed to other forms (although the corresponding @mixes tags should be preserved to keep our JSDoc plugin working - It touches source and tests of all 6 mixins and their usage in the code.
  • estimate the effort in man-days
  • check whether new mixins won't create some problems (they will work internally differently as they will use the prototype inheritance instead of copying)
• All code references (module:...) in all places need to be changed
  • test if ESLint won't complain about references used only inside JSDoc comments, maybe there's a plugin for that
  • test if JSDoc can work with import() syntax and relative imports with a custom plugin
  • estimate the effort in man-days
• Test whether JSDoc is the optimal documentation parser Maybe it'd be worth to leave the buggy JSDoc3 and try something else (like https://github.com/TypeStrong/typedoc with a spec defined here: https://github.com/Microsoft/tsdoc, unfortunately, the output differs much from the JSDoc's output) as there's a chance that advanced type system and the above imports would work out of the box.
• Fix potential errors that come from typos / invalid interfaces or interface implementations
• Check whether our different semantic for the @protected tag is not a problem
• Create a migration guide containing some advanced usage examples and some tips
• Check how the TS configuration should look like (and where it should be stored - in the main repository or in the sub-repositories)
• Check how typings for our codebase should be released and where they should be stored (NPM package or DefinitelyTyped)
• Add type-checking script on the CI (10min-30min for all repositories)
• Check whether type-checking our tests is worth to do now.
• Check how to store typedefs and interface definitions (.md files / .js files or maybe .d.ts files) and how much work is needed
• Check other potential blockers

Notes:

JSDoc

JSDoc could be a big problem here. I've been trying to add some hooks so the parser, so the special type references (like @type {import()} can be normalized to the JSDoc-friendly way but I've failed. We're also kinda stopped here because the newest JSDoc is rewritten on top of the Babylon parser and we can't use it breaks our existing plugins. So the only way to use the JSDoc would be to fork the parser but it's not a long-term solution.

The potential solutions:

• find a better documentation parser []
• create a new generation parser that we'd need to maintain on top of the [Babel parser / TS parser (especially that sounds like a quick win: Using-the-Compiler-API) / Acorn]. If we'd want to go with interfaces written in .d.ts then TS parser should be a good solution. Otherwise, 🤷‍♂️ This task shouldn't be as big as I thought. I did a simple PoC using the Acorn and I could quickly achieve very simple doclets from tokenizer. But IMO Acorn isn't as future-proof as other 2 mentioned parsers.

TS configuration

The configuration could be stored in each package, so it will be easy to test it on CI. This will also improve the experience for anyone using TS supporting IDE) The include can be extended to src and tests directories if we'd like to type check our tests too.

{
	"compilerOptions": {
		"allowJs": true,
		"noEmit": true,
		"checkJs": true,
		"target": "es6"
	},
	"include": [
		"src/**/*.js"
	]
}

Potential blockers:

• No autocompletions for Parent class when using ES2015 mixins (function returning the Class) microsoft/TypeScript#18732 (mixins)
• JSDoc: support @implements tag on class microsoft/TypeScript#17498 (@implements tag)
• Allow --declaration with --allowJs microsoft/TypeScript#7546 (generating .d.ts files)

I'll be updating the above list.

[ma2ciek]

The summary - pros, cons, and blockers

Cons, blockers:

• Our mixins should be changed as TS's type of classes can't be changed - it can create some problems (see [Proposal] Provide TS type checking based on JSDoc annotations #1415 (comment)). It's also currently blocked because of the No autocompletions for Parent class when using ES2015 mixins (function returning the Class) microsoft/TypeScript#18732.
• A huge cost of migrating the whole codebase to TS (module: references, mixins, events)
• Migrating links (although they could stay as they are, it's not a must)
• Creating a doclet generator on top of the TS compiler.
• Blockers:
  • generating declaration files from JS code
  • @implements tag

Pros:

• TS community and people that use IDEs supporting TS would gain typings for our code. This would improve the DX much (code completion, type checking, docs) The community is quickly growing (Typings for TypeScript #504).
• Tool for type checking our docs and code, so docs won't be isolated as they are now. Caught typos that we have now.
• Code completion from IDEs - lower time cost for people jumping into the project or parts of the code that they don't know.
• Easier refactoring as symbols are connected
• Improvements in docs (e.g. for callbacks: Function -> (param1: typeA) => typeB).

[ma2ciek]

Recently a guide for creating TS-correct JS code was published on https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html

[isubasti]

looks like the only blocker left is microsoft/TypeScript#7546 ?but there is a pre-packed version for that microsoft/TypeScript#32372 (comment)

[ma2ciek]

Hi @isubasti, the real blocker is the huge amount of lines to rewrite (plus possible blockers that weren't caught yet) that requires dozens (or hundreds?) of man-days. The codebase constantly grows and this task gets bigger and more complicated with each passing day.

[phaux]

the problem in the JS + JSDoc code is that JSDoc template types can't be narrowed, while TS requires to know here that a type is a constructor type.

This works:

/**
 * @template { new(...args) => {} } T
 * @param {T} Super
 */
const Mixin = Super => class extends Super {
    mixinMethod() { }
}

Playground Link

[ma2ciek]

the problem in the JS + JSDoc code is that JSDoc template types can't be narrowed, while TS requires to know here that a type is a constructor type.

This works:

Yep, it works now thanks to the generic narrowing in the @template syntax that wasn't available in the past (microsoft/TypeScript#18732). However, this creates a little bit different syntax like applyMixin1(applyMixin2(class Foo{} )) which AFAIK @Reinmar wasn't sure about.

[ma2ciek]

With the 3.8 release TS will understand @private, @protected, @public and @readonly modifiers. However, the TS's semantic is different than our in case of the @readonly and @protected (in case of @readonly the property can't be changed outside of the constructor, in case of @protected the property/method can't be changed/executed outside of the non-inheriting class).

Ref: https://devblogs.microsoft.com/typescript/announcing-typescript-3-8-beta/, the JSDoc Property Modifiers section.

[jodator]

@ma2ciek I've just found this approach to mixin (intersection type). Would it solve some issues?

Without much digging into it it looks that intersection type follow what we do while using mixins - adding some methods/properties to type.

The extends path seems like opposite to what we do with mixins and it will probably never work.

[ma2ciek]

This would work if we would export something like mix( classA, mixinA, mixinB, ... ). Currently we export a class so its type can't be enhanced by some mixins doing some stuff after the class declaration.

I also guess that in our case it's not an intersection type, but the type can be somehow computed dynamically using some TS advanced types. The intersection type would work for plain object. In our case we need to mix a class with a class or an object that would ship missing methods.

[Reinmar]

I'm closing this ticket in favor of #11704.