Skip to content

feat: implement error() factory function - #1

Merged
codewizdave merged 15 commits into
devfrom
task/01-error-factory
May 29, 2026
Merged

feat: implement error() factory function#1
codewizdave merged 15 commits into
devfrom
task/01-error-factory

Conversation

@martyy-code

Copy link
Copy Markdown
Contributor

Summary

Implement the error() factory function, the core building block of @deessejs/errors.

  • Creates typed, structured errors following Standard Schema specification for field definitions
  • Supports single and multiple inheritance via ErrorFactory references
  • Message templates with {field} placeholders and :upper/:lower/:json modifiers
  • Full ErrorInstance with all required properties (name, message, stack, fields, notes, cause, causes, context, httpStatus)

Test Plan

  • 36 unit tests passing covering all acceptance criteria:
    • Basic usage
    • ErrorInstance properties
    • inherits option (single and multiple)
    • Message templates
    • httpStatus
    • Standard Schema support
    • Type inference
    • Factory identity
    • Stack traces
  • TypeScript type-check passes
  • ESLint passes
  • Build succeeds

🤖 Generated with Claude Code

Add the core error() factory that creates typed, structured errors following
the Standard Schema specification for field definitions.
Features:
- Accept config with name, fields (Standard Schema), inherits, message, httpStatus
- Support single and multiple inheritance via ErrorFactory references
- Message templates with {field} placeholders and :upper/:lower/:json modifiers
- Full ErrorInstance with name, message, stack, fields, notes, cause, causes,
context, httpStatus
- Type inference for fields via Standard Schema
- Factory metadata (name, inherits, schema, template, httpStatus)
Tests:
- 36 unit tests covering all acceptance criteria
- Basic usage, ErrorInstance properties, inherits option, message templates,
httpStatus, Standard Schema support, type inference, factory identity, stack traces
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment threadpackages/errors/src/index.ts Outdated
Comment on lines +34 to +35
template?: string;
httpStatus?: number;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not want theses two

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe template is usefull

Restructure project to follow best practices:
- Types: packages/errors/src/types/index.ts
- Implementation: packages/errors/src/error.ts
- Tests: packages/errors/tests/error.test.ts
- Public API: packages/errors/src/index.ts
Tests now in separate 'tests' directory with its own vitest include pattern.
ESLint config updated to ignore vars starting with underscore.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment threadpackages/errors/src/index.ts Outdated
/** Injected context data */
context: Record<string, unknown> | null;
/** HTTP status code (null if not defined) */
httpStatus: number | null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, remove

Comment threadpackages/errors/src/index.ts Outdated
/** Message template with {field} placeholders */
message?: string;
/** HTTP status code */
httpStatus?: number;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove pls

Comment threadpackages/errors/src/index.ts Outdated
Comment on lines +34 to +35
template?: string;
httpStatus?: number;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe template is usefull

