Add tree-related types #1864

victorlin · 2024-10-08T18:54:29Z

Description of proposed changes

Converted most but not all tree-related files in the commit with the same title as this PR. There were many type errors exposed by conversion. They have been addressed in the following commits.

I removed a few comments that are now self-explanatory with the added types. I also moved all JSDoc types and descriptions into the function signature.

Note that there are many optional properties in the newly defined interfaces. This is not ideal and doesn't capture the fact that many properties are conditional on things such as layout. But it's a starting point to be improved over time.

Notable threads

Check bundle size (ref)
Check type of ReduxNode.currentGt (ref)
Figure out what to do with strictNullChecks violations (ref)
Figure out how to sort the new types (ref)
Which files to put types in? (ref1, ref2)
De-duplicate RootState and TreeComponentStateProps? (ref)

Checklist

Checks pass
~~If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR~~
~~(to be done by a Nextstrain team member) Create preview PRs on downstream repositories.~~

For later

jameshadfield · 2024-10-09T00:47:02Z

@genehack could you take a look at the types added / used here if you have a chance? 🙏

src/util/globals.js

victorlin · 2024-10-25T23:52:54Z

Progress update: I think I've renamed all the files that need to be renamed for sufficient types. I've temporarily turned off strictNullChecks to focus on the other typing issues and have addressed those. There are still several FIXMEs and any types that I'll come back to next week.

tsconfig.json

jameshadfield

