Skip to content

Commit 4c9d754

Browse files
authored
fix: do not copy over stack from error cause (#4784)
I've authored #4485 long time ago. Since that PR sat there for a while, I didn't get a chance to amend the design to not copy over the stack from the `cause`. Copying over the stack will just double up the same stack trace twice in the reporting tooling / console
1 parent a9a340f commit 4c9d754

2 files changed

Lines changed: 28 additions & 14 deletions

File tree

src/error/GraphQLError.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -167,10 +167,8 @@ export class GraphQLError extends Error {
167167

168168
this.name = 'GraphQLError';
169169
this.path = path ?? undefined;
170-
const underlyingError:
171-
| (Error & { readonly extensions?: unknown })
172-
| undefined =
173-
originalError ?? (errorCause instanceof Error ? errorCause : undefined);
170+
const underlyingError: typeof originalError =
171+
originalError ?? (cause instanceof Error ? cause : undefined);
174172
this.originalError = underlyingError;
175173

176174
// Compute list of blame nodes.
@@ -214,9 +212,13 @@ export class GraphQLError extends Error {
214212
});
215213

216214
// Include (non-enumerable) stack trace.
217-
if (underlyingError?.stack != null) {
215+
// Do not copy over the stack trace of the Error.cause, since the tooling
216+
// already nicely prints/reports the cause chains.
217+
// Preserve the copy-over behavior of the `originalError`, since users may
218+
// expect it to work the way it did originally.
219+
if (originalError?.stack != null) {
218220
Object.defineProperty(this, 'stack', {
219-
value: underlyingError.stack,
221+
value: originalError.stack,
220222
writable: true,
221223
configurable: true,
222224
});

src/error/__tests__/GraphQLError-test.ts

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { describe, it } from 'node:test';
2+
import { inspect as nodeInspect } from 'node:util';
23

34
import { assert, expect } from 'chai';
45

@@ -63,18 +64,29 @@ describe('GraphQLError', () => {
6364
]);
6465
});
6566

66-
it('uses the stack of cause when possible', () => {
67-
const original = new Error('original');
67+
it('does not copy over the stack of cause', () => {
68+
function createOriginalError(): Error {
69+
return new Error('original');
70+
}
71+
const original = createOriginalError();
6872
const e = new GraphQLError('msg', {
6973
cause: original,
7074
});
75+
const originalStackFrame = original.stack
76+
?.split('\n')
77+
.find((line) => line.includes('createOriginalError'));
78+
assert(originalStackFrame != null);
7179

7280
expect(e).to.include({
7381
name: 'GraphQLError',
7482
message: 'msg',
75-
stack: original.stack,
7683
cause: original,
7784
});
85+
expect(e.stack).to.not.equal(original.stack);
86+
87+
const inspectedError = nodeInspect(e);
88+
expect(inspectedError).to.include('[cause]: Error: original');
89+
expect(inspectedError).to.include(originalStackFrame.trim());
7890
});
7991

8092
it('uses the stack of an original error via originalError', () => {
@@ -107,7 +119,6 @@ describe('GraphQLError', () => {
107119
expect(e).to.deep.include({
108120
name: 'GraphQLError',
109121
message: 'msg',
110-
stack: cause.stack,
111122
cause,
112123
originalError: cause,
113124
extensions: { original: 'extensions' },
@@ -139,14 +150,15 @@ describe('GraphQLError', () => {
139150
});
140151

141152
it('creates new stack if cause has no stack', () => {
142-
const original = new Error('original');
143-
delete original.stack;
144-
const e = new GraphQLError('msg', { originalError: original });
153+
const cause = new Error('cause');
154+
delete cause.stack;
155+
const e = new GraphQLError('msg', { cause });
145156

146157
expect(e).to.include({
147158
name: 'GraphQLError',
148159
message: 'msg',
149-
cause: original,
160+
cause,
161+
originalError: cause,
150162
});
151163
expect(e.stack).to.be.a('string');
152164
});

0 commit comments

Comments
 (0)