Upgrade firebase-tools to TypeScript 7#10781
Conversation
There was a problem hiding this comment.
Code Review
This pull request upgrades TypeScript to version 7.0.2, configures TypeScript compilation settings, and refactors namespace imports to default imports across the codebase. The review feedback highlights multiple instances where any was introduced as an escape hatch to resolve type errors (such as in config.ts, config.spec.ts, utils.ts, and index.ts), violating the repository's style guide. Actionable suggestions are provided to restore type safety in these files using proper type annotations or omitting redundant type parameters.
| @@ -1,5 +1,5 @@ | |||
| import * as sinon from "sinon"; | |||
| import * as auth from "../../auth"; | |||
| const auth = require("../../auth"); | |||
There was a problem hiding this comment.
Using require without type annotations results in auth being typed as any, which loses all type safety and violates the style guide rule against using any as an escape hatch. We can preserve full type safety while still using require at runtime (to allow Sinon stubbing of the CommonJS exports) by using a typeof import type annotation.
| const auth = require("../../auth"); | |
| const auth: typeof import("../../auth") = require("../../auth"); |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| } | ||
| const envs = document.get("env") as yaml.YAMLSeq; | ||
| for (const env of envs.items as Array<NodeType<Env>>) { | ||
| for (const env of envs.items as any) { |
There was a problem hiding this comment.
Casting envs.items to any violates the style guide rule against using any as an escape hatch. Since the items in the env sequence are parsed as maps, we can cast to yaml.YAMLMap[] to maintain type safety.
| for (const env of envs.items as any) { | |
| for (const env of envs.items as yaml.YAMLMap[]) { |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| return; | ||
| } | ||
| const envs = document.get("env") as yaml.YAMLSeq<NodeType<Env>>; | ||
| const envs = document.get("env") as yaml.YAMLSeq<any>; |
There was a problem hiding this comment.
Casting envs to yaml.YAMLSeq<any> violates the style guide rule against using any as an escape hatch. We can use yaml.YAMLSeq<yaml.YAMLMap> instead to maintain type safety.
| const envs = document.get("env") as yaml.YAMLSeq<any>; | |
| const envs = document.get("env") as yaml.YAMLSeq<yaml.YAMLMap>; |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| describe("get/setEnv", () => { | ||
| it("sets new envs", () => { | ||
| const doc = new yaml.Document<NodeType<config.Config>>(); | ||
| const doc = new yaml.Document<any>(); |
There was a problem hiding this comment.
Using any as a type parameter violates the style guide. Since yaml.Document has default type parameters, we can omit the type parameter entirely to let it default to Node.
| const doc = new yaml.Document<any>(); | |
| const doc = new yaml.Document(); |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
|
|
||
| it("overwrites envs", () => { | ||
| const doc = new yaml.Document<NodeType<config.Config>>(); | ||
| const doc = new yaml.Document<any>(); |
There was a problem hiding this comment.
Using any as a type parameter violates the style guide. Since yaml.Document has default type parameters, we can omit the type parameter entirely to let it default to Node.
| const doc = new yaml.Document<any>(); | |
| const doc = new yaml.Document(); |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| `; | ||
|
|
||
| const doc = yaml.parseDocument(rawDoc) as yaml.Document<NodeType<config.Config>>; | ||
| const doc = yaml.parseDocument(rawDoc) as yaml.Document<any>; |
There was a problem hiding this comment.
Casting to yaml.Document<any> violates the style guide. Since yaml.parseDocument already returns a typed yaml.Document, we can remove the cast entirely.
| const doc = yaml.parseDocument(rawDoc) as yaml.Document<any>; | |
| const doc = yaml.parseDocument(rawDoc); |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| } else { | ||
| const { openConfig }: typeof import("astro/dist/core/config/config") = | ||
| await dynamicImport(configPath); | ||
| const { openConfig } = (await dynamicImport(configPath)) as any; |
There was a problem hiding this comment.
Casting the dynamic import to any violates the style guide rule against using any as an escape hatch. We can define a local interface representing the expected structure of the module to maintain type safety.
| const { openConfig } = (await dynamicImport(configPath)) as any; | |
| const { openConfig } = (await dynamicImport(configPath)) as { | |
| openConfig: (options: { cmd: string; cwd: string; logging: any }) => Promise<{ astroConfig: any }>; | |
| }; |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
Description
TSC 7 is apparently way faster, so i'd love to upgrade. This is Jetski's attempt to get it up and running - it works, but we did have to do a few ugly things to support some of our older deps.