Remove timeout option (node-fetch#831)

Author: Richienb

@types/index.d.ts

@@ -79,7 +79,6 @@ interface RequestInit {
 	port?: number;
 	protocol?: string;
 	size?: number;
-	timeout?: number;
 	highWaterMark?: number;
 }
 
@@ -99,7 +98,6 @@ interface Body {
 	readonly body: NodeJS.ReadableStream | null;
 	readonly bodyUsed: boolean;
 	readonly size: number;
-	readonly timeout: number;
 	buffer: () => Promise<Buffer>;
 	arrayBuffer: () => Promise<ArrayBuffer>;
 	blob: () => Promise<Blob>;
@@ -108,7 +106,7 @@ interface Body {
 }
 declare var Body: {
 	prototype: Body;
-	new (body?: BodyInit, opts?: {size?: number; timeout?: number}): Body;
+	new (body?: BodyInit, opts?: {size?: number}): Body;
 };
 
 type RequestRedirect = 'error' | 'follow' | 'manual';

@types/index.test-d.ts

@@ -34,7 +34,6 @@ async function run() {
 		const request = new Request('http://byjka.com/buka');
 		expectType<string>(request.url);
 		expectType<Headers>(request.headers);
-		expectType<number>(request.timeout);
 
 		const headers = new Headers({byaka: 'buke'});
 		expectType<(a: string, b: string) => void>(headers.append);

README.md

@@ -88,7 +88,7 @@ See Jason Miller's [isomorphic-unfetch](https://www.npmjs.com/package/isomorphic
 - Use native promise, but allow substituting it with [insert your favorite promise library].
 - Use native Node streams for body, on both request and response.
 - Decode content encoding (gzip/deflate) properly, and convert string output (such as `res.text()` and `res.json()`) to UTF-8 automatically.
-- Useful extensions such as timeout, redirect limit, response size limit, [explicit errors][error-handling.md] for troubleshooting.
+- Useful extensions such as redirect limit, response size limit, [explicit errors][error-handling.md] for troubleshooting.
 
 ## Difference from client-side fetch
 
@@ -416,7 +416,6 @@ The default values are shown after each option key.
 
     // The following properties are node-fetch extensions
     follow: 20,             // maximum redirect count. 0 to not follow redirect
-    timeout: 0,             // req/res timeout in ms, it resets on redirect. 0 to disable (OS limit applies). Signal is recommended instead.
     compress: true,         // support gzip/deflate content encoding. false to disable
     size: 0,                // maximum response body size in bytes. 0 to disable
     agent: null,            // http(s).Agent instance or function that returns an instance (see below)

docs/CHANGELOG.md

@@ -8,6 +8,7 @@ Changelog
 **Work in progress!**
 
 - **Breaking:** minimum supported Node.js version is now 10.16.
+- **Breaking:** removed `timeout` option.
 - **Breaking:** revamp TypeScript declarations.
 - Enhance: improve coverage.
 - Enhance: drop Babel (while keeping ESM) (#805).

docs/v3-UPGRADE-GUIDE.md

@@ -23,6 +23,27 @@ other comparatively minor modifications.
 
 Since Node.js will deprecate version 8 at the end of 2019, we decided that node-fetch v3.x will not only drop support for Node.js 4 and 6 (which were supported in v2.x), but also for Node.js 8. We strongly encourage you to upgrade, if you still haven't done so. Check out Node.js' official [LTS plan] for more information on Node.js' support lifetime.
 
+## The `timeout` option was removed.
+
+Since this was never part of the fetch specification, it was removed. AbortSignal offers a more finegrained control of request timeouts, and is standardized in the Fetch spec. For convenience, you can use [timeout-signal](https://github.com/Richienb/timeout-signal) as a workaround:
+
+```js
+const timeoutSignal = require('timeout-signal');
+const fetch = require('node-fetch');
+
+const {AbortError} = fetch
+ 
+fetch('https://www.google.com', { signal: timeoutSignal(5000) })
+    .then(response => {
+        // Handle response
+    })
+    .catch(error => {
+        if (error instanceof AbortError) {
+            // Handle timeout
+        }
+    })
+```
+
 ## `Response.statusText` no longer sets a default message derived from the HTTP status code
 
 If the server didn't respond with status text, node-fetch would set a default message derived from the HTTP status code. This behavior was not spec-compliant and now the `statusText` will remain blank instead.

src/body.js

@@ -25,30 +25,29 @@ const INTERNALS = Symbol('Body internals');
  */
 export default class Body {
 	constructor(body, {
-		size = 0,
-		timeout = 0
+		size = 0
 	} = {}) {
 		if (body === null) {
-		// Body is undefined or null
+			// Body is undefined or null
 			body = null;
 		} else if (isURLSearchParams(body)) {
-		// Body is a URLSearchParams
+			// Body is a URLSearchParams
 			body = Buffer.from(body.toString());
 		} else if (isBlob(body)) {
-		// Body is blob
+			// Body is blob
 		} else if (Buffer.isBuffer(body)) {
-		// Body is Buffer
+			// Body is Buffer
 		} else if (types.isAnyArrayBuffer(body)) {
-		// Body is ArrayBuffer
+			// Body is ArrayBuffer
 			body = Buffer.from(body);
 		} else if (ArrayBuffer.isView(body)) {
-		// Body is ArrayBufferView
+			// Body is ArrayBufferView
 			body = Buffer.from(body.buffer, body.byteOffset, body.byteLength);
 		} else if (body instanceof Stream) {
-		// Body is stream
+			// Body is stream
 		} else {
-		// None of the above
-		// coerce to string then buffer
+			// None of the above
+			// coerce to string then buffer
 			body = Buffer.from(String(body));
 		}
 
@@ -58,7 +57,6 @@ export default class Body {
 			error: null
 		};
 		this.size = size;
-		this.timeout = timeout;
 
 		if (body instanceof Stream) {
 			body.on('error', err => {
@@ -185,18 +183,6 @@ function consumeBody() {
 	let abort = false;
 
 	return new Body.Promise((resolve, reject) => {
-		let resTimeout;
-
-		// Allow timeout on slow response body
-		if (this.timeout) {
-			resTimeout = setTimeout(() => {
-				abort = true;
-				const err = new FetchError(`Response timeout while trying to fetch ${this.url} (over ${this.timeout}ms)`, 'body-timeout');
-				reject(err);
-				body.destroy(err);
-			}, this.timeout);
-		}
-
 		body.on('data', chunk => {
 			if (abort || chunk === null) {
 				return;
@@ -213,7 +199,6 @@ function consumeBody() {
 		});
 
 		finished(body, {writable: false}, err => {
-			clearTimeout(resTimeout);
 			if (err) {
 				if (isAbortError(err)) {
 					// If the request was aborted, reject with this Error

src/index.js

@@ -101,13 +101,6 @@ export default function fetch(url, options_) {
 			}
 		}
 
-		if (request.timeout) {
-			request_.setTimeout(request.timeout, () => {
-				finalize();
-				reject(new FetchError(`network timeout at: ${request.url}`, 'request-timeout'));
-			});
-		}
-
 		request_.on('error', err => {
 			reject(new FetchError(`request to ${request.url} failed, reason: ${err.message}`, 'system', err));
 			finalize();
@@ -168,8 +161,7 @@ export default function fetch(url, options_) {
 							compress: request.compress,
 							method: request.method,
 							body: request.body,
-							signal: request.signal,
-							timeout: request.timeout
+							signal: request.signal
 						};
 
 						// HTTP-redirect fetch step 9
@@ -214,7 +206,6 @@ export default function fetch(url, options_) {
 				statusText: res.statusMessage,
 				headers,
 				size: request.size,
-				timeout: request.timeout,
 				counter: request.counter,
 				highWaterMark: request.highWaterMark
 			};

src/request.js

@@ -93,7 +93,6 @@ export default class Request extends Body {
 				null);
 
 		super(inputBody, {
-			timeout: init.timeout || input.timeout || 0,
 			size: init.size || input.size || 0
 		});
 

src/response.js

@@ -85,8 +85,7 @@ export default class Response extends Body {
 			headers: this.headers,
 			ok: this.ok,
 			redirected: this.redirected,
-			size: this.size,
-			timeout: this.timeout
+			size: this.size
 		});
 	}
 

test/main.js

@@ -1,7 +1,6 @@
 // Test tools
 import zlib from 'zlib';
 import crypto from 'crypto';
-import {spawn} from 'child_process';
 import http from 'http';
 import fs from 'fs';
 import stream from 'stream';
@@ -844,78 +843,6 @@ describe('node-fetch', () => {
 		});
 	});
 
-	it('should allow custom timeout', () => {
-		const url = `${base}timeout`;
-		const options = {
-			timeout: 20
-		};
-		return expect(fetch(url, options)).to.eventually.be.rejected
-			.and.be.an.instanceOf(FetchError)
-			.and.have.property('type', 'request-timeout');
-	});
-
-	it('should allow custom timeout on response body', () => {
-		const url = `${base}slow`;
-		const options = {
-			timeout: 20
-		};
-		return fetch(url, options).then(res => {
-			expect(res.ok).to.be.true;
-			return expect(res.text()).to.eventually.be.rejected
-				.and.be.an.instanceOf(FetchError)
-				.and.have.property('type', 'body-timeout');
-		});
-	});
-
-	it('should not allow socket timeout before body is read', () => {
-		const url = `${base}slow`;
-		const options = {
-			timeout: 100
-		};
-		// Await the response, then delay, allowing enough time for the timeout
-		// to be created just before the socket timeout
-		return fetch(url, options).then(delay(75)).then(res => {
-			expect(res.ok).to.be.true;
-			return expect(res.text()).to.eventually.be.rejected
-				.and.be.an.instanceOf(FetchError)
-				.and.have.property('type', 'body-timeout');
-		});
-	});
-
-	it('should allow custom timeout on redirected requests', () => {
-		const url = `${base}redirect/slow-chain`;
-		const options = {
-			timeout: 20
-		};
-		return expect(fetch(url, options)).to.eventually.be.rejected
-			.and.be.an.instanceOf(FetchError)
-			.and.have.property('type', 'request-timeout');
-	});
-
-	it('should clear internal timeout on fetch response', function (done) {
-		this.timeout(2000);
-		spawn('node', ['-e', `require(’./’)(’${base}hello’, { timeout: 10000 })`])
-			.on('exit', () => {
-				done();
-			});
-	});
-
-	it('should clear internal timeout on fetch redirect', function (done) {
-		this.timeout(2000);
-		spawn('node', ['-e', `require(’./’)(’${base}redirect/301’, { timeout: 10000 })`])
-			.on('exit', () => {
-				done();
-			});
-	});
-
-	it('should clear internal timeout on fetch error', function (done) {
-		this.timeout(2000);
-		spawn('node', ['-e', `require(’./’)(’${base}error/reset’, { timeout: 10000 })`])
-			.on('exit', () => {
-				done();
-			});
-	});
-
 	it('should support request cancellation with signal', function () {
 		this.timeout(500);
 		const controller = new AbortController();
@@ -967,23 +894,6 @@ describe('node-fetch', () => {
 			});
 	});
 
-	it('should clear internal timeout when request is cancelled with an AbortSignal', function (done) {
-		this.timeout(2000);
-		const script = `
-			var AbortController = require(’abortcontroller-polyfill/dist/cjs-ponyfill’).AbortController;
-			var controller = new AbortController();
-			require(’./’)(
-				’${base}timeout’,
-				{ signal: controller.signal, timeout: 10000 }
-			);
-			setTimeout(function () { controller.abort(); }, 20);
-		`;
-		spawn('node', ['-e', script])
-			.on('exit', () => {
-				done();
-			});
-	});
-
 	it('should remove internal AbortSignal event listener after request is aborted', () => {
 		const controller = new AbortController();
 		const {signal} = controller;