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

Add TypeScript DTS file #1847

Open
wants to merge 1 commit into
base: typescript
Choose a base branch
from

Conversation

eternal-flame-AD
Copy link
Contributor

Fixes #1846

Demo (and test) are available here. Build instruction included in README

@eternal-flame-AD
Copy link
Contributor Author

@jrobinso You may see a lot of tracks are blank at first and only appear when you change the locus. This is not my fault. I tested the original code without modification it still does that, I even compared the sha256sum of generated js file my changes won't have an effect on compiled js code. Current tagged release does not do this. Unrelated regression somewhere, can you look into it?

@eternal-flame-AD
Copy link
Contributor Author

eternal-flame-AD commented Jul 12, 2024

Additionally we can pull in the test code and validate the DTS right in this repo. But we will need to pull in TypeScript package at the very least to make this work. I think a better idea is I (or igvteam) maintain the test file in a separate repo and during CI you can just validate by checking if my repo compiles.

@jrobinso
Copy link
Contributor

I won't be able to look at this quickly, we are focused on other things. Ping this PR again in a week if no response.

RE the blank tracks, could you create a standalone test case (just html & igv.js) that reproduces the issue?

@jrobinso
Copy link
Contributor

jrobinso commented Jul 12, 2024

I don't understand this instruction, exactly what artifact are you referring with the phrase "(compiled) dev igv.js source tree"?

Please put the (compiled) dev igv.js source tree in ../igv.js

also, ../ would be literally one directory up from the repository

@eternal-flame-AD
Copy link
Contributor Author

eternal-flame-AD commented Jul 12, 2024

@jrobinso Clone both repo into one single folder is what I'm trying to say, like a monorepo , like this:

yume@tsubasa ~/source > ls -lah igv-ts/                    (base) 17:11:26
total 7.7M
drwxr-xr-x  4 yume yume   99 Jul 12 17:05 ./
drwxr-xr-x 52 yume yume 4.0K Jul 12 14:28 ../
drwxr-xr-x 12 yume yume 4.0K Jul 12 16:55 igv.js/
drwxr-xr-x  7 yume yume 4.0K Jul 12 15:14 igvjs-ts-demo/

The confusion is because this repo itself is called igv.js

This way I can get immediate feedback on the test results as soon as I save the DTS, we don't pull in typescript so it is impossible to do it all within this repo.

@eternal-flame-AD
Copy link
Contributor Author

eternal-flame-AD commented Jul 13, 2024

I found an easier way to test, but you need to have a global TS compiler and any editor with TS language server ready. Create js/tests.ts, start with import igv, { ... } from './igv' and then put whatever you want to validate there. (of course this won't run bc there is no actual file backing this import, it's just type checking) I uploaded my current test here. Everything in this file except track setups are type checked.

https://gist.github.com/eternal-flame-AD/08a034ce4e1c98f5935d5c7c05eeac1b

I'm 90% confident that actually all you need is igv.d.ts and tests.ts in the same folder I think, you don't even need the JS code base.

Install TS compiler: npm install typescript, not sure which editor you use but it shouldn't be too hard to figure out integration.

@eternal-flame-AD
Copy link
Contributor Author

The JSON session exports (browser.toJSON()) are not stable right? I should just say it is a map of some sort?

@eternal-flame-AD eternal-flame-AD force-pushed the typescript-dts-patch branch 2 times, most recently from cdcaef8 to ba11cb7 Compare July 14, 2024 06:02
@jrobinso
Copy link
Contributor

I don't understand the question. I think the wiki doc is accurate on this.

@eternal-flame-AD
Copy link
Contributor Author

eternal-flame-AD commented Jul 14, 2024

@jrobinso

I don't understand the question. I think the wiki doc is accurate on this.

The wiki only says:

Return the current state of the browser as a JSON object. This object can be loaded with loadSessionObject.

It doesn't say should the user have any expectation on the stability of the structure of this returned object. If there is no stability guarantee I should type it as a black box. If it is stable I can type them so it's easier and safer to manipulate

@jrobinso
Copy link
Contributor

jrobinso commented Jul 14, 2024 via email

@jrobinso
Copy link
Contributor

In case its helpful, I am rewriting the doc for the upcoming release and replacing the wiki with the following site. The information should be the same wrt the "browser" api. A preview can be found here: https://igv.org/doc/igvjs/

@eternal-flame-AD
Copy link
Contributor Author

Thanks for letting me know! I will reference the new docs first then!

@eternal-flame-AD
Copy link
Contributor Author

Hi just an update, sorry I got a bad viral infection and hasn't been able to work the past week, I am still intending on finishing this

@jrobinso
Copy link
Contributor

No problem, get well!

@PeterKnealeCMRI
Copy link

@eternal-flame-AD Thanks for putting this together. I am currently embarking on TS rewrite of a JS app that uses IGV and this is hugely helpful.

@eternal-flame-AD
Copy link
Contributor Author

