From 993a0a07502f3072b4a54113d438d968085cb762 Mon Sep 17 00:00:00 2001 From: Konstantin Vyatkin Date: Wed, 27 May 2020 23:38:16 -0400 Subject: [PATCH 1/8] refactor to async --- src/body.js | 73 ++++++++++++++++++++++++---------------------------- src/index.js | 4 +-- test/main.js | 5 ++-- 3 files changed, 38 insertions(+), 44 deletions(-) diff --git a/src/body.js b/src/body.js index f12ee4121..1ef46efcf 100644 --- a/src/body.js +++ b/src/body.js @@ -5,7 +5,7 @@ * Body interface provides common methods for Request and Response */ -import Stream, {finished, PassThrough} from 'stream'; +import Stream, {PassThrough} from 'stream'; import {types} from 'util'; import Blob from 'fetch-blob'; @@ -148,22 +148,22 @@ Object.defineProperties(Body.prototype, { * * @return Promise */ -const consumeBody = data => { +async function consumeBody(data) { if (data[INTERNALS].disturbed) { - return Body.Promise.reject(new TypeError(`body used already for: ${data.url}`)); + throw new TypeError(`body used already for: ${data.url}`); } data[INTERNALS].disturbed = true; if (data[INTERNALS].error) { - return Body.Promise.reject(data[INTERNALS].error); + throw data[INTERNALS].error; } let {body} = data; // Body is null if (body === null) { - return Body.Promise.resolve(Buffer.alloc(0)); + return Buffer.alloc(0); } // Body is blob @@ -173,12 +173,12 @@ const consumeBody = data => { // Body is buffer if (Buffer.isBuffer(body)) { - return Body.Promise.resolve(body); + return body; } /* c8 ignore next 3 */ if (!(body instanceof Stream)) { - return Body.Promise.resolve(Buffer.alloc(0)); + return Buffer.alloc(0); } // Body is stream @@ -187,47 +187,44 @@ const consumeBody = data => { let accumBytes = 0; let abort = false; - return new Body.Promise((resolve, reject) => { - body.on('data', chunk => { + try { + for await (const chunk of body) { if (abort || chunk === null) { return; } if (data.size && accumBytes + chunk.length > data.size) { abort = true; - reject(new FetchError(`content size at ${data.url} over limit: ${data.size}`, 'max-size')); - return; + throw new FetchError(`content size at ${data.url} over limit: ${data.size}`, 'max-size'); } accumBytes += chunk.length; accum.push(chunk); - }); - - finished(body, {writable: false}, err => { - if (err) { - if (isAbortError(err)) { - // If the request was aborted, reject with this Error - abort = true; - reject(err); - } else { - // Other errors, such as incorrect content-encoding - reject(new FetchError(`Invalid response body while trying to fetch ${data.url}: ${err.message}`, 'system', err)); - } - } else { - if (abort) { - return; - } - - try { - resolve(Buffer.concat(accum, accumBytes)); - } catch (error) { - // Handle streams that have accumulated too much data (issue #414) - reject(new FetchError(`Could not create Buffer from response body for ${data.url}: ${error.message}`, 'system', error)); - } + } + } catch (error) { + if (isAbortError(error)) { + // If the request was aborted, reject with this Error + abort = true; + throw error; + } else { + // Other errors, such as incorrect content-encoding + if (error instanceof FetchError) { + throw error; } - }); - }); -}; + + throw new FetchError(`Invalid response body while trying to fetch ${data.url}: ${error.message}`, 'system', error); + } + } + + if (!abort) { + try { + return Buffer.concat(accum, accumBytes); + } catch (error) { + // Handle streams that have accumulated too much data (issue #414) + throw new FetchError(`Could not create Buffer from response body for ${data.url}: ${error.message}`, 'system', error); + } + } +} /** * Clone body given Res/Req instance @@ -370,5 +367,3 @@ export const writeToStream = (dest, {body}) => { } }; -// Expose Promise -Body.Promise = global.Promise; diff --git a/src/index.js b/src/index.js index 804d9a9a7..9da0019f2 100644 --- a/src/index.js +++ b/src/index.js @@ -12,7 +12,7 @@ import zlib from 'zlib'; import Stream, {PassThrough, pipeline as pump} from 'stream'; import dataURIToBuffer from 'data-uri-to-buffer'; -import Body, {writeToStream, getTotalBytes} from './body.js'; +import {writeToStream, getTotalBytes} from './body.js'; import Response from './response.js'; import Headers, {fromRawHeaders} from './headers.js'; import Request, {getNodeRequestOptions} from './request.js'; @@ -51,8 +51,6 @@ export default function fetch(url, options_) { return fetch.Promise.reject(new FetchError(`[${request.method}] ${request.url} invalid URL`, 'system')); } - Body.Promise = fetch.Promise; - // Wrap http.request into fetch return new fetch.Promise((resolve, reject) => { // Build request object diff --git a/test/main.js b/test/main.js index 6ecef13f9..65c7f7ccc 100644 --- a/test/main.js +++ b/test/main.js @@ -601,8 +601,9 @@ describe('node-fetch', () => { return fetch(url).then(res => { expect(res.status).to.equal(200); expect(res.ok).to.be.true; - return expect(res.text()).to.eventually.be.rejectedWith(Error) - .and.have.property('message').includes('Premature close'); + return res.text().then(text => { + expect(text).to.equal('foo'); + }); }); }); From 5feb8172012c594e6c7330231bb359680c884df4 Mon Sep 17 00:00:00 2001 From: Konstantin Vyatkin Date: Thu, 28 May 2020 11:30:18 -0400 Subject: [PATCH 2/8] no custsom promises anymore --- README.md | 9 --------- src/body.js | 13 ++++++++----- src/index.js | 15 ++++----------- test/main.js | 27 +++------------------------ test/utils/server.js | 6 +++--- 5 files changed, 18 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index e264fd387..2fe3b50d7 100644 --- a/README.md +++ b/README.md @@ -116,15 +116,6 @@ const fetch = require('node-fetch'); import fetch from 'node-fetch'; ``` -If you are using a Promise library other than native, set it through `fetch.Promise`: - -```js -const fetch = require('node-fetch'); -const Bluebird = require('bluebird'); - -fetch.Promise = Bluebird; -``` - If you want to patch the global object in node: ```js diff --git a/src/body.js b/src/body.js index 1ef46efcf..9f1a7d363 100644 --- a/src/body.js +++ b/src/body.js @@ -217,11 +217,14 @@ async function consumeBody(data) { } if (!abort) { - try { - return Buffer.concat(accum, accumBytes); - } catch (error) { - // Handle streams that have accumulated too much data (issue #414) - throw new FetchError(`Could not create Buffer from response body for ${data.url}: ${error.message}`, 'system', error); + if (body.readableEnded === true || body._readableState.ended === true) { + try { + return Buffer.concat(accum, accumBytes); + } catch (error) { + throw new FetchError(`Could not create Buffer from response body for ${data.url}: ${error.message}`, 'system', error); + } + } else { + throw new FetchError(`Premature close of server response while trying to fetch ${data.url}`); } } } diff --git a/src/index.js b/src/index.js index 9da0019f2..95f428312 100644 --- a/src/index.js +++ b/src/index.js @@ -29,12 +29,7 @@ export {Headers, Request, Response, FetchError, AbortError, isRedirect}; * @param Object opts Fetch options * @return Promise */ -export default function fetch(url, options_) { - // Allow custom promise - if (!fetch.Promise) { - throw new Error('native promise missing, set fetch.Promise to your favorite alternative'); - } - +export default async function fetch(url, options_) { // Regex for data uri const dataUriRegex = /^\s*data:([a-z]+\/[a-z]+(;[a-z-]+=[a-z-]+)?)?(;base64)?,[\w!$&',()*+;=\-.~:@/?%\s]*\s*$/i; @@ -42,17 +37,17 @@ export default function fetch(url, options_) { if (dataUriRegex.test(url)) { const data = dataURIToBuffer(url); const response = new Response(data, {headers: {'Content-Type': data.type}}); - return fetch.Promise.resolve(response); + return response; } // If invalid data uri if (url.toString().startsWith('data:')) { const request = new Request(url, options_); - return fetch.Promise.reject(new FetchError(`[${request.method}] ${request.url} invalid URL`, 'system')); + throw new FetchError(`[${request.method}] ${request.url} invalid URL`, 'system'); } // Wrap http.request into fetch - return new fetch.Promise((resolve, reject) => { + return new Promise((resolve, reject) => { // Build request object const request = new Request(url, options_); const options = getNodeRequestOptions(request); @@ -288,5 +283,3 @@ export default function fetch(url, options_) { }); } -// Expose Promise -fetch.Promise = global.Promise; diff --git a/test/main.js b/test/main.js index 65c7f7ccc..b1500b89b 100644 --- a/test/main.js +++ b/test/main.js @@ -10,7 +10,6 @@ import chai from 'chai'; import chaiPromised from 'chai-as-promised'; import chaiIterator from 'chai-iterator'; import chaiString from 'chai-string'; -import then from 'promise'; import resumer from 'resumer'; import FormData from 'form-data'; import stringToArrayBuffer from 'string-to-arraybuffer'; @@ -77,29 +76,10 @@ describe('node-fetch', () => { it('should return a promise', () => { const url = `${base}hello`; const p = fetch(url); - expect(p).to.be.an.instanceof(fetch.Promise); + expect(p).to.be.an.instanceof(Promise); expect(p).to.have.property('then'); }); - it('should allow custom promise', () => { - const url = `${base}hello`; - const old = fetch.Promise; - fetch.Promise = then; - expect(fetch(url)).to.be.an.instanceof(then); - expect(fetch(url)).to.not.be.an.instanceof(old); - fetch.Promise = old; - }); - - it('should throw error when no promise implementation are found', () => { - const url = `${base}hello`; - const old = fetch.Promise; - fetch.Promise = undefined; - expect(() => { - fetch(url); - }).to.throw(Error); - fetch.Promise = old; - }); - it('should expose Headers, Response and Request constructors', () => { expect(FetchError).to.equal(FetchErrorOrig); expect(Headers).to.equal(HeadersOrig); @@ -601,9 +581,8 @@ describe('node-fetch', () => { return fetch(url).then(res => { expect(res.status).to.equal(200); expect(res.ok).to.be.true; - return res.text().then(text => { - expect(text).to.equal('foo'); - }); + return expect(res.text()).to.eventually.be.rejectedWith(Error) + .and.have.property('message').includes('Premature close'); }); }); diff --git a/test/utils/server.js b/test/utils/server.js index fbcea48d6..a83768755 100644 --- a/test/utils/server.js +++ b/test/utils/server.js @@ -305,9 +305,9 @@ export default class TestServer { if (p === '/error/premature') { res.writeHead(200, {'content-length': 50}); res.write('foo'); - setTimeout(() => { - res.destroy(); - }, 100); + setImmediate(() => { + res.socket.destroy(); + }); } if (p === '/error/json') { From 94806863a9c80add558279a6833891cd213ab946 Mon Sep 17 00:00:00 2001 From: Konstantin Vyatkin Date: Thu, 28 May 2020 11:33:07 -0400 Subject: [PATCH 3/8] restore server premature handler --- test/utils/server.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/utils/server.js b/test/utils/server.js index a83768755..fbcea48d6 100644 --- a/test/utils/server.js +++ b/test/utils/server.js @@ -305,9 +305,9 @@ export default class TestServer { if (p === '/error/premature') { res.writeHead(200, {'content-length': 50}); res.write('foo'); - setImmediate(() => { - res.socket.destroy(); - }); + setTimeout(() => { + res.destroy(); + }, 100); } if (p === '/error/json') { From f651f0bd2b0e85c833fa6dcea2039f3eb8f76d99 Mon Sep 17 00:00:00 2001 From: Konstantin Vyatkin Date: Thu, 28 May 2020 12:48:36 -0400 Subject: [PATCH 4/8] simplify --- src/body.js | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/src/body.js b/src/body.js index 9f1a7d363..121a49c2a 100644 --- a/src/body.js +++ b/src/body.js @@ -185,16 +185,11 @@ async function consumeBody(data) { // get ready to actually consume the body const accum = []; let accumBytes = 0; - let abort = false; try { for await (const chunk of body) { - if (abort || chunk === null) { - return; - } - - if (data.size && accumBytes + chunk.length > data.size) { - abort = true; + const chunkSize = Buffer.isBuffer(chunk) ? chunk.byteLength : Buffer.from(chunk).byteLength; + if (data.size > 0 && accumBytes + chunkSize > data.size) { throw new FetchError(`content size at ${data.url} over limit: ${data.size}`, 'max-size'); } @@ -202,30 +197,22 @@ async function consumeBody(data) { accum.push(chunk); } } catch (error) { - if (isAbortError(error)) { - // If the request was aborted, reject with this Error - abort = true; + if (isAbortError(error) || error instanceof FetchError) { throw error; } else { // Other errors, such as incorrect content-encoding - if (error instanceof FetchError) { - throw error; - } - throw new FetchError(`Invalid response body while trying to fetch ${data.url}: ${error.message}`, 'system', error); } } - if (!abort) { - if (body.readableEnded === true || body._readableState.ended === true) { - try { - return Buffer.concat(accum, accumBytes); - } catch (error) { - throw new FetchError(`Could not create Buffer from response body for ${data.url}: ${error.message}`, 'system', error); - } - } else { - throw new FetchError(`Premature close of server response while trying to fetch ${data.url}`); + if (body.readableEnded === true || body._readableState.ended === true) { + try { + return Buffer.concat(accum, accumBytes); + } catch (error) { + throw new FetchError(`Could not create Buffer from response body for ${data.url}: ${error.message}`, 'system', error); } + } else { + throw new FetchError(`Premature close of server response while trying to fetch ${data.url}`); } } From 6c566ba98afb3f478af1c1a43d2bb02e69e4c61c Mon Sep 17 00:00:00 2001 From: Konstantin Vyatkin Date: Thu, 28 May 2020 16:06:24 -0400 Subject: [PATCH 5/8] fixing break --- src/body.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/body.js b/src/body.js index 121a49c2a..6951dd5a8 100644 --- a/src/body.js +++ b/src/body.js @@ -188,9 +188,10 @@ async function consumeBody(data) { try { for await (const chunk of body) { - const chunkSize = Buffer.isBuffer(chunk) ? chunk.byteLength : Buffer.from(chunk).byteLength; - if (data.size > 0 && accumBytes + chunkSize > data.size) { - throw new FetchError(`content size at ${data.url} over limit: ${data.size}`, 'max-size'); + if (data.size > 0 && accumBytes + chunk.length > data.size) { + const err = new FetchError(`content size at ${data.url} over limit: ${data.size}`, 'max-size') + body.destroy(err) + throw err; } accumBytes += chunk.length; From 63e310ba6192d56ed7b91bb59df9b9ecee5ce03e Mon Sep 17 00:00:00 2001 From: Konstantin Vyatkin Date: Thu, 28 May 2020 16:08:56 -0400 Subject: [PATCH 6/8] lint --- src/body.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/body.js b/src/body.js index 6951dd5a8..ad1c0402a 100644 --- a/src/body.js +++ b/src/body.js @@ -189,8 +189,8 @@ async function consumeBody(data) { try { for await (const chunk of body) { if (data.size > 0 && accumBytes + chunk.length > data.size) { - const err = new FetchError(`content size at ${data.url} over limit: ${data.size}`, 'max-size') - body.destroy(err) + const err = new FetchError(`content size at ${data.url} over limit: ${data.size}`, 'max-size'); + body.destroy(err); throw err; } From 645c7eeadb9ab5c689b8645aeb5ccf6b13023032 Mon Sep 17 00:00:00 2001 From: Konstantin Vyatkin Date: Thu, 28 May 2020 16:11:34 -0400 Subject: [PATCH 7/8] remove promise dependency --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index b8135bbbe..0f3f8fff6 100644 --- a/package.json +++ b/package.json @@ -60,7 +60,6 @@ "mocha": "^7.1.2", "p-timeout": "^3.2.0", "parted": "^0.1.1", - "promise": "^8.1.0", "resumer": "0.0.0", "rollup": "^2.10.8", "string-to-arraybuffer": "^1.0.2", From 7dcc7805c916f275800a618aa2de66aa277b1f5d Mon Sep 17 00:00:00 2001 From: Konstantin Vyatkin Date: Thu, 28 May 2020 17:16:13 -0400 Subject: [PATCH 8/8] fix docs --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 2fe3b50d7..9026b759b 100644 --- a/README.md +++ b/README.md @@ -85,9 +85,9 @@ See Jason Miller's [isomorphic-unfetch](https://www.npmjs.com/package/isomorphic - Stay consistent with `window.fetch` API. - Make conscious trade-off when following [WHATWG fetch spec][whatwg-fetch] and [stream spec](https://streams.spec.whatwg.org/) implementation details, document known differences. -- Use native promise, but allow substituting it with [insert your favorite promise library]. +- Use native promise and async functions. - 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. +- Decode content encoding (gzip/deflate/brotli) properly, and convert string output (such as `res.text()` and `res.json()`) to UTF-8 automatically. - Useful extensions such as redirect limit, response size limit, [explicit errors][error-handling.md] for troubleshooting. ## Difference from client-side fetch