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

Fix TypeScript errors and enable tsc in CI #2015

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Fix TypeScript errors and enable tsc in CI #2015

merged 6 commits into from
Apr 18, 2024

Conversation

digitalcora
Copy link
Contributor

@digitalcora digitalcora commented Apr 16, 2024

This adds type-checking to CI and resolves all existing type errors that are not due to implicit any types — in other words, all cases where the code or the type annotations are wrong in a way the compiler can prove without needing full type annotations. (This is more limited than it sounds; to catch all errors, explicit annotations are needed on most function arguments and return values, which is why noImplicitAny exists and is enabled by default in strict mode. For now, I've disabled it here, since we have such a huge number of implicit anys that it would explode the scope of this task.)

The "magic comment" @ts-expect-error suppresses any error that would be reported on the next line, and is used here to punt on situations that would be non-trivial to resolve, particularly in non-v2 code where we may never have to resolve them. Note this differs from @ts-ignore in that if there is no error on the next line, this is reported as an error — so if a future improvement to our types or the compiler results in the error being resolved, we'll know and can remove the comment.

There are a few general issues which are all fixed in similar ways:

  • When the compiler can't prove the conditions of an if or switch cover all possibilities, and there is no fallback else or default, we have to add one. In most cases we can just throw an error, as we expect the code path to never be reached.

  • Several places in existing type definitions were trying to use object as though it meant "object with unspecified fields of unspecified types". However, object actually means an "opaque" object: it only permits operations that would be allowed on any object, which does not include accessing any specific properties (which might not exist). The correct way to do this is Record<string, any>, though in some cases involving React props the best we can do without filling in a more detailed type is any.

  • A type annotation is required on variables initialized to null or undefined and later assigned, or arrays initially declared as [] and later populated. The compiler does not backtrack to fill in the variable's type based on later usage; it only looks at the initial value.

  • Similar to the above, useState requires a type parameter for the values that will later be assigned, if the type cannot be inferred from the initial value.

  • Also similar to the above, useRef and forwardRef require a type parameter for the DOM element the ref will later be associated with. Due to a quirk of how the types are implemented for this function, null must also be explicitly provided as the initial value (we were already doing this in most places).

  • The value in a ref is null during the initial render, so code that accesses properties of ref.current needs to account for this, even if we expect it to never happen (e.g. inside a useLayoutEffect).


@mbta mbta deleted a comment from github-actions bot Apr 16, 2024
@digitalcora digitalcora marked this pull request as ready for review April 18, 2024 19:10
Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

This is great! Thank you so much for this! Just one suggestion, otherwise good to go.

Also, Screenplay uses types pretty well at the moment, but this might be a good thing to add there as well. I can write up a task for that tomorrow.

@@ -59,7 +59,7 @@ const EditableNumberInput = ({
if (!isNaN(value)) {
doUpdate(index, mutator || id, value);
} else {
alert(`Integer value expected in ${id} for Screen ID ${rowValues.id}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Well that's embarrassing. So this alert never worked because the variable didn't exist...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to have been a copy-paste error? There is another alert like this in the same file where the variable does exist.

assets/tsconfig.json Show resolved Hide resolved
This resolves all remaining type errors that are not due to implicit
`any` types — in other words, all cases where the code or the type
annotations are wrong in a way the compiler can prove without needing
full type annotations. (This is more limited than it sounds; to catch
all errors, explicit annotations are needed on most function arguments
and return values, which is why `noImplicitAny` exists and is enabled
by default in strict mode.)

The "magic comment" `@ts-expect-error` suppresses any error that would
be reported on the next line, and is used here to punt on situations
that would be non-trivial to resolve, particularly in non-v2 code where
we may never have to resolve them. Note this differs from `@ts-ignore`
in that if there is _no_ error on the next line, this is reported as an
error — so if a future improvement to our types or the compiler results
in the error being resolved, we'll know and can remove the comment.

There are a few general issues which are all fixed in similar ways:

* When the compiler can't prove the conditions of an `if` or `switch`
  cover all possibilities, and there is no fallback `else` or `default`,
  we have to add one. In most cases we can just throw an error, as we
  expect the code path to never be reached.

* Several places in existing type definitions were trying to use
  `object` as though it meant "object with unspecified fields of
  unspecified types". However, `object` actually means an "opaque"
  object: it only permits operations that would be allowed on _any_
  object, which does not include accessing any specific properties
  (which might not exist). The correct way to do this is
  `Record<string, any>`, though in some cases involving React props the
  best we can do without filling in a more detailed type is `any`.

* A type annotation is required on variables initialized to `null` or
  `undefined` and later assigned, or arrays initially declared as `[]`
  and later populated. The compiler does not backtrack to fill in the
  variable's type based on later usage; it only looks at the initial
  value.

* Similar to the above, `useState` requires a type parameter for the
  values that will later be assigned, if the type cannot be inferred
  from the initial value.

* Also similar to the above, `useRef` and `forwardRef` require a type
  parameter for the DOM element the ref will later be associated with.
  Due to a quirk of how the types are implemented for this function,
  `null` must also be explicitly provided as the initial value (we were
  already doing this in most places).

* The value in a ref is `null` during the initial render, so code that
  accesses properties of `ref.current` needs to account for this, even
  if we expect it to never happen (e.g. inside a `useLayoutEffect`).
This uses `concurrently` for simple parallel execution of the Webpack
watcher and the TypeScript watcher in one process/terminal. The Phoenix
watcher configuration is also updated to use `npm run watch` instead of
duplicating the Webpack options from `package.json`.
In addition to a job that runs `tsc` now that we've resolved (or swept
under the rug) our type errors, this also adds a job for running the
frontend Webpack build, so we aren't surprised by a build error when we
go to deploy a change.
This particular "prefix" is a file, not a directory, and this matters
for `tsc` (but not Webpack, which configures these in a different way).
Copy link

Coverage of commit 4d1974c

Summary coverage rate:
  lines......: 44.4% (2839 of 6394 lines)
  functions..: 41.9% (1017 of 2427 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@digitalcora digitalcora merged commit d36529b into main Apr 18, 2024
7 checks passed
@digitalcora digitalcora deleted the cfg-type-check branch April 18, 2024 21:07
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.

2 participants