[Draft] plan of action for completing JS->TS

[cacieprins]

Success Criteria

• No unnecessary .js in the repository
• The repository passes linting based on the @typescript-eslint/recommended-type-checked rule set
• The repository uses prettier to autoformat, rather than the deprecated eslint formatting rules
• Optional / Stretch goal: Define a style guide for the repository that may extend past basic linting and formatting constraints. Initial thoughts:
  • There are no circular dependencies
  • There are no deep imports from undeclared exports
  • There are no implied dependencies on monorepo packages (e.g., importing @packages/foo from a file whose package.json does not declare @packages/foo as a dependency)
  • There are no implied dependencies on non-repo packages (e.g., relying on hoisting to provide a reference for an import, because the package being imported was declared a dependency in either the root package.json or a sibling's package.json

Preparation

• add .prettierignore files to each package with a * directive
• derive a non-strict rule set from the @typescript-eslint/recommended-type-checked rule set and export as a config from eslint-plugin-dev. Rules that error in the original rule set but which prove difficult to resolve in this repository are set to warn instead.
  • By consolidating typescript specific eslint rules to either "strict" (which errors) or "non-strict," (which warns) work on individual files can be further broken down into changes that resolve a specific warning, rather than a collection of linting errors.
  • Once a package emits no warnings, it can be safely set to be linted with the "strict" rule set, which will prevent changes that introduce new linting violations to that package.

Execution

For shared language purposes, both packages and files can be categorized as "leaf," "shared," or "root" nodes.

• A Leaf node has no monorepo dependencies
• A Shared node depends on one or more nodes, and is the dependent of one or more node.
• A Root node is not imported by any nodes other than ./cli

Formatting

Beginning from leaf packages, recursively down to root packages: add .prettierrc files that enforces the formatting from the style guide, and open a PR that updates any nonconforming styling in that package.

Ideally a package can be converted to prettier as a prerequisite for feature work to be done in that package.

Typescript

• define an .eslintrc for each package, that uses the non-strict rule set. Some packages may include additional rulesets or plugins for, e.g., graphql. Packages must not disable any rules from @typescript-eslint unless those rules are replaced with more analogous rules from a different plugin that are more strict. If a rule needs to be changed to warn, it must be set in the non-strict rule manifest, rather than at a package level. The following rules are likely to need to be set to warn rather than error in the non-strict rule set:
  • Basic typescript: ban-ts-comment, no-require-imports
  • Type declarations: no-explicit-any, no-unused-vars
  • Async: await-thenable, no-floating-promises, prefer-promise-reject-errors, require-await
• Resolve warnings in each package recursively, from leaf to root. Preliminary prioritization of rule warnings, from highest to lowest, non-exclusive:
  1. no-require-imports - this is a prerequisite to moving to ESM, unless we create our own require with Module.createRequire.
  2. no-floating-promises, because floating promises can introduce difficult-to-track-down errors in async codepaths
  3. no-explicit-any - any types are a useful escape hatch when first converting to TS, but prevent static type analysis. If the type of the identifier is indeterminate at declaration time, it is more accurate to type that identifier as unknown, which forces you to determine what type it actually is via type narrowing before accessing that identifier. Resolving these can run the gamut from very easy to extraordinarily difficult, so prioritize based on the pain level of the specific any triggering this warning.
  4. typedef helps with implicit any types, which have all of the pitfalls of explicit any types. Once these warnings are resolved, we can enable Typescript's noImplicitAny compiler option.
  5. ban-ts-comment - @ts-ignore comments are usually used to bypass typescript when an alternative approach might be possible. Usually if it's difficult to resolve one of these, either a dependency must be updated or a structural refactor should be considered.
  6. no-unused-vars - this is quality of life, and performance, rather than correctness. This is also the lowest hanging fruit.
  7. require-await requires that async functions actually await a promise. This avoids unnecessary promises, as the return value of an async function is always a promise. This is performance, rather than correctness.
  8. await-thenable, because awaiting a non-promise still creates a new microtask. This is a performance rule, not a correctness rule.

Constraints(TBD if they will be applied) 10. prefer-promise-reject-errors may end up being very difficult to resolve, if there are any instances of this warning

• If feature work must be done in a given file, and that file has eslint warnings, those warnings must be resolved as a prerequisite for that work.
• If feature work must be done in a given file, and that file has deep imports from an undeclared export, an implied dependency, or circular dependencies, those issues must be resolved as a prerequisite for that work. TBD: can we lint on these?

Helpful Tips

• If you're working on converting a large file with many exports, try extracting each export into its own file. This can help reduce the scope of necessary changes.
• Circular dependencies can cause havoc - use madge to detect if the file you're working on is included in a circular dependency cycle, and resolve that before converting the file.