First-party TypeScript definitions

[maxkfranz]

Description of new feature

What should the new feature do? For visual features, include an image/mockup of the expected output.

Add first-party TS support in the library.

There was a GSOC project that started this out, in the typescript branch. The approach in that project was:

• Create JSDoc comments that contain TS-compatible type annotations.
• Revise the documentation build process to pull in the specifications for each public function in the library from the JSDoc comments, rather than the docmaker.json spec file.
• Ref: Typescript & JSDoc support for Cytoscape.js nrnb/GoogleSummerOfCode#94

The following tasks are required:

• Integrate TS bindings into the release process.
  • Verify that tools like VSC can pick up the typings.
  • Verify that TS projects automatically pick up the typings when importing Cytoscape.
• Validate that the revised documentation build process is comprehensive.
• Support a similar process for extensions. See also: ESM support for extensions #3167
• (Other tasks TBD...)

N.b. this would not involve porting Cytoscape to TS. The library would remain JS but would have native TS support for consumers.

Motivation for new feature

Describe your use case for this new feature.

• Improved support for downstream devs using Cytoscape in their TS-based apps.
• Better autocomplete/intellisense in tools like Visual Studio Code.
• And more...

For reviewers

Reviewers should ensure that the following tasks are carried out for incorporated issues:

• Ensure that the reporter has adequately described their idea. If not, elicit more information about the use case. You should iteratively build a spec together.
• Ensure that the issue is a good fit for the core library. Some things are best done in extensions (e.g. UI-related features that aren't style-related). Some things are best done by app authors themselves -- instead of in Cytoscape libraries.
• The issue has been associated with a corresponding milestone.
• The commits have been incorporated into the unstable branch via pull request. The corresponding pull request is cross-referenced.

[danprince]

@maxkfranz We'd love to help out with this one. We're keen TypeScript users (and big fans of JSDoc as an incremental step there).

I'd imagine it's going to be trickier than estimated in nrnb/GoogleSummerOfCode#94 though, because of the style of JavaScript that the codebase is written in.

The heavy use of constructor functions (as opposed to actual JS classes) and dynamically building prototypes out of mixins represents a bit of a challenge for TypeScript's JSDoc support. The usual escape hatch with this kind of dynamic JavaScript is to drop back to .d.ts files, but that may still be a level of duplication you'd hope to avoid, especially if the goal is to generate the docs from the source.

Either way, I can bring a good amount of hands on experience with TypeScript + JSDoc if that's useful here. Just let me know how you'd like to approach it!

[maxkfranz]

I'd imagine it's going to be trickier than estimated in nrnb/GoogleSummerOfCode#94 though, because of the style of JavaScript that the codebase is written in.

It was, but I think that's been mostly sorted.

The heavy use of constructor functions (as opposed to actual JS classes) and dynamically building prototypes out of mixins represents a bit of a challenge for TypeScript's JSDoc support. The usual escape hatch with this kind of dynamic JavaScript is to drop back to .d.ts files, but that may still be a level of duplication you'd hope to avoid, especially if the goal is to generate the docs from the source.

Yes. Some of the annotations in the typescript branch may feel a bit kludgey for this reason, but they should be working despite the pre-ES6/TS style.

Either way, I can bring a good amount of hands on experience with TypeScript + JSDoc if that's useful here. Just let me know how you'd like to approach it!

Great. A good first step would be to explore and experiment with the typescript branch to identify outstanding issues. The branch has some code to build a .d.ts file from the annotations, so the easiest starting point would be to evaluate the quality of that file and work backwards from there.

[danprince]

Although it looks like the doc comment coverage of the typescript branch is good, the annotations themselves are often incorrect and the build step is currently producing semantically and syntactically invalid TypeScript in the declaration file.

I guess the annotations were written with producing the documentation in mind, without much focus on whether the resulting TypeScript was correct. There doesn't seem to be a typescript dependency or a tsconfig.json in the branch, so I'm doubtful whether the types were ever even sanity checked as they were authored.

Here's an example of the type of semantic problem that is very common here. These are the annotations for autolock in src/viewport.js.

  /**
 * @typedef {object} cy_autolock
 * @property {object} NULL
 * @property {object} bool - A truthy value enables autolocking; a falsey value disables it.
 */

  /**
 * Get or set whether nodes are automatically locked (i.e. if `true`, nodes are locked despite their individual state).
 * @memberof cy
 * @path Core/Viewport manipulation
 * @param {...cy_autolock} bool - Get whether autolocking is enabled. | Set whether autolocking is enabled.
 * @methodName cy.autolock
 */
  autolock: function( bool ){
    if( bool !== undefined ){
      this._private.autolock = bool ? true : false;
    } else {
      return this._private.autolock;
    }

    return this; // chaining
  },

And here's the TypeScript it generates:

declare class cy {
  /**
   * Get or set whether nodes are automatically locked (i.e. if `true`, nodes are locked despite their individual state).
   * @param bool - Get whether autolocking is enabled. | Set whether autolocking is enabled.
   */
  static autolock(...bool: cy_autolock[]): void;
}

/**
 * @property bool - A truthy value enables autolocking; a falsey value disables it.
 */
declare type cy_autolock = {
    NULL: any;
    bool: any;
};

There are quite a few individual problems here.

• autolock ends up declared as a static method on cy. This is wrong, it should be a regular method.
• The return type isn't specified in the doc comments, and therefore defaults to void when it should be this or boolean.
• autolock should have two signatures. One where it takes no arguments and returns a boolean and one where it takes a boolean and returns this.
  • Instead, the annotations here say that autolock takes any number of objects with two properties (NULL and bool). I think the intention of the implementer here was probably to try and create a union type?

Compare to the handwritten types at @types/cytoscape:

/**
 * Get whether nodes are automatically locked
 * (i.e. if true, nodes are locked despite their individual state).
 * http://js.cytoscape.org/#cy.autolock
 */
autolock(): boolean;
/**
 * Set whether nodes are automatically locked
 * (i.e. if true, nodes are locked despite their individual state).
 * http://js.cytoscape.org/#cy.autolock
 *
 * @param bool A truthy value enables autolocking; a falsey value disables it.
 */
autolock(bool?: boolean): this;

autolock is declared correctly as method with two signatures.

Ideally people would transition from @types/cytoscape to cytoscape first party types seamlessly.

Here's how the comments would need to look to generate the correct types.

/**
 * Set whether autolocking is enabled.
 * @overload
 * @param {boolean} bool A truthy value enables autolocking; a falsey value disables it.
 * @return {Core}
 *
 * Get whether autolocking is enabled.
 * @overload
 * @return {boolean}
 *
 * Get whether nodes are automatically locked (i.e. if `true` nodes are
 * locked despite their individual state).
 * @this {Core}
 * @param {boolean | void}
 * @return {Core | boolean}
 */
autolock(bool){  // <-- I had to change from `autolock: function` so the @overload tag would work

I'm not just picking on the autolock type here, either. It's just a simple example of a pattern (and problem) that shows up again and again in these annotations. There are at least 51 methods that take the same approach to describing overloads with an object that has a NULL property.

The generated TypeScript declarations also contain a bunch of strange syntactic anomalies.

/**
 * Get a new collection containing clones (i.e. copies) of the elements in the calling collection.
 */
static elesfn.clone(): void;
/**
 * Get whether the element has been removed from the graph.
 */
static elesfn.removed(): void;
/**
 * Get whether the element has been removed from the graph.
 */
static elesfn.inside(): void;

The . isn't valid inside a member name. Not sure how this even got generated!

There are other strange artifacts that seem to have confused the code generator, producing unhelpful types and comments.

/**
 * @property selector - The selector the elements should match.
 * @property selector - The selector the elements should match.
 * @property selector - The selector the nodes should match.
 * @property selector - The selector the edges should match.
 * @property selector - The selector the elements should match.
 * @property filter_callback - The filter function that returns true for elements that should be returned.
 */
declare type cy_$ = {
    selector: any;
    selector: any;
    selector: any;
    selector: any;
    selector: any;
    filter_callback: (...params: any[]) => any;
};

My gut feeling reading through this is that the comments were steered towards working with tsd-jsdoc which is probably a redundant tool these days, now that TypeScript can compile .js files into .d.ts (along with IDE support for checking everything is correct as you author them).

I think if the primary goal here is to generate first party types, then it's going to be more work to fix these than it is to go from scratch using tsc to steer.

Happy to kick off that process, but would probably suggest breaking it down into some smaller chunks for PRs, rather than trying to land one huge one for everything.

Might as well float this question too, is there any interest in moving towards more modern JavaScript for this project generally?

[maxkfranz]

Happy to kick off that process, but would probably suggest breaking it down into some smaller chunks for PRs, rather than trying to land one huge one for everything.

Sounds good.

Might as well float this question too, is there any interest in moving towards more modern JavaScript for this project generally?

Yes.

[stale]

This issue has been automatically marked as stale, because it has not had activity within the past 14 days. It will be closed if no further activity occurs within the next 7 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

[maxkfranz]

Followed up in #3306