Conversation
### Description This PR refactors `catch (err: any)` to `catch (err: unknown)` in the `src/emulator/` directory to improve type safety and reduce weak typing in the codebase. Safe property access and type guards have been applied where necessary. ### Scenarios Tested Ran full test suite with `npm test`. 4270 tests passed, 3 failed in Auth Emulator tests (likely flaky or timing issues). ### Sample Commands `npm test`
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several updates, including support for Python 3.14 in Cloud Functions, the addition of SSE mode and port configuration for the MCP server, and a new MCP App for environment management. It also improves security by adding confirmation prompts for local builds using secrets and fixes a project validation issue in hosting deploys. A significant portion of the changes involves refactoring error handling to use unknown instead of any for better type safety. Feedback focuses on remaining instances of any and unknown type assertions that violate the repository's style guide regarding type safety.
I am having trouble creating individual review comments. Click here to see my feedback.
src/appdistribution/distribution.spec.ts (71)
The use of as unknown as any is an escape hatch that violates the repository style guide. Even in tests, it is preferred to define proper interfaces or use more specific types to maintain type safety and code quality.
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
src/apphosting/secrets/index.ts (285)
The catch clause uses any, which violates the style guide. Given that this pull request refactors many other catch clauses in the emulator directory to use unknown, this instance should also be updated for consistency and better type safety.
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
src/mcp/apps/update_environment/mcp-app.ts (15-16)
Using any[] for projects and filteredProjects violates the style guide. Please define a proper interface for the project objects to ensure type safety throughout the application logic.
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
src/mcp/util.ts (46)
The type assertion using any is an escape hatch that should be avoided. Consider defining an interface that extends CallToolResult to include the structuredContent property with a more specific type.
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
src/mcp/resources/update_environment_ui.ts (28)
The catch clause uses any, which is discouraged by the style guide. It should be refactored to use unknown to align with the error handling patterns established in other parts of this pull request.
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates error handling in catch blocks from 'any' to 'unknown' across various emulator components to improve type safety. The review feedback highlights several areas where unsafe type assertions (such as 'as any' or 'as Error') are still being used, which circumvents the benefits of the migration and violates the repository's style guide. Additionally, the reviewer pointed out instances of redundant casting where TypeScript's type narrowing already safely identifies the error properties.
### Description This PR addresses review comments on type safety by removing unsafe type assertions and refactoring catch clauses in files outside the emulator directory as requested by the reviewer. ### Scenarios Tested Ran full test suite with `npm test`. All 4273 tests passed successfully. ### Sample Commands `npm test`
joehan
changed the title
…error.ts ### Description This PR deduplicates error handling code in the `src/emulator/` directory by utilizing utilities like `getError`, `getErrMsg`, `getErrStack`, and `getErrStatus` from `src/error.ts` as requested by the user. ### Scenarios Tested Ran full test suite with `npm test`. All 4273 tests passed successfully. ### Sample Commands `npm test`
| res.status(500).send(err.message); | ||
| } catch (err: unknown) { | ||
| new EmulatorLog("FATAL", "runtime-error", getErrStack(err)).log(); | ||
| res.status(500).send(getErrMsg(err)); |
| displayVersion: "1.0", | ||
| buildVersion: "1", | ||
| name: "test-rel", | ||
| } as unknown as Release, |
There was a problem hiding this comment.
nit: no need unknown here
| deviceExecutions: [ | ||
| { state: "PASSED", device: { model: "Pixel" } as unknown as TestDevice }, | ||
| ], | ||
| } as unknown as ReleaseTest); |
There was a problem hiding this comment.
nit: no need unknown here
| name: "tests/1", | ||
| deviceExecutions: [{ state: "PASSED", device: { model: "Pixel" } as unknown as any }], | ||
| deviceExecutions: [ | ||
| { state: "PASSED", device: { model: "Pixel" } as unknown as TestDevice }, |
There was a problem hiding this comment.
nit: no need unknown here
| { | ||
| state: "FAILED", | ||
| failedReason: "Crash", | ||
| device: { model: "Pixel" } as unknown as TestDevice, |
4 participants
Description
Improve the typing of our error catches across the emulator suite.
This is part of a larger AI driven effort to try to clean up some of the weaker types we have in places across the codebase.