Add first-party Typescript support #3308

maxkfranz · 2024-12-17T16:07:23Z

Cross-references to related issues. If there is no existing issue that describes your bug or feature request, then create an issue before making your pull request.

Associated issues:

Notes re. the content of the pull request. Give context to reviewers or serve as a general record of the changes made. Add a screenshot or video to demonstrate your new feature, if possible.

Adds the community Typescript definition file from DefinitelyTyped and improves it to cover API that the community definition file is currently missing.
Updates the package.json file to point to the index.d.ts definition file so that Typescript projects that import cytoscape will use the latest definition file.
This will make it easier for external contributors to make contributions to the definition file, as it has higher visibility in the main Cytoscape repo.
When new API is introduced in the library, the Typescript definition file should be accordingly updated. The PR issue template has been updated with a reminder for this.
This includes the provisional/experimental WebGL API (Initial, experimental WebGL rendering support #3305).
Note: This doesn't cover extensions. Extensions will have to be covered separately.

Checklist

Author:

The proper base branch has been selected. New features go on unstable. Bug-fix patches can go on either unstable or master.
Automated tests have been included in this pull request, if possible, for the new feature(s) or bug fix. Check this box if tests are not pragmatically possible (e.g. rendering features could include screenshots or videos instead of automated tests).
The associated GitHub issues are included (above).
Notes have been included (above).

Reviewers:

All automated checks are passing (green check next to latest commit).
At least one reviewer has signed off on the pull request.
For bug fixes: Just after this pull request is merged, it should be applied to both the master branch and the unstable branch. Normally, this just requires cherry-picking the corresponding merge commit from master to unstable -- or vice versa.

Ref: First-party Typescript support #3306

Ref: First-party Typescript support #3306 Ref: Initial, experimental WebGL rendering support #3305

Ref: First-party Typescript support #3306

maxkfranz · 2024-12-17T16:09:45Z

@d2fong, since you have a lot of TS experience I've added you as a reviewer to this PR. Since this is an update of the already-working community definition file, I don't expect any major issues -- but please let us know if you notice any red flags or best practices that aren't being followed. Thanks

maxkfranz · 2024-12-17T16:12:38Z

@mikekucera, I've cleaned up the commits for this PR so your WebGL API is in a separate commit (3a9c3a1). Let me know if you notice any discrepancies, or if you plan to change the API before the release. Thanks

mikekucera · 2024-12-17T18:08:30Z

Hi Max, I'd love to give this a proper review, but that might take some time since its a big file and I'm new to TypeScript. Are you ok with waiting for a review from me?

maxkfranz · 2025-01-06T15:15:16Z

Sounds good

d2fong · 2025-01-06T16:06:27Z

index.d.ts

@@ -2033,6 +2058,8 @@ declare namespace cytoscape {
        includeTargetLabels?: boolean | undefined;
        /** A boolean indicating whether to include overlays (such as the one which appears when a node is clicked) in the bounding box (default true). */
        includeOverlays?: boolean | undefined;
+        /** A boolean indicating whether to include underlays in the bounding box (default: true). */
+        includeUnderlays?: boolean | undefined;


This is a minor style nitpick -- when you have optional properties, the general style convention is that you don't need to include the | undefined part. I noticed that some lines in this community file follow that convention and some don't.

d2fong · 2025-01-06T16:11:01Z

@maxkfranz I reviewed the specific commits that include your additions to the community ts file. I don't see anything that looks out of place. How would you recommend that I test this out? If I install this branch in cytoscape-web, does this mean we shouldn't need to use the @types/cytoscape package anymore?

mikekucera

I skimmed the file and made some minor comments. The file is quite large and I'm new to TypeScript so I can't really comment on if the overall structure of all the type definitions are correct. My recommendation is to somehow mark the entire file as provisional so that we can fix any problems when the community finds them. We don't want to be locked into an API like we are with Cytoscape Java, at least until this file gets a decent amount of use from the community.

mikekucera · 2025-01-06T16:51:11Z

index.d.ts

+ *      The input must be a single edge.
+ *
+ * For a output of a function it will always give:
+ *  - Cy.CollectionElements


Should this be Cy.Elements? I don't see Cy.CollectionElements anywhere else in this file.

mikekucera · 2025-01-06T17:00:13Z

index.d.ts

+        /**
+         * The [[Stylesheet]] used to style the graph. For convenience, this option can alternatively be specified as a promise that resolves to the stylesheet.
+         */
+        style?: Stylesheet[] | Promise<Stylesheet[]> | undefined;


The style property is an array of StyleSheets? This naming seems a bit strange.

For example:

style: [ { selector: "node", style: { width: 20, height: 20 } }, { selector: "edge", style: { "curve-style": "unbundled-bezier", "control-point-weight": 0.5, "control-point-distance": 20 } }, ],

According to this type definition each object in the array has type Stylesheet. Is that right? Don't we think of the entire array as a Stylesheet? With each element being a style definition or style block.

mikekucera · 2025-01-06T17:01:39Z

index.d.ts

+         * The batch size for WebGL.
+         * (provisional, may change in future releases)
+         */
+        webglBatchSize?: number | undefined;


^ also needs a warning

mikekucera · 2025-01-06T17:05:37Z

index.d.ts

+         *
+         * @param name The name of the field to get.
+         */
+        data(name?: string): any;


There are declarations for ElementDataDefinition, NodeDataDefinition, EdgeDataDefinition above. Should they be used here?

Also 'data' can't contain values that are non-serializable right? Shouldn't that be captured in the type? The 'any' type seems way too permissive. That's what scratch is for.

mikekucera · 2025-01-06T17:10:56Z

index.d.ts

+         * @param name The name of the field to set.
+         * @param value The value to set for the field.
+         */
+        data(name: string, value: any): this;


The value argument can't be any, it has to be serializable Json data right?

mikekucera · 2025-01-06T17:14:10Z

index.d.ts

+         * Get the element's plain JavaScript object representation.
+         * http://js.cytoscape.org/#ele.json
+         */
+        json(): string;


This does not return a string, it returns a JSON object.

maxkfranz added 5 commits December 17, 2024 10:57

Add initial community file

71579f1

Ref: First-party Typescript support #3306

Update TS definition with missing style properties and functions

c0f489a

Ref: First-party Typescript support #3306

Add WebGL properties

3a9c3a1

Ref: First-party Typescript support #3306 Ref: Initial, experimental WebGL rendering support #3305

Update package.json for TS support

1bc6325

Ref: First-party Typescript support #3306

Update PR template checklist re. TS

8bc94bd

Ref: First-party Typescript support #3306

maxkfranz requested review from d2fong and mikekucera December 17, 2024 16:07

This was referenced Dec 17, 2024

First-party Typescript support #3306

Open

TypeScript Definition for Cytoscape.js - It is being worked on now! #1012

Closed

d2fong reviewed Jan 6, 2025

View reviewed changes

mikekucera approved these changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add first-party Typescript support #3308

Add first-party Typescript support #3308

maxkfranz commented Dec 17, 2024 •

edited

Loading

maxkfranz commented Dec 17, 2024

maxkfranz commented Dec 17, 2024

mikekucera commented Dec 17, 2024

maxkfranz commented Jan 6, 2025

d2fong Jan 6, 2025 •

edited

Loading

d2fong commented Jan 6, 2025

mikekucera left a comment

mikekucera Jan 6, 2025

mikekucera Jan 6, 2025

mikekucera Jan 6, 2025

mikekucera Jan 6, 2025

mikekucera Jan 6, 2025

mikekucera Jan 6, 2025

mikekucera Jan 6, 2025

Add first-party Typescript support #3308

Are you sure you want to change the base?

Add first-party Typescript support #3308

Conversation

maxkfranz commented Dec 17, 2024 • edited Loading

maxkfranz commented Dec 17, 2024

maxkfranz commented Dec 17, 2024

mikekucera commented Dec 17, 2024

maxkfranz commented Jan 6, 2025

d2fong Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

d2fong commented Jan 6, 2025

mikekucera left a comment

Choose a reason for hiding this comment

mikekucera Jan 6, 2025

Choose a reason for hiding this comment

mikekucera Jan 6, 2025

Choose a reason for hiding this comment

mikekucera Jan 6, 2025

Choose a reason for hiding this comment

mikekucera Jan 6, 2025

Choose a reason for hiding this comment

mikekucera Jan 6, 2025

Choose a reason for hiding this comment

mikekucera Jan 6, 2025

Choose a reason for hiding this comment

mikekucera Jan 6, 2025

Choose a reason for hiding this comment

maxkfranz commented Dec 17, 2024 •

edited

Loading

d2fong Jan 6, 2025 •

edited

Loading