Uh oh!
There was an error while loading. Please reload this page.
feat: implement is() type checking function - #3
Conversation
Add is() function for checking error type with inheritance support: - Symbol-based factory identity (prevents name collision issues) - Proper native error handling via instanceof - Inheritance chain walking for parent type checks - Full instanceof compatibility (errors extend native Error) Key fixes after review: - Use Symbol.for() for factory identity instead of string names - Leverage instanceof for native errors instead of hardcoded list - Store factory reference via Symbol on error instances - Errors now properly extend Error class for native compatibility Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
b102c16 to
83aceeeCompareKey fixes: - DFS algorithm with stack for proper multiple inheritance support - Proper type inference: ExtractFields<T> for accurate type narrowing - Cyclic protection with seen Set - No array allocation per iteration (reuse stack) - Instance points to factory, factory holds inheritance metadata Type safety improvements: - is(err, SyntaxError) now returns error is SyntaxError - is(err, CustomError) returns error is ErrorInstance<CustomFields> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
martyy-code
left a comment
There was a problem hiding this comment.
PR Review: feat: implement is() type checking function
Summary
Solid implementation overall. The DFS algorithm with cyclic protection is correct, Symbol-based identity using Symbol.for() is appropriate for cross-realm compatibility, and the type inference via ExtractFields<T> is a reasonable approach given TypeScript's constraints. The PR makes good architectural decisions with proper separation of concerns.
What Works Well
- Symbol.for() usage is correct - creates globally shared symbol for cross-realm error identity (cross-frame iframe scenarios)
- DFS with seen Set correctly prevents infinite loops in cyclic inheritance without GC pressure from recursive calls
- Native error handling with instanceof and try-catch for cross-realm errors is well implemented
- Both single and array
inheritssupport is handled correctly in the traversal - Proper instanceof Error behavior on factory-created errors improves debuggability
- Good test coverage - tests cover single inheritance, multiple inheritance, deep chains, native errors, and edge cases
Blocking Issues
ExtractFields for native errors falls to
Record<string, unknown>(non-blocking for correctness, but affects type narrowing precision)The type definition:
typeExtractFields<T>=TextendsErrorFactory<infer F> ? F : Textendsnew( ...args: unknown[])=> infer E ? EextendsErrorInstance<infer F> ? F : Record<string,unknown> : Record<string,unknown>;
For
is(err, SyntaxError), the chain is:SyntaxError extends new (...) => ErrorbutErroris notErrorInstance<any>, so it returnsRecord<string, unknown>. This is acceptable - native errors don't have the fields structure anyway.Type narrowing with multiple inheritance returns only one branch's fields (inherent TypeScript limitation, not a bug)
When
CombinedErrorinherits from[NetworkError, StorageError], type narrowing viais(err, NetworkError)returnsErrorInstance<{networkField: ...}>rather than a union. This is standard TypeScript behavior for type predicates - you get the fields corresponding to that specific branch. The implementation is correct; this is a limitation of how TypeScript handles union types in narrowing.
Suggestions (Non-blocking)
Consider extracting the inheritance traversal into a dedicated helper function for reusability, especially if
.from()or other methods will need similar traversal logic. Current implementation is correct but scattered across the is() function.The
instanceof Errorcheck succeeds becausenew Error(message)is used internally. However,instanceofchecks against the prototype chain, not the FACTORY_SYMBOL. This is intentional and correct - the Symbol enables type-safeis()checking whileinstanceof Errorprovides basic Error compatibility. No change needed.Cyclic protection: The
seenSet correctly handles cyclic inheritance by skipping already-visited factories. This is robust.
Answering Key Questions
Is the DFS algorithm correct for multiple inheritance?
Yes. The stack-based DFS correctly explores all parent branches. Singleinherits: Parentand arrayinherits: [A, B]are both handled. TheseenSet prevents revisiting nodes in diamond inheritance patterns.Is the type inference (ExtractFields) sound?
Yes, with two caveats: (a) native errors fall back toRecord<string, unknown>which is acceptable, (b) multiple inheritance returns single-branch fields which is a TypeScript limitation, not an implementation bug.Any logic bugs or edge cases missed?
No. Null/undefined handling, cross-realm instanceof failures, non-error objects, native errors without factory markers, and cyclic protection are all handled correctly.Is the Symbol-based identity approach correct?
Yes.Symbol.for('@deessejs/errors/factory')ensures the same symbol is shared across realms (iframes, workers, different JS contexts). This is the correct approach for cross-realm error identity.
Recommendation
Comment Only - The implementation is correct and ready to merge. No changes required.
Summary
is()function for runtime type checking of error instancesinstanceofcompatibilityChanges
New
is()functionBreaking change:
_factory→_nameThe previous
_factoryreference-based comparison was unreliable. Replaced with_namestring comparison for stable type identity.Implementation details
Errorclass for properinstanceofsupport_nameproperty stores the factory name for type checkingTest plan
🤖 Generated with Claude Code