Skip to content

chore: enable TypeScript strict mode#3112

Open
kyungseopk1m wants to merge 2 commits intofirebase:mainfrom
kyungseopk1m:chore/enable-strict-true
Open

chore: enable TypeScript strict mode#3112
kyungseopk1m wants to merge 2 commits intofirebase:mainfrom
kyungseopk1m:chore/enable-strict-true

Conversation

@kyungseopk1m
Copy link
Copy Markdown

Summary

Completes the long-standing TODO in tsconfig.json:

// TODO(rsgowman): enable `"strict": true,` and remove explicit setting of: ...
  • Replaces 6 individual strict compiler options with "strict": true
  • Enables strictPropertyInitialization (previously commented out)
  • Fixes all resulting TS2564 errors using definite assignment assertions (!:) for properties initialized via Object.defineProperty-based helpers (addReadonlyGetter, copyAttr) or Promise executor callbacks
  • Fixes TS18046 errors from newly enabled useUnknownInCatchVariables with as Error casts in catch blocks
  • Refactors ServerTemplateImpl.cache to use optional type (?:) with a getCachedTemplate() helper for proper runtime validation; adds corresponding tests

Changes

File Change
tsconfig.json Replace 6 explicit strict flags with "strict": true
src/**/*.ts (16 files) Add !: / ?: / as Error to resolve new strict errors
test/unit/remote-config/remote-config.spec.ts Add tests for cache-not-loaded error paths

tsc --noEmit passes with zero errors after these changes.

Replace individual strict compiler options with "strict": true in
tsconfig.json. This also enables strictPropertyInitialization, which
required adding definite assignment assertions (!:) to properties
initialized via Object.defineProperty helpers or Promise executors.
Additionally fixes useUnknownInCatchVariables errors introduced by the
new flag.
Copy link
Copy Markdown
Contributor

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

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 enables strict type checking in the TypeScript configuration and updates the codebase to comply with these stricter rules. The changes involve adding definite assignment assertions, making certain properties optional, and explicitly casting error types in catch blocks across various modules. Additionally, the RemoteConfig implementation was refactored to centralize cache validation into a new helper method, supported by additional unit tests. Review feedback suggests using dynamic method names instead of hardcoded strings in error messages to improve maintainability.

Comment thread src/remote-config/remote-config.ts Outdated
'failed-precondition',
'No Remote Config Server template in cache. Call load() before calling evaluate().');
}
const cachedTemplate = this.getCachedTemplate('evaluate()');
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

The method getCachedTemplate is called with 'evaluate()' as an argument. While this works, it is better to pass the method name dynamically using this.evaluate.name to avoid hardcoding the string and ensure it stays in sync if the method is renamed.

Comment thread src/remote-config/remote-config.ts Outdated
* @returns JSON representation of the server template
*/
public toJSON(): ServerTemplateData {
return this.getCachedTemplate('toJSON()');
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

Similar to the evaluate method, use this.toJSON.name instead of the hardcoded string 'toJSON()' to improve maintainability.

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.

1 participant