@PeterKnealeCMRI Thanks for the support! I think the definition is at a usable state where most common operations can be type-checked just fine (and it isn't hard to override type-checking one-short in TS at all). If you want to use it now you can copy the definition file to your project and wrap it in declare module "igv" { ... } and it will be automatically applied!

@eternal-flame-AD
Copy link
Contributor Author

eternal-flame-AD commented Sep 2, 2024

I think all documented APIs are done.

We may want to write JSdocs into the definition file but I am still considering whether to do it or not. The benefit is users can see documentations in their IDE as they write but the downside is yet another place to manage your documentations.

I have gone ahead and type-checked the examples/ folder. (some uses undocumented APIs so probably won't get type hints but they would still compile).

This is all the additional diff that is needed to make all examples type check: mainly ambiguous semicolons that TS doesn't like, type hints and type imports, the only semantic difference is "labelIsAnnotatedJunction" to which I did not find any implementation of so I commented it out instead of writing it into the definition.

Diff file in examples
examples_ts/custom-reader.html_0.ts -> examples_ts_fixed/custom-reader.html_0.ts
40a41,42
> 
>     const igvDiv = document.getElementById("igvDiv");

===========
examples_ts/custom-track-click.html_0.ts -> examples_ts_fixed/custom-track-click.html_0.ts
14d13
< var igv_custom_track_click = $('#igv-custom-track-click')
16c15
< igv.createBrowser(igv_custom_track_click.get(0), options)
---
> igv.createBrowser(document.getElementById("igvDiv"), options)
20c19
<         var genesInList = {}
---
>         var genesInList: { [key: string]: boolean } = {}
32d30
<                 $("#geneList").append('<li><a href="https://uswest.ensembl.org/Multi/Search/Results?q=' + symbol + '">' + symbol + '</a></li>')

===========
examples_ts/custom-webservice.html_0.ts -> examples_ts_fixed/custom-webservice.html_0.ts
7c7
< const cytobands = {
---
> const cytobands: TrackLoad<TrackType> = {
36c36
< const cbio1 = {
---
> const cbio1: TrackLoad<TrackType> = {

===========
examples_ts/roi-api.html_0.ts -> examples_ts_fixed/roi-api.html_0.ts
6c6
< const browser_config =
---
> const browser_config: CreateOpt =
42,44c42,44
<     const browser = await igv.createBrowser((document.getElementById('myDiv') as HTMLElement), browser_config)
<     (document.getElementById("roi-load-button") as HTMLElement).addEventListener('click', () => browser.loadROI(roi_configs))
<     (document.getElementById("roi-clear-button") as HTMLElement).addEventListener('click', () => browser.clearROIs())
---
>     const browser = await igv.createBrowser((document.getElementById('myDiv') as HTMLElement), browser_config);
>     (document.getElementById("roi-load-button") as HTMLElement).addEventListener('click', () => browser.loadROI(roi_configs));
>     (document.getElementById("roi-clear-button") as HTMLElement).addEventListener('click', () => browser.clearROIs());

===========
examples_ts/spliceJunctions.html_0.ts -> examples_ts_fixed/spliceJunctions.html_0.ts
30c30
<                 labelIsAnnotatedJunction: " [A]",
---
>                 // labelIsAnnotatedJunction: " [A]",

===========
examples_ts/tracks-hg38.html_0.ts -> examples_ts_fixed/tracks-hg38.html_0.ts
109c109
<         })
---
>         });
112c112
<             () => (window as any).history.pushState({}, "IGV", browser.sessionURL()))
---
>             () => (window as any).history.pushState({}, "IGV", browser.sessionURL()));

===========
This is the python script I used to extract all the scripts
import argparse
import os
from bs4 import BeautifulSoup
import re

def unindent(content):
    lines = content.splitlines()
    min_indent = None
    for line in lines:
        if line.strip():
            indent = len(line) - len(line.lstrip())
            if min_indent is None or indent < min_indent:
                min_indent = indent
    
    return "\n".join(line[min_indent:] for line in lines)

GET_ELEMENT_BY_ID_PATTERN = re.compile(r"document.getElementById\(([^)]+)\)")

def fix_getelementbyid(content):
    def repl(match):
        return f"(document.getElementById({match.group(1)}) as HTMLElement)"
    
    return GET_ELEMENT_BY_ID_PATTERN.sub(repl, content)

CONST_OPTIONS_PATTERN = re.compile(r"(const|var|let) options(\d*) ")

def fix_const_options(content):
    def repl(match):
        return f"{match.group(1)} options{match.group(2)}: CreateOpt "

    return CONST_OPTIONS_PATTERN.sub(repl, content)

CONST_TRACKS_PATTERN = re.compile(r"(const|var|let) tracks(\d*) ")

def fix_const_tracks(content):
    def repl(match):
        return f"{match.group(1)} tracks{match.group(2)}: TrackLoad<TrackType>[] "

    return CONST_TRACKS_PATTERN.sub(repl, content)

