-
Notifications
You must be signed in to change notification settings - Fork 764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an HTTP client which uses fetch. #1236
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
01b9ff2
Add an HTTP client which uses fetch.
dcr-stripe 363b79c
Add timeout comment.
dcr-stripe f696014
Add link to node-fetch.
dcr-stripe 1d19a45
Add error when fetch Response#headers is not iterable.
dcr-stripe b0bfd40
Disable class-methods-use-this linting error.
dcr-stripe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
'use strict'; | ||
|
||
const {HttpClient, HttpClientResponse} = require('./HttpClient'); | ||
|
||
/** | ||
* HTTP client which uses a `fetch` function to issue requests. This fetch | ||
* function is expected to be the Web Fetch API function or an equivalent, such | ||
* as the function provided by the node-fetch package (https://github.com/node-fetch/node-fetch). | ||
*/ | ||
class FetchHttpClient extends HttpClient { | ||
constructor(fetchFn) { | ||
super(); | ||
this._fetchFn = fetchFn; | ||
} | ||
|
||
/** @override. */ | ||
getClientName() { | ||
return 'fetch'; | ||
} | ||
|
||
makeRequest( | ||
host, | ||
port, | ||
path, | ||
method, | ||
headers, | ||
requestData, | ||
protocol, | ||
timeout | ||
) { | ||
const isInsecureConnection = protocol === 'http'; | ||
|
||
const url = new URL( | ||
path, | ||
`${isInsecureConnection ? 'http' : 'https'}://${host}` | ||
); | ||
url.port = port; | ||
|
||
const fetchPromise = this._fetchFn(url.toString(), { | ||
method, | ||
headers, | ||
body: requestData || undefined, | ||
}); | ||
|
||
// The Fetch API does not support passing in a timeout natively, so a | ||
// timeout promise is constructed to race against the fetch and preempt the | ||
// request, simulating a timeout. | ||
// | ||
// This timeout behavior differs from Node: | ||
// - Fetch uses a single timeout for the entire length of the request. | ||
// - Node is more fine-grained and resets the timeout after each stage of | ||
// the request. | ||
// | ||
// As an example, if the timeout is set to 30s and the connection takes 20s | ||
// to be established followed by 20s for the body, Fetch would timeout but | ||
// Node would not. The more fine-grained timeout cannot be implemented with | ||
// fetch. | ||
let pendingTimeoutId; | ||
const timeoutPromise = new Promise((_, reject) => { | ||
pendingTimeoutId = setTimeout(() => { | ||
pendingTimeoutId = null; | ||
reject(HttpClient.makeTimeoutError()); | ||
}, timeout); | ||
}); | ||
|
||
return Promise.race([fetchPromise, timeoutPromise]) | ||
.then((res) => { | ||
return new FetchHttpClientResponse(res); | ||
}) | ||
.finally(() => { | ||
if (pendingTimeoutId) { | ||
clearTimeout(pendingTimeoutId); | ||
} | ||
}); | ||
} | ||
} | ||
|
||
class FetchHttpClientResponse extends HttpClientResponse { | ||
constructor(res) { | ||
super( | ||
res.status, | ||
FetchHttpClientResponse._transformHeadersToObject(res.headers) | ||
); | ||
this._res = res; | ||
} | ||
|
||
getRawResponse() { | ||
return this._res; | ||
} | ||
|
||
toStream(streamCompleteCallback) { | ||
// Unfortunately `fetch` does not have event handlers for when the stream is | ||
// completely read. We therefore invoke the streamCompleteCallback right | ||
// away. This callback emits a response event with metadata and completes | ||
// metrics, so it's ok to do this without waiting for the stream to be | ||
// completely read. | ||
streamCompleteCallback(); | ||
|
||
// Fetch's `body` property is expected to be a readable stream of the body. | ||
return this._res.body; | ||
} | ||
|
||
toJSON() { | ||
return this._res.json(); | ||
} | ||
|
||
static _transformHeadersToObject(headers) { | ||
// Fetch uses a Headers instance so this must be converted to a barebones | ||
// JS object to meet the HttpClient interface. | ||
const headersObj = {}; | ||
|
||
for (const entry of headers) { | ||
if (!Array.isArray(entry) || entry.length != 2) { | ||
throw new Error( | ||
'Response objects produced by the fetch function given to FetchHttpClient do not have an iterable headers map. Response#headers should be an iterable object.' | ||
); | ||
} | ||
|
||
headersObj[entry[0]] = entry[1]; | ||
} | ||
|
||
return headersObj; | ||
} | ||
} | ||
|
||
module.exports = {FetchHttpClient, FetchHttpClientResponse}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
'use strict'; | ||
|
||
const expect = require('chai').expect; | ||
const fetch = require('node-fetch'); | ||
const {Readable} = require('stream'); | ||
const {FetchHttpClient} = require('../../lib/net/FetchHttpClient'); | ||
|
||
const createFetchHttpClient = () => { | ||
return new FetchHttpClient(fetch); | ||
}; | ||
|
||
const {createHttpClientTestSuite, ArrayReadable} = require('./helpers'); | ||
|
||
createHttpClientTestSuite( | ||
'FetchHttpClient', | ||
createFetchHttpClient, | ||
(setupNock, sendRequest) => { | ||
describe('raw stream', () => { | ||
it('getRawResponse()', async () => { | ||
setupNock().reply(200); | ||
const response = await sendRequest(); | ||
expect(response.getRawResponse()).to.be.an.instanceOf(fetch.Response); | ||
}); | ||
|
||
it('toStream returns the body as a stream', async () => { | ||
setupNock().reply(200, () => new ArrayReadable(['hello, world!'])); | ||
|
||
const response = await sendRequest(); | ||
|
||
return new Promise((resolve) => { | ||
const stream = response.toStream(() => true); | ||
|
||
// node-fetch returns a Node Readable here. In a Web API context, this | ||
// would be a Web ReadableStream. | ||
expect(stream).to.be.an.instanceOf(Readable); | ||
|
||
let streamedContent = ''; | ||
stream.on('data', (chunk) => { | ||
streamedContent += chunk; | ||
}); | ||
stream.on('end', () => { | ||
expect(streamedContent).to.equal('hello, world!'); | ||
resolve(); | ||
}); | ||
}); | ||
}); | ||
|
||
it('toStream invokes the streamCompleteCallback', async () => { | ||
setupNock().reply(200, () => new ArrayReadable(['hello, world!'])); | ||
|
||
const response = await sendRequest(); | ||
|
||
return new Promise((resolve) => { | ||
response.toStream(() => { | ||
resolve(); | ||
}); | ||
}); | ||
}); | ||
}); | ||
} | ||
); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT of throwing an exception here if
entry
is not an array with two entries?I think maybe it's worth being more defensive here because
fetchFn
is injectedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Done.