Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
eternal-flame-AD opened this issue Jul 12, 2024 · 9 comments
Open

Comments

@eternal-flame-AD
Copy link
Contributor

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

@jrobinso regarding your #1663 (comment)

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:

  • Robust compile-time checking of correct API usage. Catching common mistakes like typo, extra arguments, conflicting information, etc.
  • Better static analysis and language server support, that means Intellisense almost always work.

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
Copy link
Contributor

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
Copy link
Contributor Author

eternal-flame-AD commented Jul 12, 2024

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:

  • Nowadays a significant number of dev use TS
  • they have a valid reason to use TS
  • these devs are having trouble using igv

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:

  • Reinvent wheels 3 times by writing their own declaration file.
  • Force TS to treat igv as a black box. This hack sort of works but will involve a lot of error-prone unsafe type casting & loss of advanced type checking capabilities (such as the 'genome' and 'reference' cannot both be present)
  • Just write JS (which is what I understand where you are standing at), but they have to accept loss of strong typing on the whole project(website) only for one dependency that won't interop with it. Additionally rewriting a modern frontend web app in JS is not trivial at all.
  • Petition to the package maintainer and discuss about setting up a unified, maintained declaration file that just works. This is why I opened this issue.

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
Copy link
Contributor Author

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: DefinitelyTyped/DefinitelyTyped@master...huxulm:DefinitelyTyped:948aba4b04f0f14f74b6dd32698d1ec2ff59596c

@jrobinso
Copy link
Contributor

jrobinso commented Jul 12, 2024

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
Copy link
Contributor Author

eternal-flame-AD commented Jul 12, 2024

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
Copy link
Contributor

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
Copy link

lk101 commented Aug 8, 2024

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
Copy link

lk101 commented Aug 8, 2024

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.

@eternal-flame-AD
Copy link
Contributor Author

eternal-flame-AD commented Oct 14, 2024

@lk101 You can reference my PR and consider adapting it to definitely-typed. I just want a useful TS definition so you might need to bring it up to definitely-typed standard (tests and such).

The PR is stalled and I don't really have any time to push it through DT's review with extensive tests. Verbatim copying from the PR changes I don't require any attribution, would be nice if you choose to use my code write my commit info in a comment or co-author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants