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

TypeScript: Address strictNullChecks violations #1888

Open
victorlin opened this issue Nov 7, 2024 · 3 comments
Open

TypeScript: Address strictNullChecks violations #1888

victorlin opened this issue Nov 7, 2024 · 3 comments

Comments

@victorlin
Copy link
Member

victorlin commented Nov 7, 2024

from #1864 (comment)

The TSConig option strictNullChecks will be disabled with #1864. We should re-enable it to catch existing bugs and prevent new bugs.

Possible solutions

  1. Ignore all current and future violations (status quo)
  2. ⛔️ Ignore current violations only using a baseline approach
  3. Use equality narrowing
    • I started this in 260ba00 but it's tedious with the hundreds of violations.
  4. TypeScript: Reduce the amount of optional properties #1889
@jameshadfield
Copy link
Member

strictNullChecks as in the handbook examples will be really helpful to catch little bugs, but 260ba00 shows how intertwined it is with #1889. Using an abridged example from that commit:

const unrootedPlaceSubtree = (node: PhyloNode, totalLeafWeight: number) => {
  // FIXME: consider an UnrootedPhyloNode that guarantees presence of these? will need some casting...
  if (node.depth === undefined || ... ) {
    return;
  }

This looks safe (the function handles the optional properties) but if it's ever triggered it indicates a much larger bug in the architecture of Auspice, namely that this function should never be called if these values haven't been set. Returning or throwing essentially masks this deeper more structural issue. Ideally the type system would provide guarantees about the overall architecture and ensure that a function like unrootedPlaceSubtree can never be given an argument which doesn't have the necessary values set.

@genehack
Copy link
Contributor

genehack commented Nov 7, 2024

This looks safe (the function handles the optional properties) but if it's ever triggered it indicates a much larger bug in the architecture of Auspice, namely that this function should never be called if these values haven't been set.

As we discussed yesterday, I think the best approach here is to have 2 types per "thing" — one where the properties are largely optional and a second where they're not (insert naming bikeshed convo here...) — and then use type guards to "up-convert" the optional form into the fully reified one.

@victorlin
Copy link
Member Author

Sorry to shift the conversation. I want to split up this issue vs. #1889 because, while I think a bulk of the strictNullChecks violations are due to optional properties, it's not the only factor. There are others such as accessing tree.nodes[0] that are considered violations as well. I think this is related to nextstrain/nextstrain.org#892.

@genehack I'll respond to your comment in #1889.

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