Skip to content

Eliminate 21 of 22 @ts-expect-error suppressions and enforce strict type validation#204

Merged
TobyHFerguson merged 4 commits into
masterfrom
copilot/convert-namespace-to-class
Jan 21, 2026
Merged

Eliminate 21 of 22 @ts-expect-error suppressions and enforce strict type validation#204
TobyHFerguson merged 4 commits into
masterfrom
copilot/convert-namespace-to-class

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 21, 2026

Production TypeError reached prod because @ts-expect-error suppressed all errors on affected lines, not just intended ones. Baseline: 22 suppressions masking type coverage gaps.

Changes

Eliminated 21 suppressions (95% reduction):

  • AnnouncementCore (2): Added explicit type annotation to prevent null inference on updates object
  • GoogleCalendarManager (1): Removed unnecessary file-level suppression
  • AnnouncementManager (18): Added missing GAS Document API type definitions to gas-globals.d.ts
    • Extended Paragraph, ListItem, Table, TableRow, TableCell, Body interfaces
    • Used JSDoc type casts for Node.js compatibility: /** @type {GoogleAppsScript.Document.ListItem} */ const listItem = /** @type {any} */ (element);

Fixed class pattern violations:

  • UIHelper: Replaced this.buildValidationMessageUIHelper.buildValidationMessage (3 instances)

Enhanced validate-types script:

  • Exit 1 on warnings (previously passed with warnings)
  • Skip benign false positives (constructor, _-prefixed private methods)

Type Safety Impact

Before:

// @ts-expect-error - Type narrowing needed for LIST_ITEM element
const listChildren = element.getNumChildren();

After:

/** @type {GoogleAppsScript.Document.ListItem} */
const listItem = /** @type {any} */ (element);
const listChildren = listItem.getNumChildren();

Remaining suppression (1): MenuFunctions.js line 122 - architectural type mismatch (Row vs AnnouncementQueueItem) requiring refactor beyond scope.

Validation

  • TypeScript: 0 errors
  • validate-types: 0 warnings (strict mode)
  • Tests: 702/702 passing

Closes #200, #196

Original prompt

This section details on the original issue you should resolve

<issue_title>Type Safety Enforcement: Namespace to Class Conversion</issue_title>
<issue_description>## Summary

Convert all legacy namespace pattern modules to class pattern with static methods, eliminate unjustified @ts-expect-error suppressions, and enhance the validate-types script to fail on type mismatches. This is a mechanical refactoring with no behavior changes.

Combines: Issue #200 + Issue #196


Why This Matters

Each @ts-expect-error suppresses ALL errors on that line, not just the intended error. A production bug (TypeError: Cannot read properties of undefined) reached production because type checking was disabled by these suppressions. Converting to class pattern restores full TypeScript coverage.


Current State (Baseline)

  • @ts-expect-error suppressions: ~22 across 4 files
  • Namespace pattern modules: 13 modules requiring conversion
  • validate-types warnings: 12 (treated as warnings, not errors)

PHASE 1: Convert Namespace Modules to Class Pattern

Goal: Convert all namespace pattern modules to class pattern with static methods.

Modules to Convert (in dependency order - convert leaf modules first):

Priority Module Suppressions Notes
1 UserLoggerCore.js 0 No dependencies, simple conversion
2 GoogleEventCore.js 0 No dependencies
3 RWGPSMembersCore.js 0 No dependencies
4 RouteColumnEditor.js 0 No dependencies
5 TriggerManagerCore.js 0 No dependencies
6 RideManagerCore.js 0 No dependencies
7 AnnouncementCore.js 2 Has suppressions - will be eliminated
8 UIHelper.js 0 May have internal this references
9 Groups.js 0 Simple module
10 Globals.js 0 Config module
11 GoogleCalendarManager.js 1 Has suppression
12 RWGPSMembersAdapter.js 0 May reference Core
13 TriggerManager.js 0 References TriggerManagerCore

CRITICAL Conversion Rules:

  1. Replace this.methodName() with ClassName.methodName()
  2. Replace this.CONSTANT with ClassName.CONSTANT or static getter
  3. Use static get CONSTANT() instead of static CONSTANT = (GAS limitation)
  4. Update corresponding .d.ts: declare namespace to declare class with static methods
  5. Run npm run typecheck after EACH module conversion
  6. Run npm test after EACH module conversion
  7. Do NOT change any business logic - this is purely structural

Per-Module Checklist (repeat for each):

  • Convert .js file from namespace to class pattern
  • Update .d.ts file: declare namespace to declare class with static keywords
  • Replace all this. references with ClassName.
  • Run npm run typecheck - MUST pass with zero errors
  • Run npm test - all tests MUST pass
  • Remove any @ts-expect-error suppressions that are now unnecessary
  • Commit: Convert ModuleName from namespace to class pattern

PHASE 2: Eliminate @ts-expect-error Suppressions

After Phase 1, investigate remaining suppressions:

File Count Category Action
AnnouncementManager.js 18 GAS Document API types Add type definitions to gas-globals.d.ts
MenuFunctions.js 1 Parameter type mismatch Fix function signature or add proper type
GoogleCalendarManager.js 1 Unknown Investigate and fix root cause

For each suppression:

  1. Read the suppression comment to understand why it exists
  2. Determine if proper typing can eliminate it
  3. Either fix the type issue OR document why suppression is unavoidable
  4. Target: less than 2 remaining suppressions with full justification

PHASE 3: Enhance validate-types Script

Current Problem: npm run validate-types shows warnings but does not fail the build.