Comment on lines +134 to +140
config: {
name: string;
fields?: StandardSchemaV1;
inherits?: ErrorFactory | ErrorFactory[];
message?: string;
httpStatus?: number;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should have it's own type

Comment on lines +186 to +202
if ( inherits !== undefined ) {
( ErrorFactoryInstance as ErrorFactory<T> ).inherits = inherits;
}

if ( fields !== undefined ) {
( ErrorFactoryInstance as ErrorFactory<T> ).schema = fields;
}

if ( message !== undefined ) {
( ErrorFactoryInstance as ErrorFactory<T> ).template = message;
}

if ( httpStatus !== undefined ) {
( ErrorFactoryInstance as ErrorFactory<T> ).httpStatus = httpStatus;
}

return ErrorFactoryInstance as ErrorFactory<T>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this ?

@martyy-codemartyy-code left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: feat: implement error() factory function

Summary

A solid foundation for the error factory pattern. The implementation is clean and the type safety with generics is well-done. However, there are several issues that need addressing before this can be considered production-ready.


Critical Issues

1. in dependencies (not devDependencies)
The diff shows was added as a regular , not . This is unusual and will bloat the installed package size for consumers. TypeScript type packages should never be runtime dependencies.

Fix:

"devDependencies": {
"@types/node": "^25.9.1",
...
}

**2. Type/Implementation Mismatch: , , , **
The interface includes , , , and , with JSDoc referencing methods like and . However, these are never initialized with usable values (always empty arrays, null) and no methods exist to populate them. This creates an incomplete API that doesn't match the documented intent.

3. Template skips formatting when no fields provided (line 152)
The condition means calling with an empty string will skip formatting entirely. An empty string is a valid field value that should still trigger template substitution.


Suggestion Issues

**4. Inconsistent underscore prefix on **
Factory has (no underscore, line 31-32 of types) but ErrorInstance has (with underscore, line 56). This inconsistency suggests different naming conventions for what should be the same concept. Consider either:

  • Both with underscore: on factory, on instance
  • Neither with underscore: on both

5. is V8-specific
The implementation assumes V8 stack format with comments acknowledging "V8 engines provide Error.stack". While this works in Node.js and modern browsers, it will break in Deno or other JavaScript runtimes. Consider adding a fallback or at least documenting this limitation.

6. Stack cleaning uses fragile string matching
Filtering frames via could miss bundled versions of the library or paths with different casing. Consider a more robust approach using regex patterns or URL normalization.

7. Missing method
The PR description mentions "ErrorInstance with all required properties" but a common pattern for error factories is a method that actually throws the error. This is missing and may be expected by users.


DX Positives

praise: The factory function pattern with generics () is excellent TypeScript usage
praise: Multiple inheritance via array is a thoughtful design choice
praise: Message template modifiers (, , ) are a nice DX touch
praise: Test coverage is comprehensive with 36 tests
praise: The interface is clean and correctly structured


Questions

  1. Are , , and context injection planned for future PRs? The types suggest they are, but they're not implemented.
  2. Should the factory itself extend so checks work?
  3. Is the V8-only stack trace intentional for the initial release?

@martyy-codemartyy-code left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: feat: implement error() factory function

Summary

A solid foundation for the error factory pattern. The implementation is clean and the type safety with generics is well-done. However, there are several issues that need addressing before this can be considered production-ready.


Critical Issues

1. @types/node in dependencies (not devDependencies)
The package.json diff shows @types/node was added as a regular dependencies, not devDependencies. This is unusual and will bloat the installed package size for consumers. TypeScript type packages should never be runtime dependencies.

2. Type/Implementation Mismatch: notes, cause, causes, context
The ErrorInstance interface includes notes: string[], cause: Error | null, causes: Error[], and context: Record<string, unknown> | null, with JSDoc referencing methods like .addNote() and .from(). However, these are never initialized with usable values (always empty arrays, null) and no methods exist to populate them. This creates an incomplete API that doesn't match the documented intent.

3. Template skips formatting when no fields provided (line 152)
The condition Object.keys( fieldsData ).length > 0 means calling ValidationError({ field: '' }) with an empty string will skip formatting entirely. An empty string is a valid field value that should still trigger template substitution.


Suggestion Issues

4. Inconsistent underscore prefix on _inherits
Factory has inherits (no underscore, line 31-32 of types) but ErrorInstance has _inherits (with underscore, line 56). This inconsistency suggests different naming conventions for what should be the same concept.

5. captureStack is V8-specific
The implementation assumes V8 stack format with comments acknowledging "V8 engines provide Error.stack". While this works in Node.js and modern browsers, it will break in Deno or other JavaScript runtimes. Consider adding a fallback or at least documenting this limitation.

6. Stack cleaning uses fragile string matching
Filtering frames via !line.includes( 'node_modules/@deessejs' ) could miss bundled versions of the library or paths with different casing. Consider a more robust approach using regex patterns or URL normalization.

7. Missing throw() method
The PR description mentions "ErrorInstance with all required properties" but a common pattern for error factories is a .throw() method that actually throws the error. This is missing and may be expected by users.


DX Positives

praise: The factory function pattern with generics (const T) is excellent TypeScript usage
praise: Multiple inheritance via array is a thoughtful design choice
praise: Message template modifiers (:upper, :lower, :json) are a nice DX touch
praise: Test coverage is comprehensive with 36 tests
praise: The ErrorFactory interface is clean and correctly structured


Questions

  1. Are .addNote(), .from(), and context injection planned for future PRs? The types suggest they are, but they're not implemented.
  2. Should the factory itself extend Error so instanceof checks work?
  3. Is the V8-only stack trace intentional for the initial release?

@martyy-codemartyy-code left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: Line 152 in error.ts has a logic bug in template formatting.

The condition:

if(message&&Object.keys(fieldsData).length>0){

This means calling an error factory with an empty string value will skip template formatting:

constValidationError=error({name: 'V',message: 'Field "{field}" is invalid'});ValidationError({field: ''});// message stays as 'Field "{field}" is invalid'

An empty string is a valid field value. The fix should check for the existence of the key, not the truthiness of the value:

consthasFields=Object.keys(fieldsData).length>0||Object.values(fieldsData).some(v=>v!==undefined);if(message&&hasFields){

Or alternatively, pass a special marker when you want raw template output.

@martyy-codemartyy-code left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: @types/node should be in devDependencies, not dependencies.

In package.json, @types/node is listed under dependencies. This is incorrect because:

  • @types/node provides TypeScript type definitions, not runtime code
  • It should only be used during development (compilation)
  • Including it as a runtime dependency unnecessarily increases installation size for all consumers

Move it to devDependencies:

"devDependencies": {
"@types/node": "^25.9.1",
...
}

@martyy-codemartyy-code left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The ErrorInstance type defines notes, cause, causes, and context properties but no way to populate them.

The types show:

  • notes: string[] (with JSDoc referencing .addNote())
  • cause: Error | null (with JSDoc referencing .from())
  • causes: Error[]
  • context: Record<string, unknown> | null

But the implementation only ever sets:

  • notes: [] (always empty array)
  • cause: null (never set)
  • causes: [] (always empty array)
  • context: null (never set)

This creates an API surface that promises functionality that doesn't exist. Options:

  1. Implement the missing methods (.addNote(), .from()) in this PR
  2. Remove these properties from the interface until they're implemented
  3. Add TODO comments explicitly marking these as unimplemented

The current state is misleading to consumers who will see these properties exist but can't actually use them.

martyy-codeand others added 13 commits May 29, 2026 09:04
Critical fixes:
- Move @types/node to devDependencies (was incorrectly in dependencies)
- Fix template formatting: now checks for placeholder existence, not field values
- Fix regex reuse bug that caused :upper/:lower/:json modifiers to fail
Refactoring:
- Rename _inherits to inherits for consistency with ErrorFactory
- Add TODO comments for unimplemented methods (addNote, from, context)
- Add hasTemplatePlaceholders() helper to avoid regex state issues
- Use early returns and inverse ifs to reduce indentation
Tests:
- Update tests to use new 'inherits' property name
- Add test for template formatting with no fields
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Convert all function declarations to arrow functions with const
- Convert all interface declarations to type aliases
- Re-create src/index.ts which was missing
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
httpStatus will be handled differently in a future implementation.
Removing from:
- Types (ErrorFactory, ErrorInstance, ErrorConfig)
- Implementation (error function)
- Tests (removed httpStatus test suite)
- Test assertions (removed httpStatus checks)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
New structure:
- src/error/error.ts - error factory implementation
- src/error/types/index.ts - type definitions
- src/index.ts - public API exports
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- src/error/types.ts (not in subfolder)
- Updated imports accordingly
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- src/error/capture.ts - stack capture utility
- src/error/error.ts - imports capture from capture.js
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- src/error/constants.ts - shared regex patterns
- src/error/format.ts - message template formatting
- src/error/capture.ts - now imports from constants
- src/error/error.ts - main error factory
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
on ErrorFactory for clarity
Co-Authored-By: Claude Opus 4.[^1]
<noreply@anthropic.com>
Renamed fields to data and added proper generic constraints
to formatTemplate for better type safety following senior
TypeScript patterns.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Use TEMPLATE_PLACEHOLDER_REGEX from constants instead of inline regex.
Also use proper generic key access with data[fieldName as keyof T].
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add ExtractKeys type that extracts placeholder keys from template string
at compile-time. Data objects now validated against actual template
keys, catching missing properties before runtime.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Keep string-based stack filtering approach for cross-environment
compatiblity. Memory updated with stack capture patterns from
Senior to Expert level.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@martyy-code
martyy-code changed the base branch from main to devMay 29, 2026 07:56
@codewizdave
codewizdave merged commit eea65ac into devMay 29, 2026
4 checks passed
martyy-code added a commit that referenced this pull request Aug 3, 2026
Replaces the existing release.yml with the version described in
docs/internal/engineering/plans/release-system.md (Section 3):
- Explicit 'has_changesets' detection step. All publish steps are
gated on this. A 'version bump' PR with no changesets is a no-op.
- Tag is pushed at the version bump commit, not at the merge commit.
Fixes the @deessejs/errors@1.1.1 tag drift.
- pnpm install --frozen-lockfile (was pnpm install) for reproducibility.
- Adds dry_run and packages inputs to workflow_dispatch for tabletop
exercises and selective re-publishes.
- Keeps the existing 'version bump' label gate on PRs to main.
- Adds a changeset to pass the new ci.yml lint.
This is the second layer of the release system plan implementation
stack: lint CI (PR #1) first, workflow rewrite (this commit) second,
documentation update (next) third.
Sign up for freeto 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.

2 participants

@martyy-code@codewizdave