Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support serializing primitives within Error causes (#158) #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: Support serializing primitives within Error causes (#158) #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions lib/err-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@ const getErrorCause = (err) => {
? causeResult
: undefined
} else {
return isErrorLike(cause)
? cause
: undefined
return cause
}
}

Expand All @@ -44,8 +42,6 @@ const getErrorCause = (err) => {
* @returns {string}
*/
const _stackWithCauses = (err, seen) => {
if (!isErrorLike(err)) return ''

const stack = err.stack || ''

// Ensure we don't go circular or crazily deep
Expand All @@ -56,8 +52,12 @@ const _stackWithCauses = (err, seen) => {
const cause = getErrorCause(err)

if (cause) {
seen.add(err)
return (stack + '\ncaused by: ' + _stackWithCauses(cause, seen))
if (isErrorLike(cause)) {
seen.add(err)
return (stack + '\ncaused by: ' + _stackWithCauses(cause, seen))
} else {
return (stack + '\ncaused by: ' + String(cause))
}
} else {
return stack
}
Expand All @@ -79,8 +79,6 @@ const stackWithCauses = (err) => _stackWithCauses(err, new Set())
* @returns {string}
*/
const _messageWithCauses = (err, seen, skip) => {
if (!isErrorLike(err)) return ''

const message = skip ? '' : (err.message || '')

// Ensure we don't go circular or crazily deep
Expand All @@ -91,14 +89,18 @@ const _messageWithCauses = (err, seen, skip) => {
const cause = getErrorCause(err)

if (cause) {
seen.add(err)

// @ts-ignore
const skipIfVErrorStyleCause = typeof err.cause === 'function'

return (message +
(skipIfVErrorStyleCause ? '' : ': ') +
_messageWithCauses(cause, seen, skipIfVErrorStyleCause))
if (isErrorLike(cause)) {
seen.add(err)

return (message +
(skipIfVErrorStyleCause ? '' : ': ') +
_messageWithCauses(cause, seen, skipIfVErrorStyleCause))
} else {
return (message + (skipIfVErrorStyleCause ? '' : ': ') + String(cause))
}
} else {
return message
}
Expand Down
2 changes: 1 addition & 1 deletion lib/err-with-cause.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ function errWithCauseSerializer (err) {
_err.aggregateErrors = err.errors.map(err => errWithCauseSerializer(err))
}

if (isErrorLike(err.cause) && !Object.prototype.hasOwnProperty.call(err.cause, seen)) {
if (err.cause !== undefined && !Object.prototype.hasOwnProperty.call(err.cause, seen)) {
_err.cause = errWithCauseSerializer(err.cause)
}

Expand Down
18 changes: 18 additions & 0 deletions test/err-with-cause.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,24 @@ test('keeps non-error cause', () => {
assert.strictEqual(serialized.cause, 'abc')
})

test('keeps non-error cause from constructor', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think this test hints at why supporting non-Error objects is probably not a great idea. What should be done if someone threw?:

{
	cause: {
		foo: 'foo',
		cause: {
			bar: 'bar',
			cause: {
				baz: 'baz'
			}
		}
	}
}

Copy link

@ifeltsweet ifeltsweet Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If cause is not an error then it should be serialized as a non error value would be.

Copy link
Member

Choose a reason for hiding this comment

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

But the point of this PR is to serialize non-error objects as error objects, right?

Copy link
Author

@darcyrush darcyrush Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess an additional test is needed to vizualize the output, but really the point of this PR was to serialize non-error objects and display them as is, not to serialize them as Error objects specifically.

So if an object with nested objects was passed as a cause, regardless if its nested properties were named cause or something else, the entire object and its nested properties and values would be output.

for (const cause of [
'string',
42,
['an', 'array'],
['a', ['nested', 'array']],
{ an: 'object' },
{ a: { nested: 'object' } },
Symbol('symbol')
]) {
const err = Error('foo', { cause })
const serialized = serializer(err)
assert.strictEqual(serialized.type, 'Error')
assert.strictEqual(serialized.message, 'foo')
assert.strictEqual(serialized.cause, cause)
}
})

test('prevents infinite recursion', () => {
const err = Error('foo')
err.inner = err
Expand Down
21 changes: 20 additions & 1 deletion test/err.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,25 @@ test('serializes error causes', () => {
}
})

test('serializes non Error error cause from constructor', () => {
for (const cause of [
'string',
42,
// ['an', 'array'],
// ['a', ['nested', 'array']],
// { an: 'object' },
// { a: { nested: 'object' } },
Comment on lines +69 to +72
Copy link
Member

Choose a reason for hiding this comment

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

Are these commented out because String({ an: 'object' }) returns [object Object]?

Copy link
Author

Choose a reason for hiding this comment

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

Originally, yes. I commented about a WIP approach I have locally in this comment.

Symbol('symbol')
]) {
const err = Error('foo', { cause })
const serialized = serializer(err)
assert.strictEqual(serialized.type, 'Error')
assert.strictEqual(serialized.message, 'foo: ' + String(cause))
assert.match(serialized.stack, /err\.test\.js:/)
assert.match(serialized.stack, /Error: foo/)
}
})

test('serializes error causes with VError support', function (t) {
// Fake VError-style setup
const err = Error('foo: bar')
Expand All @@ -85,7 +104,7 @@ test('keeps non-error cause', () => {
err.cause = 'abc'
const serialized = serializer(err)
assert.strictEqual(serialized.type, 'Error')
assert.strictEqual(serialized.message, 'foo')
assert.strictEqual(serialized.message, 'foo: abc')
assert.strictEqual(serialized.cause, 'abc')
})

Expand Down
Loading