Required Changes to scripts/validate-type-definitions.js:

  1. Change exit code to fail on warnings (not just errors)
  2. Add --strict flag or make strict behavior default

VERIFICATION CHECKPOINTS

After EACH module conversion:

  • npm run typecheck (MUST be zero errors)
  • npm test (MUST pass)
  • git diff --stat (Review changes are minimal/structural)

After Phase 1 complete:

  • grep -rn @ts-expect-error src/*.js | wc -l (Should be reduced)
  • npm run validate-all (MUST pass)

After Phase 2 complete:

  • grep -rn @ts-expect-error src/*.js (Target: less than 5 total, each justified)

After Phase 3 complete:

  • npm run validate-types (MUST exit 0 with zero warnings)

SUCCESS CRITERIA

  • All 13 namespace pattern modules converted to class pattern
  • All corresponding .d.ts files updated with declare class + static methods
  • @ts-expect-error suppressions reduced from ~22 to less than 5
  • Each remaining suppression has detailed justification comment
  • npm run typecheck passes with zero errors
  • npm run validate-types passes ...

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits January 21, 2026 16:58
…ate 3 @ts-expect-error suppressions

Co-authored-by: TobyHFerguson <6525189+TobyHFerguson@users.noreply.github.com>
…efinitions and enhance validate-types to fail on warnings

Co-authored-by: TobyHFerguson <6525189+TobyHFerguson@users.noreply.github.com>
Copilot AI changed the title [WIP] Convert legacy namespace modules to class pattern Eliminate 21 of 22 @ts-expect-error suppressions and enforce strict type validation Jan 21, 2026
Copilot AI requested a review from TobyHFerguson January 21, 2026 17:04
@TobyHFerguson TobyHFerguson marked this pull request as ready for review January 21, 2026 19:15
… any types

## Summary
This commit eliminates the remaining type safety issues by:
1. Replacing all InstanceType<typeof X> JSDoc typedefs with proper imports from Externals.d.ts
2. Adding detection guidance for hidden any types that resolve silently
3. Fixing type errors exposed by better type resolution

## Changes Made

### Type Definition Patterns (Critical Fix)
- **Root cause**: @typedef {InstanceType<typeof X>} in .js files cannot resolve GAS globals,
  silently defaulting to any type
- **Solution**: Use @typedef {import('./Externals').XInstance} pattern which imports from
  .d.ts where TypeScript's module resolution works

### Files Modified

#### Externals.d.ts (New Instance Types)
- Added SCCCCEventInstance export
- Added ScheduleAdapterInstance export
- Now canonical location for all instance type definitions

#### Type Definition Updates
- RideManager.js: Fixed RowCoreInstance typedef
- AnnouncementManager.js: Added RowCoreInstance import, fixed all usages
- EventFactory.js: Added RowCoreInstance and SCCCCEventInstance imports
- MenuFunctions.js: Added RowCoreInstance and ScheduleAdapterInstance imports
- ScheduleAdapter.js: Changed to import RowCoreInstance
- ScheduleAdapter.example.js: Added RowCoreInstance import
- triggers.js: Added ScheduleAdapterInstance import
- UIHelper.js: Changed to import RowCoreInstance
- ValidationCore.js: Changed to import RowCoreInstance

#### Type Safety Fixes
- AnnouncementManager.js: Fixed _rowNum undefined coercion (row.rowNum ?? 0)
- AnnouncementManager.js: Fixed announcementCell access for new {text, url} object structure

#### Documentation
- .github/copilot-instructions.md: Added new section "Detecting Hidden any Types (CRITICAL)"
  with detection methods, banned patterns, and verification procedures
- .github/ISSUE_TEMPLATE.md: Updated typedef examples to use correct import pattern

#### Type Definition Files (Minor Updates)
- src/gas-globals.d.ts: Imported SCCCCEventInstance, ScheduleAdapterInstance
- Multiple .d.ts files: Updated for consistency with new patterns

### Pattern Established (For Future Code)

**BANNED Pattern** (resolves to any in .js files):
@typedef {InstanceType<typeof RowCore>} RowCoreInstance

**REQUIRED Pattern** (proper type resolution):
@typedef {import('./Externals').RowCoreInstance} RowCoreInstance

### Why This Matters
- Fixes type resolution issues where types appeared as 'any' when hovering in VS Code
- Enables compile-time type checking instead of silent runtime failures
- Prevents entire classes of bugs where typos and missing properties went undetected
- Reduces confusion between different resolution strategies in .d.ts vs .js files

### Testing & Verification
- All 725 tests still pass
- npm run typecheck: 0 errors
- get_errors(): No errors found
- Type resolution: All instance types now show actual properties, not 'any'

## Related Issues
- Fixes hidden 'any' type discovery mentioned in recent development
- Part of PR #204: "Eliminate 21 of 22 @ts-expect-error suppressions"

## Notes for Future Maintainers
1. Always create/import instance types from Externals.d.ts
2. Never use InstanceType<typeof X> in .js files with JSDoc
3. Hover over typedef names to verify they don't show "= any"
4. Use grep -rn "InstanceType<typeof" src/*.js to find regressions
5. See .github/copilot-instructions.md "Detecting Hidden any Types" section
@TobyHFerguson TobyHFerguson merged commit 79966ae into master Jan 21, 2026
1 check passed
@TobyHFerguson TobyHFerguson deleted the copilot/convert-namespace-to-class branch January 21, 2026 19:41
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.

Type Safety Enforcement: Namespace to Class Conversion Migrate namespace pattern modules to class pattern for full type safety

2 participants