Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the safe() method to accept both Promise objects and functions that return values or promises, making it more ergonomic for error handling in both synchronous and asynchronous contexts.
Key Changes
- Added function overload to
safe()that accepts() => T | Promise<T>in addition to the existingPromise<T>signature - Updated implementations to detect function inputs and wrap them in Promises before error handling
- Added test cases for synchronous function throws
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/errata/src/errata.ts | Updated safe type definition to support both function and promise inputs; modified implementation to handle function inputs by wrapping them in a Promise |
| packages/errata/src/client.ts | Updated safe type definition to support both function and promise inputs; modified implementation to handle function inputs; minor comment update |
| packages/errata/test/errata.test.ts | Added test case to verify synchronous throws from function inputs are properly captured and converted to ErrataError |
| packages/errata/test/client.test.ts | Added test case to verify synchronous throws from function inputs are properly captured and converted to ErrataClientError |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Promise helper returning a tuple without try/catch at call sites. */ | ||
| const safe = async <T>( | ||
| promise: Promise<T>, | ||
| /** Promise/helper returning a tuple without try/catch at call sites. */ |
There was a problem hiding this comment.
The comment has a grammatical issue. "Promise/helper" should be "Promise helper or function" to properly describe that this function accepts both promises and functions.
| /** Promise/helper returning a tuple without try/catch at call sites. */ | |
| /** Promise helper or function returning a tuple without try/catch at call sites. */ |
| it('handles synchronous throw from function input', async () => { | ||
| const [value, err] = await errors.safe(() => { | ||
| throw new Error('boom') | ||
| }) | ||
|
|
||
| expect(value).toBeNull() | ||
| expect(err).toBeInstanceOf(errors.ErrataError) | ||
| expect(err?.code).toBe('errata.unknown_error') | ||
| }) |
There was a problem hiding this comment.
The test coverage for the new function input overload is incomplete. While synchronous throws are tested, there's no test for the case where a function returns a Promise (e.g., an async function or a function that returns Promise.resolve/reject). Consider adding a test case like:
it('handles async function that returns promise', async () => {
const [value, err] = await errors.safe(async () => {
throw new Error('async boom')
})
expect(value).toBeNull()
expect(err?.code).toBe('errata.unknown_error')
})Also consider testing successful async function returns.
| it('captures synchronous throws from function input', async () => { | ||
| const [data, err] = await client.safe(() => { | ||
| throw new Error('boom') | ||
| }) | ||
|
|
||
| expect(data).toBeNull() | ||
| expect(err?.code).toBe('errata.unknown_error') | ||
| }) |
There was a problem hiding this comment.
The test coverage for the new function input overload is incomplete. While synchronous throws are tested, there's no test for the case where a function returns a Promise (e.g., an async function or a function that returns Promise.resolve/reject). Consider adding a test case like:
it('handles async function that returns promise', async () => {
const [data, err] = await client.safe(async () => {
throw new Error('async boom')
})
expect(data).toBeNull()
expect(err?.code).toBe('errata.unknown_error')
})Also consider testing successful async function returns.
No description provided.