def process_html(html_path, dest_path):
    with open(html_path, 'r') as fp:
        soup = BeautifulSoup(fp, 'html.parser')
    
    script_tags = soup.find_all('script')

    if not script_tags:
        print(f"No script tags found in {html_path}")
    
    for i, script in enumerate(script_tags):
        if script.string:
            content = unindent(script.string)
            content = r"import type { CreateOpt, TrackLoad, TrackType } from '../js/igv';" + "\n" + content

            content = content.replace("../../dist/igv.esm.min.js", "../js/igv")
            content = content.replace("../../dist/igv.esm.js", "../js/igv")
            content = fix_getelementbyid(content)
            content = fix_const_options(content)
            content = fix_const_tracks(content)
            content = content.replace("window.", "(window as any).")
            
            ts_path = os.path.join(dest_path, f"{os.path.basename(html_path)}_{i}.ts")
            with open(ts_path, 'w') as fp:
                fp.write(content)



def main():
    parser = argparse.ArgumentParser()
    parser.add_argument('html_files', nargs='+', help='HTML files to process')
    args = parser.parse_args()
    
    dest_path = "examples_ts/"
    os.makedirs(dest_path, exist_ok=True)
    
    for html_path in args.html_files:
        dest_file_name = os.path.basename(html_path).replace(".html", ".ts")
        dest_file_path = os.path.join(dest_path, dest_file_name)
        os.makedirs(os.path.dirname(dest_file_path), exist_ok=True)

        process_html(html_path, dest_path)

if __name__ == "__main__":
    main()

The command is tsc --module es2022 -t es2017 --noEmit examples_ts_fixed/**.ts

@eternal-flame-AD
Copy link
Contributor Author

A demo file:

import type { Tracks } from './igv';
import igv from './igv';

let b = await igv.createBrowser(document.getElementById('igv-app')!, {
    // you get completions for possible hosted genomes based on docs, but if you specify a different string it will still accept it
    // if you start an object, you get hints for required and optional fields for custom genomes
    "genome": "hg38",
    "tracks": [
        // this definition is missing a data source, so this will not check
        // @ts-expect-error
        {
            // you will get completions for possible track types
            "type": "wig",
            // once you specify a type, you get completions for possible formats only for that type
            "format": "bigWig",
        },
        // this will check all options are valid for an annotation track
        {
            "type": "annotation",
            "url": "/data/genes/genes.gff3",
            "displayMode": "EXPANDED",
        },
        // this will not check, format mismatches type
        // @ts-expect-error
        {
            "type": "annotation",
            "url": "my.bigWig",
            "displayMode": "EXPANDED",
        },
        {
            // even just a url will infer the type
            // the options will be restricted to the inferred type and wrong options will be caught
            // however autocomplete will still show all possible options, possible due to reduce performance cost
            "url": "my.vcf.gz?param=1",
        },
        // this is valid
        {
            "type": "seg",
            "isLog": true,
            "features": [],
        },
        // gzipped files can be inferred too, `isLog` is not for variant tracks, so this will not check
        // @ts-expect-error
        {
            "url": "my.vcf.gz?param=1",
            "isLog": true,
        },
        // you can override things if you really need to
        {
            "type": "alignment",
            "format": "sam" as Tracks.AlignmentFormat,
            "sort": {
                "chr": "chr1",
                "position": 123456,
                "option": "BASE",
            },
            "url": "my.sam",
            ...({
                "somethingNew": "yes",
            } as {})
        },
        {
            "type": "gwas",
            "format": "bed",
            "columns": {
                // this should take a number, so this will not check
                // @ts-expect-error
                "chromosome": "chr1",
                "position": 1,
                "value": 1,
            }
        }
    ],
});

b.search('chr1:123456-123457');

// unfortunately here you need to give it a little encouragement to return the subtype instead of the generalized type
let t = await b.loadTrack<'alignment'>({
    "url": "my.bam",
});

// now you can use the alignment track-specific methods
t.sort({
    "chr": "chr1",
    "position": 123456,
    "option": "BASE",
    // options is not 'TAG', so this will not check
    // @ts-expect-error
    "tag": "AS",
})

b.on('trackclick', (track, _popoverData) => {
    // this gets inferred as a track
    let _: Tracks.Track = track;

    return true;
});

b.on('trackremoved', (track) => {
    // this gets inferred differently based on the event name
    let _: Tracks.Track[] = track;
});

@eternal-flame-AD
Copy link
Contributor Author

eternal-flame-AD commented Sep 9, 2024

@jrobinso I am done with the initial definition, requesting a review on this, some notes:

  • My standard is: (1) everything defined in the official docs should check. (2) Existing files in examples/ should type check in default (non-strict) mode with minimal changes and no changes to the transpiled JS code. The non-strict mode is specifically designed in TS for adapting existing JS code, it allows some common JS tricks but do not hamper checking the type conformity of regular JS objects. (3) Autocomplete should work when reasonable (main exception is excluding suggestions based on URL).
  • Track-specific APIs that are not well-defined in the documentation is not written in, users are free to do define what they need on their own and downcast as neeeded, it is a common practice in TS.
  • I gave up on JSDoc, it is too much work for me and since we don't have JSdoc in the main codebase I don't think I am the right person to decide to start it.
  • Not sure why CI is failing, doesn't look like it's on my end.

See the last comment for a demo.

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

Successfully merging this pull request may close these issues.

3 participants