Skip to content

[Refactor]: Remove redundant typeof/null guard after narrowing in is/index.ts #73

Description

@martyy-code

Current State

In packages/errors/src/is/index.ts:60-78, the is() function performs a null check on error at the top of the function, then re-checks typeof error === 'object' && error !== null ten lines later before accessing error[FACTORY_SYMBOL]. The compiler has already narrowed error to object; the second check is redundant and reads as the author having lost confidence in their own code.

// Line 60-63
if (error == null) {
  return false;
}

// Line 77-78
if (typeof error === 'object' && error !== null) {
  const marker = error as Record<typeof FACTORY_SYMBOL, unknown>;

Rule 0004 names this exact pattern in its "What this looks like in violation" section:

"A function declared with err: unknown that, three statements later, re-checks typeof err === 'object' && err !== null even though a prior branch already returned on err == null. The author was not sure the narrowing survived the intervening statements."

Located in:

  • packages/errors/src/is/index.ts:77-78

Problems with current implementation:

  • The compiler has the narrowing. The re-check is a tax on every reader who has to confirm the second check is dead.
  • It mirrors the canonical violation example in rule 0004.
  • The pattern signals "the author didn't trust their own type narrowing", which propagates distrust to the next contributor.

Proposed State

After refactoring, the re-check is removed. The function accesses error[FACTORY_SYMBOL] directly after the top-level null guard:

if (error == null) {
  return false;
}

// Native errors
if (typeof ErrorType === 'function' && 'prototype' in ErrorType) {
  return error instanceof ErrorType;
}

// Factory instances
if (typeof error === 'object') {
  const marker = error as Record<typeof FACTORY_SYMBOL, unknown>;
  // ...
}

Expected improvements:

  • The narrowing is trusted (rule 0004's trust-the-type principle).
  • One fewer branch in the hot path.
  • The reader can follow the narrowing without re-auditing the second check.

Motivation

This refactoring is needed because:

  • It is the canonical violation example in rule 0004.
  • The project slogan ("trust the type") is operationalised by trusting the narrowing; this code does the opposite.
  • The redundant branch is noise that hides the actual logic of the function.

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: TypeScript's narrowing across the instanceof block may be lost, causing a new type error on error[FACTORY_SYMBOL]. — Mitigation: the compiler tracks narrowing across early-returns and type guards. If a regression appears, the fix is a single typeof error === 'object' cast at the access point, not the re-check.
  • Risk 2: Consumer code paths where error is null reach the second block. — Mitigation: the top-level if (error == null) return false; covers this. The re-check does not add safety.

Migration Plan

Migration approach:

  1. Remove the typeof error === 'object' && error !== null wrapper.
  2. Run pnpm --filter @deessejs/errors type-check to confirm the narrowing is preserved.
  3. Run the test suite to confirm runtime behaviour is unchanged.

Rollback plan: revert the PR. The change is local.

Backward Compatibility

  • This refactoring maintains full backward compatibility

Scope

Files/Folders affected:

  • packages/errors/src/is/index.ts (lines 77-78)

Out of scope:

Component(s) Affected

  • Multiple Components

Note: the component_affected dropdown is calibrated for a web template project. The actual affected component is packages/errors.

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

Test Coverage Requirements

  • Existing tests cover this code area (will update)

Testing Approach

Testing strategy:

  • Unit tests: existing is() test cases (factory instances, native errors, null/undefined) should all pass without modification.

Verification steps:

  1. pnpm --filter @deessejs/errors test:run
  2. pnpm --filter @deessejs/errors type-check

Related Issues / Pull Requests

Relevant Documentation

  • Architecture doc: docs/engineering/architecture/rules/0004-no-speculative-defences.md

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)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    p1: highRequired for next releasetype: refactorRefactoring / code restructuring

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions