Skip to content

[Refactor]: Move ErrorInstance implementation to an internal class while keeping the FP public API #88

Description

@martyy-code

Context

The lead's review of PR #87 (branded ErrorInstance) noted that the implementation is structurally OOP (carries methods, owns mutable state, has identity) but written in a functional style that requires casts and symbols to simulate class semantics. Three options were proposed:

  1. Phantom brand: 0 octet runtime, type-only. Loses the runtime guard capability that FACTORY_SYMBOL already provides.
  2. Branding runtime via object literal: builds the object in one pass with the brand as a literal key. Cleaner than the current new Error() + cast + helper pattern. Pure refactor; preserves the FP API.
  3. Class-based internals, FP externals: a private class implements the methods and the brand; the public API is the factory function that returns a typed object (not the class).

Option 2 lands cleanly inside PR #87. Option 3 is a structural shift that deserves its own decision.

Decision to evaluate

The lead suggested moving the implementation to OOP while keeping the API purely functional. Concretely:

  • A private class (ErrorInstanceImpl<TFields>) owns the methods (from, addNote), the mutable state (notes, causes, context), and the brand assignment.
  • The factory error() instantiates the class internally, binds the public-facing methods, and returns a typed object whose type is exposed but whose class is not.
  • The public API (error, is, causes, raise) is unchanged: factory functions return typed values, no new is exposed.

This would resolve several smells from the current shape:

  • The brand-instance helper (brandInstance(instance)) becomes the class constructor's job. The cast on the brand property disappears.
  • The addNote / from methods, currently attached via instance.addNote = ... with no type narrowing, are real class methods with proper this types.
  • The brand is set in the constructor (the only assignment site), making it impossible to construct an ErrorInstance that lacks the brand.
  • Rule 0014's "no export class" remains satisfied: the class is internal to the module; only the factory function is exported.

Trade-offs

Easier:

  • Removes the as cast at the construction site.
  • Methods get this types instead of capturing instance via const.
  • The brand becomes constructor-enforced, not a property assignment after the fact.
  • Aligns with how mature error libraries (VError, http-errors, Node's own ERR_* patterns) implement this internally.

Harder:

  • The bundle shape changes: the class survives in the compiled output, but only the factory call sites see it. Tree-shaking removes the class symbol after the factory call returns (the returned value is an object, not the class). Bundle size impact: minimal.
  • Rule 0014 is satisfied by keeping the class non-exported. This requires discipline — any temptation to export class ErrorInstanceImpl for "convenience" would violate the rule. The naming convention (Impl suffix, never exported) is the guard.
  • The brand property becomes constructor-enforced. If a future contributor wants to bypass the constructor (e.g. for testing), they have to either reflect the brand manually or use a testing helper. The current brandInstance helper from PR feat(errors): brand ErrorInstance so the type can be trusted at boundaries #87 is the testing escape hatch and should be preserved as a private export.

Stack interaction

PR #87 is the bottom of a 3-PR sequence described in its description:

  1. feat(errors): brand ErrorInstance so the type can be trusted at boundaries #87 (in review) — brand ErrorInstance<T>. Pure addition.
  2. (next) — implement is() predicate (issue [BUG] is() should return TypeScript type predicate for narrowing #35). Brand becomes reachable to consumers.
  3. (later) — shrink public function parameters, retire dead guards.

The class-based refactor would sit at position 0 of the stack, before #87. It would obsolete the brandInstance helper (the constructor sets the brand) and the as cast (the class is the type). The brand-as-symbol concept survives — the class declares [ErrorInstanceBrand]: 'ErrorInstance' on its instances — but the construction shape becomes natural.

The cleanest sequence is therefore:

Scope

Revisit conditions

This issue should be revisited when:

  • A class is needed in the public API for any reason (rule 0014 carve-out expansion). Then this refactor's discipline needs revisiting.
  • The brand-property pattern proves insufficient for some new feature (e.g. prototype-chain introspection, Symbol.hasInstance customisation).
  • A bundle-size audit flags the class as a regression.

Effort and priority

  • Effort: m - 1-2 days. The class is straightforward; the tests pass unchanged.
  • Priority: p2: medium. The class refactor is an improvement but not blocking. PR feat(errors): brand ErrorInstance so the type can be trusted at boundaries #87 lands the brand without it; the class work can follow in a focused PR.
  • Milestone: arch-rules-audit if bundled with the brand work; otherwise a new milestone for the type-system revamp.

Related

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

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions