igvteam / igv.js

Embeddable genomic visualization component based on the Integrative Genomics Viewer
MIT License
640 stars 226 forks source link

Make igv.js work with TypeScript: Add TypeScript API description #1846

Open eternal-flame-AD opened 2 months ago

eternal-flame-AD commented 2 months ago

Rel: #1663 The original issue was closed due to inactivity but I believe this is a useful feature to include.

@jrobinso regarding your https://github.com/igvteam/igv.js/issues/1663#issuecomment-1633602596

Hi, no it doesn't, it is plain javascript, specifically client (web browser) javascript. [...] I do not know what you mean by "support typescript".

Currently TypeScript cannot accept igv.js because it has no type information available. However TypeScript has a easy and extensible way to accommodate JavaScript programmers: A declaration file that describes the JavaScript API interface: like a C header file but it never compile into any code so existing JavaScript users are impossible to be affected by the inclusion of this extra file in the dist. Compiler references this file to provide:

So I would argue this can provide significant convenience to TypeScript users while at a low risk of regression so I'm suggesting support this in-tree other than out-of-tree.

Raw imports such as

import igv from 'igv'

are not supported in browsers at this time, unfortunately.

Modern frontend development use bundlers like webpack/vite/etc, even when targetting browsers they like ESM more and then use the bundler to take care of the resolution, so the import never gets to the browser.

I have made a starter example on writing this declaration and actually using the declaration in TS code.. together I also wrote a purposefully badly written code that can barely stumble through the Javascript engine. If you compare the helpfulness of JS alone vs. TS the improvement is drastic.

