|
| 1 | +# Writing and Migrating To TypeScript |
| 2 | + |
| 3 | +## Style Guidance |
| 4 | + |
| 5 | +In general, the [ts.dev](https://ts.dev/style/) style guide shall be adhered to as close as possible, with the caveat that end-of-line semicolons are omitted unless necessary to prevent syntax errors. |
| 6 | + |
| 7 | +#### Type Definitions |
| 8 | + |
| 9 | +Prefer `interface` to `type` when possible. Interfaces are never inlined, which solves some potential issues with generated `.d.ts` files. |
| 10 | + |
| 11 | +When to use `type`: |
| 12 | +* The type being declared must be a Union type |
| 13 | +* Declaration merging may happen when it should not |
| 14 | +* Complicated types using utility types |
| 15 | + |
| 16 | +#### Object Literals |
| 17 | +When working with a structural implementation, always declare the type. |
| 18 | + |
| 19 | +Correct: |
| 20 | +```typescript |
| 21 | +interface Foo { |
| 22 | + bar: string |
| 23 | +} |
| 24 | + |
| 25 | +const foo: Foo = { |
| 26 | + bar: 'baz', |
| 27 | +} |
| 28 | +``` |
| 29 | +Incorrect: |
| 30 | +```typescript |
| 31 | +const foo = { |
| 32 | + bar: 'baz', |
| 33 | +} |
| 34 | +``` |
| 35 | + |
| 36 | +#### Function Definitions |
| 37 | + |
| 38 | +Always declare types for parameters and return values. Declaring parameter types ensures that callers are providing the correct parameters. Declaring the return type ensures that the function body is returning what is intended. |
| 39 | + |
| 40 | +_Exception_: inlined single-line arrow functions can usually have their type declarations inferred. This is especially the case if the ultimate return value is declared. Example: |
| 41 | +```ts |
| 42 | +const odds: string[] = [1,2,3,4,5] |
| 43 | + .filter(n => n % 2) |
| 44 | + .map(n => String(n)) |
| 45 | +``` |
| 46 | + |
| 47 | +**Prefer** inline types. Parameter types and return types often do not need to be referenced outside of the function itself. Bag parameters (object literals with more than two properties) should be declared as a named interface. If possible, do not export this interface. If a dependent needs to reference the bag type because it is composing an object literal in preparation to call the function in question, that dependent should use the [Parameters utility type](https://www.typescriptlang.org/docs/handbook/utility-types.html#parameterstype) instead. This way the interface name can change without necessitating a change to the dependent. Use this technique, example below, as a last resort. Prefer composing the parameter bag inline, rather than assigning it to an identifier. |
| 48 | + |
| 49 | +Choosing definition styles is almost more of an art than a science. Conciseness and flexibility often go hand in hand, but do not compromise on enforcing correctness to achieve conciseness. Fancy type manipulation with mapped types derived from templated types can be satisfying to figure out, but they often indicate leaky abstractions, vague overloads, and so forth. Use with caution. |
| 50 | + |
| 51 | +Incorrect: |
| 52 | +```ts |
| 53 | +// foo.ts |
| 54 | +export const foo({ a, b, n}: { a: string, b: boolean, n: number}): void { } |
| 55 | + |
| 56 | +// baz.ts |
| 57 | +export interface BazParam { |
| 58 | + foobar: string |
| 59 | +} |
| 60 | + |
| 61 | +export const baz(bag: BazParam) {} |
| 62 | + |
| 63 | +// bar.ts |
| 64 | +import { foo } from './foo' |
| 65 | +import { BazParam, baz } from './baz' |
| 66 | + |
| 67 | +const bag = { |
| 68 | + a: 'a', |
| 69 | + b: false, |
| 70 | + n: 2 |
| 71 | +} |
| 72 | + |
| 73 | +foo(bag) |
| 74 | + |
| 75 | +const bazBag: BazParam = { |
| 76 | + foobar: 'buzz' |
| 77 | +} |
| 78 | +baz(bazBag) |
| 79 | +``` |
| 80 | + |
| 81 | +Better: |
| 82 | +```ts |
| 83 | +// foo.ts |
| 84 | +interface FooParams { |
| 85 | + a: string |
| 86 | + b: boolean |
| 87 | + n: number |
| 88 | +} |
| 89 | + |
| 90 | +export const foo({ a, b, n }: FooParams) { |
| 91 | +} |
| 92 | + |
| 93 | +// bar.ts |
| 94 | +import { foo } from './foo' |
| 95 | + |
| 96 | +const FooParams = Parameters<typeof foo>[0] |
| 97 | + |
| 98 | +const bag: FooParams = { a: 'a', b: false, n: 2 } |
| 99 | +foo(bag) |
| 100 | +``` |
| 101 | + |
| 102 | +Best: |
| 103 | +```ts |
| 104 | +// foo.ts |
| 105 | +interface FooParams { |
| 106 | + a: string |
| 107 | + b: boolean |
| 108 | + n: number |
| 109 | +} |
| 110 | + |
| 111 | +export const foo({ a, b, n }: FooParams) { |
| 112 | +} |
| 113 | + |
| 114 | +// bar.ts |
| 115 | +import { foo} from './foo' |
| 116 | + |
| 117 | +foo({ |
| 118 | + a: 'a', |
| 119 | + b: false, |
| 120 | + n: 2 |
| 121 | +}) |
| 122 | +``` |
| 123 | + |
| 124 | +#### DRY |
| 125 | + |
| 126 | +If you find yourself defining more than two very similar interfaces or types, consider what can be DRY'd. Is it purely the types, or is there further refactoring that should be done? When DRY, prefer defining the type as close to the implementation that depends on that type as possible. Define parameter bags next to the function they're an argument for, and export them only when necessary. New interfaces should almost never need to be declared for intermediary variables in a function - if you find yourself doing this, there are probably structural refactors that should be considered before DRYing the type definitions. |
| 127 | + |
| 128 | +**Exception to DRY**: |
| 129 | + |
| 130 | +Type declarations composed for public API consumption & definition don't need to be as rigorously DRY'd. Readability and understandability is preferred over conciseness. |
| 131 | + |
| 132 | +## Migration Success Criteria |
| 133 | + |
| 134 | +- No unnecessary `.js` in the repository |
| 135 | +- The repository passes linting based on the `@typescript-eslint/recommended-type-checked` rule set |
| 136 | +- The repository uses modern eslint: [stylistic](https://eslint.style/) for formatting, various non-formatting-related plugins for code correctness, and the modern flat config pattern. |
| 137 | +- Optional / Stretch goal: Define a style guide for the repository that may extend past basic linting and formatting constraints. Initial thoughts: |
| 138 | + - There are no circular dependencies |
| 139 | + - There are no deep imports from undeclared exports |
| 140 | + |
| 141 | +## Execution |
| 142 | + |
| 143 | +For shared language purposes, both packages and files can be categorized as "leaf," "shared," or "root" nodes. |
| 144 | +* A Leaf node has no monorepo dependencies |
| 145 | +* A Shared node depends on one or more nodes, and is the dependent of one or more node. |
| 146 | +* A Root node is not imported by any nodes other than `./cli` |
| 147 | + |
| 148 | + |
| 149 | +### Stage 1: Typescript Shift |
| 150 | + |
| 151 | +Working through each package from leaf to shared to root nodes, and files within each package from leaf to shared to root files: |
| 152 | + |
| 153 | +- `.js` files are renamed to `.ts` |
| 154 | +- Type definitions are added _only_ when adding them does not have a domino effect |
| 155 | +- When adding a type definition would have a domino effect, use an `any` type. Because we can warn on the use of `any` types, this serves as an implicit "todo" to come back and fix it. |
| 156 | +- Add definitely-typed dependencies as necessary |
| 157 | +- Add `@ts-expect-error` directives as necessary - we will warn on them, so they become an implicit "todo". |
| 158 | + |
| 159 | +### Stage 2: Reduce Warnings |
| 160 | + |
| 161 | +Beginning again with packages from leaf to root, and within each from leaf file to root file, begin to address warnings. Addressing warnings should be prioritized based on code quality impact. Loosely: |
| 162 | + |
| 163 | +1. `no-require-imports` is a prerequisite to ESM. We use `require` intentionally in several places, so this requires additional consideration. |
| 164 | +2. `no-explicit-any` ensures that we're defining types and interfaces for function parameters and return values. This is the number one tool to help catch bugs and incorrect code. `any` types are a useful escape hatch when first converting a project, but quickly become a crutch. Do the easiest ones first - if you start in on one and realize it has a wide ranging domino effect, add a comment about which other files are involved and move on. That type may become more straightforward to define once other explicit `any` types are filled in. |
| 165 | +3. `require-await`, `await-thenable`, and `no-floating-promises` help reduce logical errors and unnecessary microtasks with asynchronous code. |
| 166 | + |
| 167 | +### Stage 3: Enforce previously warned rules as errors |
| 168 | + |
| 169 | +As warnings are removed from packages, those warnings should be converted to errors so they do not re-occur. These rules are specifically called out when overridden in the base eslint configuration. |
| 170 | + |
| 171 | +## Helpful Tips |
| 172 | + |
| 173 | +* 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, and may help resolve circular dependencies. |
| 174 | +* Circular dependencies can cause havoc - use [madge](https://www.npmjs.com/package/madge) to detect if the file you're working on is included in a circular dependency cycle, and resolve that before converting the file. |
| 175 | +* If you find yourself on a change cascade, pause and back up. Use `any` types to help get to the finish line - we'll warn on them, and they can be fixed later. |
0 commit comments