From 94e5b92de1d7bb3506ac6487581963a327e00d22 Mon Sep 17 00:00:00 2001 From: Richie Bendall Date: Mon, 25 May 2020 23:30:05 +1200 Subject: [PATCH] Remove `timeout` option (#831) --- @types/index.d.ts | 4 +- @types/index.test-d.ts | 1 - README.md | 3 +- docs/CHANGELOG.md | 1 + docs/v3-UPGRADE-GUIDE.md | 21 ++++++++++ src/body.js | 35 +++++----------- src/index.js | 11 +---- src/request.js | 1 - src/response.js | 3 +- test/main.js | 90 ---------------------------------------- 10 files changed, 36 insertions(+), 134 deletions(-) diff --git a/@types/index.d.ts b/@types/index.d.ts index 2f94ad6..f0eb606 100644 --- a/@types/index.d.ts +++ b/@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; arrayBuffer: () => Promise; blob: () => Promise; @@ -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'; diff --git a/@types/index.test-d.ts b/@types/index.test-d.ts index 094a0a8..be9b665 100644 --- a/@types/index.test-d.ts +++ b/@types/index.test-d.ts @@ -34,7 +34,6 @@ async function run() { const request = new Request('http://byjka.com/buka'); expectType(request.url); expectType(request.headers); - expectType(request.timeout); const headers = new Headers({byaka: 'buke'}); expectType<(a: string, b: string) => void>(headers.append); diff --git a/README.md b/README.md index 575b169..fdd43aa 100644 --- a/README.md +++ b/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) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f3dc62a..157b0ca 100644 --- a/docs/CHANGELOG.md +++ b/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). diff --git a/docs/v3-UPGRADE-GUIDE.md b/docs/v3-UPGRADE-GUIDE.md index 6a31554..0cc541a 100644 --- a/docs/v3-UPGRADE-GUIDE.md +++ b/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. diff --git a/src/body.js b/src/body.js index a0f37ac..5a04bfc 100644 --- a/src/body.js +++ b/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 diff --git a/src/index.js b/src/index.js index 8b1913e..71c502c 100644 --- a/src/index.js +++ b/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 }; diff --git a/src/request.js b/src/request.js index 4a2677e..fc23e14 100644 --- a/src/request.js +++ b/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 }); diff --git a/src/response.js b/src/response.js index c1ee8a0..3c69d5e 100644 --- a/src/response.js +++ b/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 }); } diff --git a/test/main.js b/test/main.js index aaffc54..750f96a 100644 --- a/test/main.js +++ b/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;