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 types to tree component #1864

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Add types to tree component #1864

wants to merge 22 commits into from

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Oct 8, 2024

Description of proposed changes

Adding types to link things together.

For later: b37a77c, 260ba00

Checklist

@victorlin victorlin self-assigned this Oct 8, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-tree--ntadob October 8, 2024 18:54 Inactive
@victorlin victorlin temporarily deployed to auspice-victorlin-tree--ntadob October 8, 2024 20:49 Inactive
@victorlin victorlin temporarily deployed to auspice-victorlin-tree--ntadob October 8, 2024 22:47 Inactive
@jameshadfield
Copy link
Member

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

@victorlin victorlin temporarily deployed to auspice-victorlin-tree--ntadob October 9, 2024 17:56 Inactive
@victorlin victorlin mentioned this pull request Oct 21, 2024
4 tasks
@victorlin victorlin force-pushed the victorlin/tree-typing branch 2 times, most recently from e27d5d2 to 864ed12 Compare October 25, 2024 02:10
ES2015 allows iteration of sets which is used in this codebase.
export const strainSymbol = Symbol('strain');
export const genotypeSymbol = Symbol('genotype');
export const measurementIdSymbol = Symbol('measurementId');
export const strainSymbol = 'strain';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume this change is to make types easier?

I'd prefer to keep symbols - they are perfect for this use case. One downside to them is that they are skipped with things like Object.keys(), but I'd hope TS would be kind enough to flag this up for us?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah mainly for ease of typing. I can look into working with them as-is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed with with 3c3ef2d

@victorlin
Copy link
Member Author

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.

Use named imports from the package instead of direct imports from
source. This allows proper typing by the external types/d3 package.
Converted most but not all tree-related files. There are some type
errors exposed by conversion - those will be addressed by subsequent
commits.
The conversion done in the previous commit came with 443 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.
Passing a number to parseInt is a type error. The extra function call is
not necessary since the number is already base 10.
calcTipRadii takes a key name of tipSelectedIdx. Previously, the value
of idx was unused.
svgPropsToUpdate is Set<string>, not Set<string[]>.
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.
The extra value `true` had no effect.
Not [string], [], [[string], [string]], or [string, [string]].
Previously, there was no guarantee that an unknown error would produce a
helpful message.
Not sure how this worked previously. I briefly searched for a test
dataset that goes through any of these conditions but couldn't find one.
Only KeyboardEvent has the shiftKey property.
This is easier for the compiler to understand.
These were initialized with default values but unused in the code.
There is a default value set in modifyControlsStateViaTree, but this is
the only property that doesn't have an explicit default set by
getDefaultControlsState. Add it for consistency.
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 prevents type errors when calling setState.
@victorlin victorlin marked this pull request as ready for review October 30, 2024 00:52
@@ -31,6 +32,7 @@ Visit https://aka.ms/tsconfig.json for a detailed list of options.

/* Type Checking */
"strict": true, /* Enable all strict type-checking options. */
"strictNullChecks": false, /* Allow unhandled false/null/undefined values to make incremental TypeScript adoption easier. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eslint has the option of disabling a rule for a particular file - does tsc / typescript-eslint have the same? If so, that may be better as we can add a "todo" comment in those files and then incrementally move to adhering to this rule

Copy link
Contributor

@genehack genehack Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't read the whole Typescript issue thread, but the answer to this seems to be somewhere between "no" and "kinda via a PITA workaround". See this SO question which leads to a Typescript issue.

(The PITA workaround is "make the general config non-strict and then 'mask' that with strict configs that are limited to a subset of files" which is kinda 🤮, I think?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging @genehack. The closest thing I could find in TypeScript docs is ts-nocheck which means "disable all rules for this file" (not great).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another bad alternative is the per-line ts-ignore/ts-expect-error:

  1. This disables all rules for the line, which is similarly not great
  2. Doing this for 443 violations of strictNullChecks seems overkill

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update the commit message of 8a0f4fa to note that we've considered these alternatives.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the responses - pity about the answers tho!

I can update the commit message of 8a0f4fa to note that we've considered these alternatives.

Sounds good. Ideally we would turn this rule on in the future as I think it'll catch a whole lot of bugs. 443 is daunting tho!

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.

4 participants