Start using TypeScript

[jameshadfield]

Description of proposed changes

Adds basic TypeScript support and conversion of a few files into TS/TSX.

See commit messages.

[ivan-aksamentov]

I thought you'd just delete the types when porting the badges, but you decided to add Typescript to auspice instead :)

Looks good! Mild concern only that in package.json typings do not correspond to the versions of js packages (e.g. types for react and styled-components are a major version ahead), which may make type checking and text editor assist less useful and even harmful. Otherwise I am super jazzed about auspice getting some types!

[jameshadfield]

I was surprised TS didn't allow ...geoFilter given that the preceding if statement ensures there are 2 elements. Perhaps three's some gotchas in the spread operator that I don't understand.

[ivan-aksamentov]

Compiled languages rarely check array lengths[1], because it often requires runtime information, while compilers only have information available at compile time. In this particular case, it might be possible to prove that array size is 2, but in many cases it's not.

I'd say, this code is questionable. If geoFilter always has two items, then why it's not an object with named fields. They are even given names inside determineLocationMatch().

It looks like an abuse of container type. I guess it can also be empty (parameter is defaulted to [])? Can it be anything else? 1 element? How about 3? Long story short, it's a bit of a sloppy, indecisive code, because it does not know what it wants.

Typescript does not make it easy to write code where things constantly transmute - an elephant becomes a banana and then in another file it becomes a space ship. Typescript requires some discipline. You need to decide things in advance and spell them. It puts constraints onto the Wild West of JS, and tries to make code less dynamic and easier to reason about.

Ideally I'd write something like:

// TODO: prehaps rename fields to have less of repeated "geo" prefix
export interface GeoFilters {
  geoResolution: string;
  geoValueToMatch: string;
}

function calcTipRadii(
  // ... more params
  geoFilters?: GeoFilters // can be `undefined`
  // ... more params
) {
  // ...
  if (geoFilters) {
    return tree.nodes.map((node) => determineLocationMatch(node, geoFilters))
  }
  // ...
}

// This function is very short and can probably be inlined, unless reused elsewhere
function determineLocationMatch(
  node: Node,
  { geoResolution, geoValueToMatch }: GeoFilters // destructuring
) {
  return geoValueToMatch === getTraitFromNode(node, geoResolution);
}

