Skip to content

[Refactor]: Replace chained cast in error.ts:130 with a single cast on the declared type #71

Description

@martyy-code

Current State

In packages/errors/src/error/error.ts:130-131, the marker used to identify factory-created errors is attached via a chained type assertion that violates the project's own rule 0008 (No Chained Type Assertions):

(instance as unknown as Record<typeof FACTORY_SYMBOL, () => unknown>)[FACTORY_SYMBOL] =
  ErrorFactoryInstance;

The first as unknown erases the Error type. The second as Record<...> reinvents a type that already exists. This is the canonical example of the pattern the rule was written to forbid, and it ships in the file that produces the library's main export (error()).

Located in:

  • packages/errors/src/error/error.ts:130-131
  • Indirectly in packages/errors/src/error/types.ts:40-84 (the ErrorInstance<T> type already declares the augmented shape)

Problems with current implementation:

  • Two casts in one expression — explicitly forbidden by rule 0008.
  • The as unknown as Record<typeof FACTORY_SYMBOL, () => unknown> cast is mechanical noise that the ErrorInstance<T> type makes unnecessary: instance is already declared as ErrorInstance<T> two lines earlier.
  • The cast hides the fact that the author could not type the property assignment directly. A reader has to mentally reconstruct why a property assignment needs two type-system escapes.

Proposed State

After refactoring, the assignment becomes a single cast that crosses one boundary (the new Error() constructor returns a base Error, not the augmented ErrorInstance<T>), and the property assignment happens on the declared type without a second cast.

Expected improvements:

  • One cast in the expression, not two (rule 0008 compliance).
  • The marker assignment is type-checked against ErrorInstance<T> directly.
  • The example in rule 0008's "What this looks like in violation" section is no longer mirrored in the library's source.

Motivation

This refactoring is needed because:

  • The library's own documentation (rule 0008 in docs/engineering/architecture/rules/0008-no-chained-type-assertions.md) names this exact pattern as the canonical violation. The library should not be a counter-example to its own rules.
  • Invariant 7 of rule 0001 (Project Mindset): no compiler bypass. The chained cast is a compiler bypass.
  • Reader cost: every reader of error.ts has to mentally replay the cast to understand why the assignment is structured this way.

Triggers for this work:

  • Working on feature X and encountered this (audit of packages/errors/src/ against the 16 architecture rules, August 2026)
  • Technical debt accumulation
  • Performance issues
  • Maintainability concerns

Risks

Potential risks:

  • Risk 1: Breaking the public ErrorInstance<T> shape if the property is removed by accident — mitigation: keep the FACTORY_SYMBOL declaration and the assignment; only remove the chained cast.
  • Risk 2: Regression in is() discrimination if the marker is not attached correctly — mitigation: existing test suite in packages/errors/tests/ covers factory-vs-instance checks; the refactor should leave the test suite green.
  • Risk 3: TypeScript version sensitivity (the project pins typescript: ^6.0.3) — mitigation: the single-cast shape uses stable language features (as ErrorInstance<T> on a new Error() return) and is independent of compiler minor versions.

Migration Plan

Migration approach:

  1. Change const instance = new Error(errorMessage) as ErrorInstance<T>; is already the shape; the chained cast is on the assignment, not the construction. Replace lines 130-131 with instance[FACTORY_SYMBOL] = ErrorFactoryInstance; once instance is typed as ErrorInstance<T>.
  2. Verify that FACTORY_SYMBOL is declared as a property of ErrorInstance<T> in error/types.ts (or add it if missing — currently the symbol is Symbol.for('@deessejs/errors/factory') and is attached dynamically, which is the root cause of the cast).
  3. Run the existing test suite; add a regression test that constructs an error via error() and verifies instance[FACTORY_SYMBOL] === ErrorFactoryInstance without any cast at the call site.
  4. Update rule 0008's "What this looks like in violation" section to point to a new example rather than mirroring this one (optional, after the refactor lands).

Rollback plan: revert the PR. The change is local to error/error.ts and does not touch the public API.

Backward Compatibility

  • This refactoring maintains full backward compatibility
  • This refactoring has breaking changes (migration required)
  • This refactoring deprecates APIs (grace period needed)

Scope

Files/Folders affected:

  • packages/errors/src/error/error.ts (lines 130-131)
  • packages/errors/src/error/types.ts (may need FACTORY_SYMBOL declared on ErrorInstance<T>)
  • packages/errors/tests/ (regression test)

Out of scope:

  • Refactoring the rest of error.ts (covered by separate issues: P0 [Chore]: Add badges to README.md #7 — generic verb naming of ErrorFactoryInstance).
  • Changing the public ErrorInstance<T> shape beyond the marker declaration.

Component(s) Affected

  • packages/db — Drizzle ORM schema + PostgreSQL
  • packages/api — tRPC server
  • packages/api/auth — Better Auth configuration
  • .github/workflows — CI/CD GitHub Actions
  • Multiple Components

Note: the component_affected dropdown is calibrated for a web template project, not for the @deessejs/errors package. The actual affected component is packages/errors (the published library). Flagging this on the project's issue template is a separate concern, not blocking.

Priority

  • p0: Critical - Blocking major work or causing bugs
  • p1: High - Important, should do soon
  • p2: Medium - Normal priority
  • p3: Low - Nice to have

Estimated Effort

  • effort: xs - Few minutes
  • effort: s - Half a day
  • effort: m - 1-2 days
  • effort: l - A week or more (needs breakdown)

Test Coverage Requirements

  • Existing tests cover this code area (will update)
  • Need to add new tests for this refactor
  • This area lacks test coverage (technical debt)
  • Integration tests will be added/updated
  • E2E tests will be added/updated

Testing Approach

Testing strategy:

  • Unit tests: a test that constructs an error via error() and asserts the marker is attached to the returned instance without any cast at the call site.
  • Integration tests: re-run the existing inheritance / is() test suite to confirm the marker still discriminates correctly.

Verification steps:

  1. pnpm --filter @deessejs/errors test:run — all existing tests pass.
  2. pnpm --filter @deessejs/errors type-check — no new type errors.
  3. pnpm --filter @deessejs/errors lint — no new lint errors.

Related Issues / Pull Requests

Relevant Documentation

  • Architecture doc: docs/engineering/architecture/rules/0008-no-chained-type-assertions.md
  • Architecture doc: docs/engineering/architecture/rules/0001-project-mindset.md (invariant 7: no compiler bypass)

Pre-Submission Checklist

  • I have searched existing issues for related refactoring requests
  • Risks and migration plan are documented
  • Test coverage approach is defined
  • I understand this issue will be labeled according to the project taxonomy
  • This is NOT a security vulnerability (see security note above)

Metadata

Metadata

Assignees

No one assigned

    Labels

    p0: criticalEverything stops, fix it nowtype: refactorRefactoring / code restructuring

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions