Repository: sindresorhus/got (ts)

Issue:
Wrapping `RequestError` obscures its source
#### What would you like to discuss?

When I’m working on and with nock, I often see stack traces that look like this:

```
OverriddenClientRequest.request.once.error (node_modules/got/source/request-as-event-emitter.js:178:14)
OverriddenClientRequest.origin.emit.args (node_modules/@szmarczak/http-timer/source/index.js:37:11)
lib/request_overrider.js:24:552
```

Or sometimes:

```
OverriddenClientRequest.request.once.error (/Users/pnm/code/nock/node_modules/got/source/request-as-event-emitter.js:178:14)
OverriddenClientRequest.emit (events.js:187:15)
OverriddenClientRequest.EventEmitter.emit (domain.js:442:20)
OverriddenClientRequest.origin.emit.args (/Users/pnm/code/nock/node_modules/@szmarczak/http-timer/source/index.js:37:11)
/Users/pnm/code/nock/lib/request_overrider.js:24:552
process._tickCallback (internal/process/next_tick.js:61:11)
```

The underlying stack trace is this:

```
end (/Users/pnm/code/nock/lib/request_overrider.js:32:187)
/Users/pnm/code/nock/lib/request_overrider.js:13:2198
OverriddenClientRequest.RequestOverrider.req.write (/Users/pnm/code/nock/lib/request_overrider.js:13:1387)
OverriddenClientRequest.RequestOverrider.req.end (/Users/pnm/code/nock/lib/request_overrider.js:13:1970)
handleRequest (/Users/pnm/code/nock/node_modules/got/source/request-as-event-emitter.js:212:14)
get (/Users/pnm/code/nock/node_modules/got/source/request-as-event-emitter.js:236:5)
Immediate.setImmediate (/Users/pnm/code/nock/node_modules/got/source/request-as-event-emitter.js:307:10)
```

However when got wraps the error in a RequestError, the original stack trace is no longer available.

To be fair, part of what makes this confusing is that the test runner’s output (which has nothing to do with nock or got) and looks like this:

```
 FAIL  tests/test_intercept.js
Nock: No match for request {   "method": "GET",   "url": "http://example.com/",   "headers": {     "user-agent": "got/9.6.0 (https://github.com/sindresorhus/got)",     "accept-encoding": "gzip, deflate"   } }

				} else {
					error = new RequestError(error, options);
-------------^
				}

  stack: >
    OverriddenClientRequest.request.once.error
    (node_modules/got/source/request-as-event-emitter.js:178:14)

    OverriddenClientRequest.origin.emit.args (node_modules/@szmarczak/http-timer/source/index.js:37:11)

    lib/request_overrider.js:24:552
  at:
    line: 178
    column: 14
    file: node_modules/got/source/request-as-event-emitter.js
    function: OverriddenClientRequest.request.once.error
```

For a long time it was unclear that the error and had anything to do with Nock. Nock doesn’t appear in the stack trace despite being the layer that threw the error. And the test runner's output doesn't make it quite so clear to me that “Nock: No match for request” was the message for the error that was being emitted by got.

There is [not much that is documented about RequestError](https://github.com/sindresorhus/got/blob/dfb46ad0bf2427f387968f67ac943476597f0a3b/readme.md#gotrequesterror), though I imagine the benefit of wrapping errors in RequestError is to associate with the thrown error the additional bits information about the request. In this case, however, the wrapping did not provide a benefit. Instead, it made it harder to understand what was happening. 

Some possible ideas for making this easier for developers to understand:

1. Preserve the original `error.stack` and attach it to the new RequestError. This could be done in GotError’s constructor. I experimented with this locally and it seems to work. (Though it is slightly disorienting to see Nock’s stack trace attached to something called RequestError, which is not from nock.)
2. Replace the wrapping with decorating. Instead of creating a new error, attach the got-related `options` to the existing error object.
3. Modify nock so it doesn’t emit this error through the request.

I’d be really curious to hear  your thoughts on (1) and (2)! Regardless of what's decided here, I think (3) could be worth pursuing, as it will make it easier both to test nock and also to use it.

I can’t find a discussion on this specific topic, though there was some discussion about async stack traces in #279 and #447.

Thanks so much for all your work on this amazing library!

#### Checklist

- [x] I have read the documentation.


Pull request:
Preserve stacktrace when wrapping errors
Fixes #795

#### Checklist

- [x] I have read the documentation.
- [x] I have included a pull request description of my changes.
- [x] I have included some tests.
- [ ] ~If it's a new feature, I have included documentation updates.~


Patch:
diff --git a/readme.md b/readme.md
index b182f0c..b9c8c57 100644
--- a/readme.md
+++ b/readme.md
@@ -258,7 +258,7 @@ The promise has `.json()` and `.buffer()` and `.text()` functions which set this
 Example:
 
 ```js
-const {body} = await got(url).json();
+const body = await got(url).json();
 ```
 
 ###### resolveBodyOnly
@@ -1333,7 +1333,7 @@ To receive a JSON body you can either set `responseType` option to `json` or use
 const got = require('got');
 
 (async () => {
-	const {body} = await got.post('https://httpbin.org/anything', {
+	const body = await got.post('https://httpbin.org/anything', {
 		body: {
 			hello: 'world'
 		}
diff --git a/source/errors.ts b/source/errors.ts
index b880772..f624547 100644
--- a/source/errors.ts
+++ b/source/errors.ts
@@ -7,7 +7,7 @@ export class GotError extends Error {
 	code?: string;
 	readonly options: NormalizedOptions;
 
-	constructor(message: string, error: (Error & {code?: string}) | {code?: string}, options: NormalizedOptions) {
+	constructor(message: string, error: Partial<Error & {code?: string}>, options: NormalizedOptions) {
 		super(message);
 		Error.captureStackTrace(this, this.constructor);
 		this.name = 'GotError';
@@ -19,6 +19,20 @@ export class GotError extends Error {
 		Object.defineProperty(this, 'options', {
 			value: options
 		});
+
+		// Recover the original stacktrace
+		if (!is.undefined(error.stack)) {
+			const indexOfMessage = this.stack.indexOf(this.message) + this.message.length;
+			const thisStackTrace = this.stack.slice(indexOfMessage).split('\n').reverse();
+			const errorStackTrace = error.stack.slice(error.stack.indexOf(error.message) + error.message.length).split('\n').reverse();
+
+			// Remove duplicated traces
+			while (errorStackTrace.length !== 0 && errorStackTrace[0] === thisStackTrace[0]) {
+				thisStackTrace.shift();
+			}
+
+			this.stack = `${this.stack.slice(0, indexOfMessage)}${thisStackTrace.reverse().join('\n')}${errorStackTrace.reverse().join('\n')}`;
+		}
 	}
 }
 
@@ -96,7 +110,7 @@ export class TimeoutError extends GotError {
 	event: string;
 
 	constructor(error: TimedOutError, timings: Timings, options: NormalizedOptions) {
-		super(error.message, {code: 'ETIMEDOUT'}, options);
+		super(error.message, error, options);
 		this.name = 'TimeoutError';
 		this.event = error.event;
 		this.timings = timings;
diff --git a/source/request-as-event-emitter.ts b/source/request-as-event-emitter.ts
index 04812f2..c70df27 100644
--- a/source/request-as-event-emitter.ts
+++ b/source/request-as-event-emitter.ts
@@ -14,6 +14,7 @@ import {CacheError, MaxRedirectsError, RequestError, TimeoutError} from './error
 import urlToOptions from './utils/url-to-options';
 import {NormalizedOptions, Response, ResponseObject} from './utils/types';
 
+const setImmediateAsync = () => new Promise(resolve => setImmediate(resolve));
 const pipeline = promisify(stream.pipeline);
 
 const redirectCodes: ReadonlySet<number> = new Set([300, 301, 302, 303, 304, 307, 308]);
@@ -283,7 +284,13 @@ export default (options: NormalizedOptions) => {
 		}
 	};
 
-	setImmediate(async () => {
+	(async () => {
+		// Promises are executed immediately.
+		// If there were no `setImmediate` here,
+		// `promise.json()` would have no effect
+		// as the request would be sent already.
+		await setImmediateAsync();
+
 		try {
 			for (const hook of options.hooks.beforeRequest) {
 				// eslint-disable-next-line no-await-in-loop
@@ -294,7 +301,7 @@ export default (options: NormalizedOptions) => {
 		} catch (error) {
 			emitError(error);
 		}
-	});
+	})();
 
 	return emitter;
 };


Test patch:
diff --git a/test/error.ts b/test/error.ts
index 369a75a..ece3a38 100644
--- a/test/error.ts
+++ b/test/error.ts
@@ -197,3 +197,17 @@ test('errors are thrown directly when options.stream is true', t => {
 		message: 'Parameter `hooks` must be an Object, not boolean'
 	});
 });
+
+test('the old stacktrace is recovered', async t => {
+	const error = await t.throwsAsync(got('https://example.com', {
+		request: () => {
+			throw new Error('foobar');
+		}
+	}));
+
+	t.true(error.stack.includes('at Object.request'));
+
+	// The first `at get` points to where the error was wrapped,
+	// the second `at get` points to the real cause.
+	t.not(error.stack.indexOf('at get'), error.stack.lastIndexOf('at get'));
+});