(Incomplete review - just posting now as I won't return to this until next week)

Looks good @victorlin - I'm only part way through the really big commit so far but will pick this up later. No action necessary at the moment I don't think.

I think I can review the correctness, but I haven't written enough TS to give advice about the big scale approach.

src/components/tree/phyloTree/layouts.js

src/actions/tree.ts

src/components/tree/phyloTree/types.ts

genehack

picked some nits.

general feedback:

if you're going to have a types.ts file, centralize all the types and interfaces on that level into that file (e.g., phylotree)
there are many methods where the arguments got typed, but the return value of the method did not -- i would urge you to try to always provide a type for the return value too

src/actions/tree.ts

src/components/tree/index.ts

src/components/tree/phyloTree/change.ts

src/components/tree/phyloTree/helpers.ts

src/components/tree/phyloTree/phyloTree.ts

src/components/tree/phyloTree/types.ts

src/util/colorScale.ts

src/util/treeCountingHelpers.ts

jameshadfield

This is a big undertaking - thanks for persevering Victor. It's already apparent how many bugs this has identified and will really help going forward. A few of my comments are blocking, but most are questions only.

src/components/tree/phyloTree/types.ts

src/components/tree/tree.tsx

src/components/tree/phyloTree/types.ts

src/components/tree/tree.tsx

src/util/treeJsonProcessing.ts

src/components/tree/phyloTree/types.ts

src/util/colorScale.ts

The conversion done in the previous commit came with hundreds of violations of this rule. While some could be addressed by better types, I think the types as-is are acceptable and we can live without these checks. 🚧 this is being re-evaluated: #1864 (comment)

The conversion done in the previous commits came with hundreds of violations of this rule. While this could be addressed by better types, it will take some work and can be deferred. A new issue/PR will be created after this is merged. <#1864 (comment)>

src/components/tree/tree.tsx

The conversion done in the previous commits came with hundreds of violations of this rule. While this could be addressed by better types, it will take some work and can be deferred. A new issue/PR will be created after this is merged. <#1864 (comment)>

To satisfy the TypeScript rule noImplicitOverride.

To satisfy the ESLint rule @typescript-eslint/no-unused-vars.

Define the list of layouts in a single place.

Previous usage of parseInt was to convert float to integer. However, this isn't the intended usage of parseInt, which is made apparent with type checking - the parameter is expected to be a string type. Replace it with Math.trunc which is intended for converting float to integer.

Thse are typed to return a value that is not undefined.

This is easier for the TypeScript compiler to understand.

D3 expects the empty value to be null, not undefined.

Previously, there was no guarantee that an unknown error would produce a helpful message.

svgPropsToUpdate is Set<string>, not Set<string[]>.

Only KeyboardEvent has the shiftKey property.

calcTipRadii takes a key name of tipSelectedIdx. Previously, the value of idx was unused.

This should be a boolean, not whatever value newDistance is when defined.

This should be a boolean, not whatever value ReduxNode.parentInfo.original is when defined.

Not [string], [], [[string], [string]], or [string, [string]].

The extra value `true` had no effect.

These were initialized with default values but unused in the code.

Follow-up to "Lift node-selected modal to redux state" (f7e944d). This is left over from before we shifted the selected node to redux state.

This value was causing a type error because {} is not a Map. Remove it entirely since it isn't actually used - the value is created from scratch in src/middleware/performanceFlags.js.

There is a type error that prevents this from working properly: includes() takes a single string, but an array of PhyloNodes is given. It's not used on any existing calls to updateTipRadii, so just remove it entirely.

This previously worked because d3 was able to properly parse a color string out of RGBColor. However, it's better to keep things consistent and simple. Convert so that range is simply an array of hex color strings.

Centralize the defaults for consistency. Also, this avoids the need to mark the property as optional on the state.

This is more consistent with other booleans.

React will shallow merge the provided object into the existing state¹ which works for our usage. The type definition of setState does not allow partial state by default. Use 'never' to configure it to expect no fields in particular². ¹ <https://react.dev/reference/react/Component#setstate-parameters> ² <https://stackoverflow.com/a/55824499>

victorlin · 2024-11-12T21:55:07Z

I'm going to merge this. The types are largely in-progress with further follow-up to be done with #1888 and #1889, but we're at a good point now where this can be merged to allow new work to happen on top of these changes.

victorlin self-assigned this Oct 8, 2024

nextstrain-bot temporarily deployed to auspice-victorlin-tree--ntadob October 8, 2024 18:54 Inactive

victorlin force-pushed the victorlin/tree-typing branch from 34d10ba to 38b94d4 Compare October 8, 2024 20:49

victorlin temporarily deployed to auspice-victorlin-tree--ntadob October 8, 2024 20:49 Inactive

victorlin force-pushed the victorlin/tree-typing branch from 38b94d4 to 4b25ff9 Compare October 8, 2024 22:47

victorlin temporarily deployed to auspice-victorlin-tree--ntadob October 8, 2024 22:47 Inactive

victorlin temporarily deployed to auspice-victorlin-tree--ntadob October 9, 2024 17:56 Inactive

victorlin mentioned this pull request Oct 10, 2024

tree: Add toggle to focus on selected #1373

Merged

16 tasks

victorlin mentioned this pull request Oct 21, 2024

Code refactoring #1874

Open

4 tasks

victorlin force-pushed the victorlin/tree-typing branch 2 times, most recently from e27d5d2 to 864ed12 Compare October 25, 2024 02:10

jameshadfield reviewed Oct 25, 2024

View reviewed changes

src/util/globals.js Outdated Show resolved Hide resolved

victorlin mentioned this pull request Oct 25, 2024

Migrate to modern Redux #1711

Open

8 tasks

victorlin force-pushed the victorlin/tree-typing branch from 864ed12 to 0962aa8 Compare October 25, 2024 23:46

victorlin force-pushed the victorlin/tree-typing branch 2 times, most recently from 0523c8f to 701d856 Compare October 30, 2024 00:51

victorlin marked this pull request as ready for review October 30, 2024 00:52

victorlin requested a review from jameshadfield October 30, 2024 00:52

jameshadfield reviewed Oct 30, 2024

View reviewed changes

tsconfig.json Show resolved Hide resolved

jameshadfield reviewed Oct 31, 2024

View reviewed changes

genehack reviewed Nov 1, 2024

View reviewed changes

jameshadfield requested changes Nov 4, 2024

View reviewed changes

victorlin force-pushed the victorlin/tree-typing branch from 51cb597 to 8bb453a Compare November 4, 2024 19:15

victorlin force-pushed the victorlin/tree-typing branch from 4e51551 to 12aff24 Compare November 5, 2024 19:52

victorlin force-pushed the victorlin/tree-typing branch from 12aff24 to 9549dfb Compare November 5, 2024 23:59

victorlin commented Nov 6, 2024

View reviewed changes

src/components/tree/tree.tsx Outdated Show resolved Hide resolved

victorlin added 24 commits November 12, 2024 13:50

Add override keyword for React lifecycle methods

2e96210

To satisfy the TypeScript rule noImplicitOverride.

Add _ prefix for unused variables

7d22da0

To satisfy the ESLint rule @typescript-eslint/no-unused-vars.

Use exported Layout type

75fd6ad

Define the list of layouts in a single place.

Use Math.min/max instead of min/max

a79226e

Thse are typed to return a value that is not undefined.

Use separate variables for array and set of legend values

5f4fe7e

This is easier for the TypeScript compiler to understand.

Use null type for domRefs

aa30581

D3 expects the empty value to be null, not undefined.

Properly handle unknown errors when creating color scales

6083f73

Previously, there was no guarantee that an unknown error would produce a helpful message.

Properly check for stroke-width

83326a2

svgPropsToUpdate is Set<string>, not Set<string[]>.

Properly check window.event.shiftKey

25294e8

Only KeyboardEvent has the shiftKey property.

Properly set tipSelectedIdx

aedce7e

calcTipRadii takes a key name of tipSelectedIdx. Previously, the value of idx was unused.

Properly set timeSliceHasPotentiallyChanged

02b7206

This should be a boolean, not whatever value newDistance is when defined.

Properly set isSubtreeRoot

5b81e88

This should be a boolean, not whatever value ReduxNode.parentInfo.original is when defined.

Properly set PhyloNode.branch to [string, string]

79608a5

Not [string], [], [[string], [string]], or [string, [string]].

Remove unused parameter to numDate

9d57be8

The extra value `true` had no effect.

Remove unused mapLegendOpen, treeLegendOpen from controls state

5803970

These were initialized with default values but unused in the code.

Remove unused selectedNode from state

4d2e633

Follow-up to "Lift node-selected modal to redux state" (f7e944d). This is left over from before we shifted the selected node to redux state.

Remove unused default value for performanceFlags

8c6a301

This value was causing a type error because {} is not a Map. Remove it entirely since it isn't actually used - the value is created from scratch in src/middleware/performanceFlags.js.

Remove broken/unused searchNodes parameter

3bd4b55

There is a type error that prevents this from working properly: includes() takes a single string, but an array of PhyloNodes is given. It's not used on any existing calls to updateTipRadii, so just remove it entirely.

Convert RGBColor to hex string

56b791b

This previously worked because d3 was able to properly parse a color string out of RGBColor. However, it's better to keep things consistent and simple. Convert so that range is simply an array of hex color strings.

Set default colorings in getDefaultControlsState

5a681ff

Centralize the defaults for consistency. Also, this avoids the need to mark the property as optional on the state.

Set default value of focus to false

931f1e6

This is more consistent with other booleans.

victorlin force-pushed the victorlin/tree-typing branch from 3e23f85 to 9c7644b Compare November 12, 2024 21:50

victorlin merged commit 1cf70f8 into master Nov 12, 2024
25 checks passed

victorlin deleted the victorlin/tree-typing branch November 12, 2024 21:57

joverlee521 mentioned this pull request Nov 14, 2024

Color tree by measurements #1819

Open

victorlin mentioned this pull request Nov 15, 2024

[bugfix] Restore shift-click branch behaviour #1901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tree-related types #1864

Add tree-related types #1864

victorlin commented Oct 8, 2024 •

edited

Loading

jameshadfield commented Oct 9, 2024

victorlin commented Oct 25, 2024

jameshadfield left a comment

genehack left a comment

jameshadfield left a comment

victorlin commented Nov 12, 2024

Add tree-related types #1864

Add tree-related types #1864

Conversation

victorlin commented Oct 8, 2024 • edited Loading

Description of proposed changes

Notable threads

Checklist

For later

jameshadfield commented Oct 9, 2024

victorlin commented Oct 25, 2024

jameshadfield left a comment

Choose a reason for hiding this comment

genehack left a comment

Choose a reason for hiding this comment

jameshadfield left a comment

Choose a reason for hiding this comment

victorlin commented Nov 12, 2024

victorlin commented Oct 8, 2024 •

edited

Loading