Skip to content

Upgrade firebase-tools to TypeScript 7#10781

Draft
joehan wants to merge 3 commits into
mainfrom
update-typescript-7
Draft

Upgrade firebase-tools to TypeScript 7#10781
joehan wants to merge 3 commits into
mainfrom
update-typescript-7

Conversation

@joehan

@joehan joehan commented Jul 9, 2026

Copy link
Copy Markdown
Member

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/test/helpers/index.ts
@@ -1,5 +1,5 @@
import * as sinon from "sinon";
import * as auth from "../../auth";
const auth = require("../../auth");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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.

Suggested change
const auth = require("../../auth");
const auth: typeof import("../../auth") = require("../../auth");
References
  1. Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)

Comment thread src/apphosting/config.ts
}
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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
for (const env of envs.items as any) {
for (const env of envs.items as yaml.YAMLMap[]) {
References
  1. Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)

Comment thread src/apphosting/config.ts
return;
}
const envs = document.get("env") as yaml.YAMLSeq<NodeType<Env>>;
const envs = document.get("env") as yaml.YAMLSeq<any>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
const envs = document.get("env") as yaml.YAMLSeq<any>;
const envs = document.get("env") as yaml.YAMLSeq<yaml.YAMLMap>;
References
  1. 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>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
const doc = new yaml.Document<any>();
const doc = new yaml.Document();
References
  1. 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>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
const doc = new yaml.Document<any>();
const doc = new yaml.Document();
References
  1. 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>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Casting to yaml.Document<any> violates the style guide. Since yaml.parseDocument already returns a typed yaml.Document, we can remove the cast entirely.

Suggested change
const doc = yaml.parseDocument(rawDoc) as yaml.Document<any>;
const doc = yaml.parseDocument(rawDoc);
References
  1. 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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
  1. Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)

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