Stackoverflow 2013 survey shows (# of ts users) / (# of js users) is around 2/3 so it is already very widely used.

I would like the maintainers to reconsider supporting TypeScript by distributing in-tree declaration files. I want to get a green light from maintainers before committing to finishing the whole thing. Additionally I am willing to devote time to maintain the definitions should the API change in the future.

Thanks!!

jrobinso commented 2 months ago

I am unsure what you are requesting, are you asking us to include a file such as this one in our repository that you are offering to develop? https://github.com/eternal-flame-AD/igvjs-ts-demo/blob/main/src/igv.d.ts. And what exactly is an "in tree declaration file"? Actually you mention files (plural), is there one declaration file or many?

I don't have time or interest in debating the merits of TS or webpack here, and that is not the purpose of the igv.js issues forum, I just want to understand clearly what you are proposing. Please don't assume any TS knowledge in the explanation.

eternal-flame-AD commented 2 months ago

Thanks for the response. Yes in summary I want to commit as few as one single file to the repo.

I hope the following clarify things better and addresses your concern:

A little misunderstanding

I don't have time or interest in debating the merits of TS or webpack here.

I think you misunderstood, I included these information to try to establish that fixing this issue can benefit a lot of developers in the Web/JS ecosystem (and I will explain shortly why), not to push any ideology. Apologies if I focused too much on this so the meaning of the text was skewed. All I wanted to say was:

What is TS and what does this have to do with a JS project?

TS is a syntactic and semantic superset of JS. It writes like JS and transpiles to JS but contain strong typing information to aid in static analysis and auto-completion. TS is (nearly) always transpiled to JS (in other words, type-checked and then stripped of the typing information) and then be executed on a browser/node/etc.

What are the files I want to include?

It's called a declaration file, which is a subset of TS that is merely to inform the TS static analyzer the presence and proper usage of an external API (such as igv). I do not believe there is a full name to declaration files, but they are also known as .d.ts or DTS files. All mean the same thing. Without it TS has no way of knowing what API the igv package exports. In this case if a downstream TS project want to import igv, the analyzer will refuse to take it because it is unsafe. The 3 commenters in #1663 are all stuck at this point. They have these options:

For extra robustness, tests can be written for these declaration files too, these will be extra files in the name of *.test.ts if we decide to write them, we don't have to (IMO their utility is limited). These tests are not actually run on an engine (and thus only depends on the DTS files, the real JS code is not involved). You only specify: expect createBrowser({ genome: 'hg19' }) be accepted, expect createBrowser({ }) to be rejected, etc.

How many files?

It's just like how you write JavaScript. You can write everything in one file or you can separate them and import/export between them. If you decide to separate them then at build you will use a tool like @microsoft/api-extractor to roll them up into one file.

On the package user side, TS will look for "index.d.ts" in the package or whatever you specify in package.json#types and then it will know how to deal with your program even if it is not written in TS & contain no type information.

Here comes in the second option I proposed which is to do this off-tree. In that case your repository stays the same and I publish an empty npm package that only contain the definition file and use declare module 'igv' to link it to your package. The biggest downside of doing it this way is it is easy for the user to get the 'igv' but the '@types/igv' is for a different version of 'igv'. This risk however can be minimized by sticking to strict semver guidelines and not changing API (including the type of things a function can take!) without a proper version bump. Thus regardless I would like to keep you in the loop about this.

In terms of size I estimate a single-file declaration of the public API would be 400-1000 lines, sorry for the big variance I have not got into all the nitty gritty minute detail of the API.

eternal-flame-AD commented 2 months ago

To give a visual on how it would look when done:

Here's an example of bundling DTS with the package itself: https://github.com/aws/aws-sdk-js/tree/master/lib

Here's an example of out-of-tree DTS definition: https://github.com/DefinitelyTyped/DefinitelyTyped/compare/master...huxulm:DefinitelyTyped:948aba4b04f0f14f74b6dd32698d1ec2ff59596c

jrobinso commented 2 months ago

Thanks for the detailed explanation, I think I have a clearer picture of what is involved and agree this would be useful. I am open to either (1) distributing a single index.d.ts file, or (2) doing it from your repository. We never change the API without a the proper semvar version bump, in fact we virtually never change existing functions at all, the only changes are new additions and those are infrequent.

One thing I'm still a bit confused on, where does index.d.ts live? I assume (in our repository) in the js directory. However we distribute packages created with rollup (dist/igv.esm.js, etc). Would we also distribute an index.d.ts file in the dist directory?

I do not have time to develop this but will review it, and can answer API questions where types are not specified or ambiguous.

eternal-flame-AD commented 2 months ago

Glad we could reach an agreement.

One thing I'm still a bit confused on, where does index.d.ts live? I assume (in our repository) in the js directory. However we distribute packages created with rollup (dist/igv.esm.js, etc). Would we also distribute an index.d.ts file in the dist directory?

The actual source of the DTS can technically live anywhere but I agree with you to just squeeze it in the js directory. Yes, the DTS should be copied into dist folder at build and published on npm.

One small correction: I thought about it if in the near future we never intend to add other modules other than "igv.js". It would be prettier to just call the DTS igv.d.ts and then write in {"types": "dist/igv.d.ts"} on package.json. The distributed package would look like this, better than a single index.d.ts that it just blends in the file structure:

yume@tsubasa ~/O/A/2/p/i/node_modules (main) > tree igv/   (base) 13:50:41
igv/
├── dist
│   ├── igv.d.ts (Created)
│   ├── igv.esm.js
│   ├── igv.esm.min.js
│   ├── igv.esm.min.js.map
│   ├── igv.js
│   ├── igv.min.js
│   └── igv.min.js.map
├── LICENSE
├── package.json (Modified)
└── README.md

2 directories, 10 files

I propose we do it this way, I will keep updating the PR and start discussions on places I need help, I will explain any repercussions if it involves a choice of strict/lax typing that is not obvious. An typical example (but in this case trivial so I won't ask this question) issue we certainly would have is:

interface SortOptionsLax {
    order: string;
}
// VS.
interface SortOptionsStrict {
  order: "ASC" | "DESC";
}

When you have time just go in the PR and reply on the discussions. If I feel too much is piled up without review I will mention you (or any people you specify) and slow down to let you catch up.

@jrobinso Let me know if you are ready for me to start working.

jrobinso commented 2 months ago

I will have created a branch "typescript" to which you can submit PRs.

I won't have much time to review this, so let's start incrementally. Perhaps first with just the TS you need for your application. Something very minimal but that can be tested.

lk101 commented 1 month ago

I think this is a very meaningful improvement for the promotion of igv.js. If possible, please let me participate in this work together. I think the introduction of typescript will be helpful for those who want to use igv.js more effectively. Facilitate them to use the API provided by igv.js more quickly.

lk101 commented 1 month ago

There are two ways to add corresponding API descriptions. You can add index.d.ts within this project, or create a separate @types/igv npm project, which is also a good choice.