If you really want to abuse the container type (e.g. because it comes from a stable file format, and you don't want to write additional conversions), then in Typescript there's a tuple type, which can emulate fixed-size array. You could make geoTypes of type [string, string], which is different from string[]. But in this particular code geoTypes defaults to [], and [] cannot be of type [string, string]. So it needs additional re-thinking.

--

[1] One exception is Rust, where you have separate Vec type for dynamic arrays and array type [i32;N] for fixed-size arrays. The N in the array type is then a part of the type, and is checked on compile-time, while Vec is only checked at runtime.

[jameshadfield]

Thanks @ivan-aksamentov - since TS allows conditional statements to narrow the types I expected this to be ok; perhaps it doesn't like it because we may have overriden the length prototype?

And fully agree with your general comments about the code -- this is one of the reasons I want to mostly write things in TS going forward (as well as the implicit documentation it'll provide when reading/writing code). However I didn't want to labour this PR with code changes, I just tried to find the most self-contained file to port to TS I could.

[jameshadfield]

Interestingly, changing the type from geoFilter:([string, string]|[]) to geoFilter?:[string, string] (and therefore using undefined for the default argument) does allow the spread operator to be used. I'm not going to make this change in this PR as there's other files which call this function using an empty array argument, but it was an interesting insight into how TS works.

 export const calcTipRadii = (
-  {tipSelectedIdx = false, selectedLegendItem = false, geoFilter = [], searchNodes = false, colorScale, tree}:
-  {tipSelectedIdx:(number|false), selectedLegendItem:(number|string|false), geoFilter:([string, string]|[]), searchNodes:any, colorScale:any, tree:any}
+  {tipSelectedIdx = false, selectedLegendItem = false, geoFilter = undefined, searchNodes = false, colorScale, tree}:
+  {tipSelectedIdx:(number|false), selectedLegendItem:(number|string|false), geoFilter?:[string, string], searchNodes:any, colorScale:any, tree:any}
 ) => {
   if (selectedLegendItem !== false && tree && tree.nodes) {
     return tree.nodes.map((d:any) => determineLegendMatch(selectedLegendItem, d, colorScale) ? tipRadiusOnLegendMatch : tipRadius);
-  } else if (geoFilter.length===2 && tree && tree.nodes) {
-    return tree.nodes.map((d:any) => determineLocationMatch(d, geoFilter[0], geoFilter[1]) ? tipRadiusOnLegendMatch : tipRadius);
+  } else if (geoFilter?.length===2 && tree && tree.nodes) {
+    return tree.nodes.map((d:any) => determineLocationMatch(d, ...geoFilter) ? tipRadi
usOnLegendMatch : tipRadius);
   } else if (searchNodes) {
     return tree.nodes.map((d:any) => d.name.toLowerCase().includes(searchNodes) ? tipR
adiusOnLegendMatch : tipRadius);
   } else if (tipSelectedIdx) {

[tsibley]

I tried to reproduce what you described with a minimal toy example in a separate file (checking with npx tsc --strict --target esnext --skipLibCheck repro.ts) and couldn't even after several attempts. Then I tried to reproduce using ...geoFilter in place… and also couldn't. It seems fine for me?

[jameshadfield]

With the changes @victorlin has made, as of 18c809b (and tsc version 5.0.4) I can't reproduce my original error. Which is great -- I am glad typescript can infer this. I'm not inclined to revert back the change (i.e. go back to the spread operator); as Ivan alludes to, we'll hopefully improve the underlying structure over time.

[ivan-aksamentov]

I just realized I have a bunch of typings written for Aupice in Nextclade. These are just the things that are used in Nextcade, and written long time ago, so many things may be missing or inaccurate

https://github.com/nextstrain/nextclade/tree/master/packages_rs/nextclade-web/src/types/auspice

and the tree type is for some reason in a separate place:

[tsibley]

These weren't flagged as errors previously, however the previous commit swapped the rule "no-use-before-define" to "@typescript/no-use-before-define".
[…]
I don't know enough about specifications to know why this is considered an error but still works in practice, however it's not much effort to reorganise the code in these 2 files.

I find this sort of reorganization actively harmful to the ability to read the code in a file. Reading top to bottom, I want to read the higher level functions first, which call the lower level functions that I'll read next. The opposite is like putting the cart before the horse. decodePositions is out of context and makes no sense until I see it called in decodeColorByGenotype.

This is why in Auspice 6f22d9f disabled no-use-before-define for functions, based on my change to nextstrain.org's config.

[jameshadfield]

There's a few things at play here.

Firstly, I didn't notice the existing eslint overrule for that line [1]:

auspice/src/util/getGenotype.js

Line 45 in b58f148

const positions = decodePositions(encodedPositions, geneLength); // eslint-disable-line no-use-before-define

With this PR this overrule needs to change to // eslint-disable-line @typescript-eslint/no-use-before-define, which I'll do as part of this PR. (This keeps the code as intended, which is appropriate for this PR.)

Secondly, I can't remember the specifics of why the following change was made (by me, but over a year ago):

-  no-use-before-define: ["error", { "functions": false }]
+  no-use-before-define: "off"  # note you must disable the base rule as it can report incorrect errors
+  "@typescript-eslint/no-use-before-define": ["error", { "functions": false }]

The intention is to continue to allow function (not fat-arrow) hoisting as per 6f22d9f (rule docs here). Linting behavior under no-use-before-define": ["error", { "functions": false }]:

# Not valid JS, and has linting errors
const foo = () => bar();
foo();
const bar = () => {console.log("42");};

# Valid JS, and no linting errors
const foo = () => bar();
foo();
function bar() {console.log("42");}

# Valid JS, but linting fails because of the introduction
# of a temporal dead zone. Allowable by using
# ["error", { "functions": false, "variables": false}]
const foo = () => bar();
const bar = () => {console.log("42");};
foo();

The third example is why we get the errors in choose-layout.js [2], however why they were flagged up now and not before I don't understand 🤷 . Note that while setting "variables": false allows the existing code in choose-layout.js it also allows the first toy example above to pass linting, despite it throwing a ReferenceError when run.

[1] You wrote that override a ~year before we allowed function hoisting in 6f22d9f, but it's not a function declaration so it wouldn't be affected by "functions": false anyways.

[2] And also in getGeontype.js if we didn't have the eslint overrule for that line

[tsibley]

Ah yeah, I missed some subtly. Nice breakdown of it. Thanks!

With this PR this overrule needs to change to // eslint-disable-line @typescript-eslint/no-use-before-define, which I'll do as part of this PR. (This keeps the code as intended, which is appropriate for this PR.)

Sounds good.

however why they were flagged up now and not before I don't understand 🤷. Note that while setting "variables": false allows the existing code in choose-layout.js it also allows the first toy example above to pass linting, despite it throwing a ReferenceError when run.

Agreed we don't want to miss dangerous dead zones by setting variables: false, but it'd be nice to ignore harmless dead zones.

I briefly tried to identify why the ESLint parser change and/or no-use-before-define rule change caused this, but couldn't find anything conclusive. My guess would be that the ESLint AST emitted by the parser (for the same code) is different, and why the rule triggers now but not before would be more apparent if we could compare the ASTs.

[tsibley]

Compared ASTs for this toy example:

class Foo extends React.Component {
  renderX() {
    return (<Bar />);
  }
}

const Bar = ({ }) => (<>bar</>);

using https://astexplorer.net with @babel/eslint-parser and @typescript-eslint/parser. Sorting the JSON output and diffing it produces nothing obvious. It's very similar. So maybe Typescript's no-use-before-define rule supports JSXIdentifier nodes and the standard rule just… doesn't?

OTOH, signs still point to a parsing difference when I interrogate the example above with npx eslint --ext .js,.ts,.jsx,.tsx --parser @babel/eslint-parser vs. … --parser @typescript-eslint/parser and also try switching between the two rules.

parser / rule	no-use-before-define	@typescript-eslint/no-use-before-define
@babel/eslint-parser	ok	ok
@typescript-eslint/parser	ok	errors

[jameshadfield]

Nice work going down that rabbit hole!

As part of PR #1665, 9fc42f0 turned decodePositions into a function declaration which is hoisted:

- export const decodePositions = (positions, geneLength = 'Infinity') => {
+ export function decodePositions(positions, geneLength = 'Infinity') {

This is left unchanged by this PR.

The changes to choose-layout.js still remain in this PR -- af4c742 -- due to the change in interpretation between @babel/eslint-parser and @typescript-eslint/parser (as per Tom's message above).

I think this thread can be considered resolved.

[jameshadfield]

Aside from the threaded discussions happening above, how do we feel about merging this PR? Specifically @ivan-aksamentov, @tsibley, @joverlee521 and @victorlin - but I welcome anyone else's thoughts as well! I either want to discard this PR or merge it (with appropriate fixes), not leave it hanging for another year!

[tsibley]

Happy to see this merged. Good to get Typescript support going!

(Would be good to resolve the outstanding threads and minor fixes before merging.)

[tsibley]

Ah yeah, I missed some subtly. Nice breakdown of it. Thanks!

With this PR this overrule needs to change to // eslint-disable-line @typescript-eslint/no-use-before-define, which I'll do as part of this PR. (This keeps the code as intended, which is appropriate for this PR.)

Sounds good.

however why they were flagged up now and not before I don't understand 🤷. Note that while setting "variables": false allows the existing code in choose-layout.js it also allows the first toy example above to pass linting, despite it throwing a ReferenceError when run.

Agreed we don't want to miss dangerous dead zones by setting variables: false, but it'd be nice to ignore harmless dead zones.

I briefly tried to identify why the ESLint parser change and/or no-use-before-define rule change caused this, but couldn't find anything conclusive. My guess would be that the ESLint AST emitted by the parser (for the same code) is different, and why the rule triggers now but not before would be more apparent if we could compare the ASTs.

[tsibley]

Compared ASTs for this toy example:

class Foo extends React.Component {
  renderX() {
    return (<Bar />);
  }
}

const Bar = ({ }) => (<>bar</>);

using https://astexplorer.net with @babel/eslint-parser and @typescript-eslint/parser. Sorting the JSON output and diffing it produces nothing obvious. It's very similar. So maybe Typescript's no-use-before-define rule supports JSXIdentifier nodes and the standard rule just… doesn't?

OTOH, signs still point to a parsing difference when I interrogate the example above with npx eslint --ext .js,.ts,.jsx,.tsx --parser @babel/eslint-parser vs. … --parser @typescript-eslint/parser and also try switching between the two rules.

parser / rule	no-use-before-define	@typescript-eslint/no-use-before-define
@babel/eslint-parser	ok	ok
@typescript-eslint/parser	ok	errors

[ivan-aksamentov]

Regarding no-use-before-define, I think typescript-eslint is not configured correctly, the "extends" clause is missing:
https://typescript-eslint.io/getting-started
My current understanding is as follows: some of the eslint rules are also duplicated in typescript-eslint (under @typescript-eslint/ prefix) but adapted for typescript. Some eslint rules make little sense for typescript. In this case the original eslint rule needs to be disabled. This is what the extends clause is doing - it "pastes" config for some of the rules - disables old ones and enables their replacements, plus new ones:
https://github.com/typescript-eslint/typescript-eslint/blob/304b551915a3f5e0d715a7e3ebd0d630f4afb456/packages/eslint-plugin/src/configs/all.ts#L125-L126

There are also additional rules that use type information:
https://typescript-eslint.io/linting/configs

Usually ts rules, including typescript-eslint rules, should work well for js. But if needed, eslint allows to configure lints for .js and .ts files separately, either using 2 separate configs, or by using "overrides" section.

[victorlin]

I've updated this PR with improvements to webpack/ESLint/TS configs and converted a couple more files over to TS. This PR should be re-reviewed.

new changes

[ivan-aksamentov]

@victorlin IMO typescript-eslint is still not configured correctly, as I mentioned above. The extends clause is missing:
https://typescript-eslint.io/getting-started
That's why you guys have all these weird pink elephants. Or at least some of them.

[victorlin]

@ivan-aksamentov I don't think extending plugin:@typescript-eslint/recommended is necessary. We are already extending airbnb which is another set of recommended config presets, and airbnb-typescript (added in 9b0c47f) has the same rule override as the configuration you linked.

Regardless, I tried: extended plugin:@typescript-eslint/recommended and reverted 9cab2aa, still got the same no-use-before-define errors. @tsibley's digging might be the closest explanation that we have right now.

[jameshadfield]

LGTM 🎉 🙌