-
Notifications
You must be signed in to change notification settings - Fork 258
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
Remove request library #275
Open
jaeh
wants to merge
19
commits into
nickmerwin:master
Choose a base branch
from
jaeh:remove-request
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
3f6f131
add request.js to replace request lib
jaeh c754134
sendToCoveralls: use local request instead of npm lib
jaeh d59343f
update tests for local request
jaeh 35ebed5
pkg: remove request dependency
jaeh 3250b10
eql to equal
jaeh 32968ba
use application/json instead of x-www-form-urlencoded, doh.
jaeh dca99b9
sendToCoveralls.js: send json object
jaeh 5b6551e
http adds json for us
jaeh 1a98bec
request data gets encodeURIComponent encoded
jaeh fff6024
request: body of response is an array now, unicode character boundaries.
jaeh 2d35226
make sure chunks in body array are buffers for Buffer.concat.
jaeh 443621e
rename http to https to remove ambiguity
jaeh 8572f74
add early bailout errors if url or data are undefined or not strings
jaeh 9d8fc07
add nock to dev dependencies
jaeh 0f10de9
tests: request.post, only happy path and obvious errors
jaeh 108d01a
Merge pull request #1 from jaeh/remove-request
jaeh aefbf19
merge upstream
jaeh 66bf91a
deps: update
jaeh cfafe62
git is empty if localGit.branch is not set
jaeh 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
'use strict'; | ||
|
||
const https = require('https'); | ||
|
||
const post = (url, data, cb) => { | ||
if (!url) { | ||
cb(new Error('request.post(url, data, cb): url must be non-empty.')); | ||
return; | ||
} | ||
|
||
if (typeof url !== 'string') { | ||
cb(new Error('request.post(url, data, cb): url must be a String.')); | ||
return; | ||
} | ||
|
||
if (!data) { | ||
cb(new Error('request.post(url, data, cb): data must be non-empty.')); | ||
return; | ||
} | ||
|
||
if (typeof data !== 'string') { | ||
cb(new Error('request.post(url, data, cb): data must be a String.')); | ||
return; | ||
} | ||
|
||
const options = { | ||
method: 'POST', | ||
headers: { | ||
'Content-Type': 'application/json', | ||
'Content-Length': Buffer.byteLength(data), | ||
}, | ||
}; | ||
|
||
let errored = false; | ||
|
||
const req = https.request(url, options, (res) => { | ||
res.setEncoding('utf8'); | ||
|
||
const { statusCode } = res; | ||
const body = []; | ||
|
||
res.on('data', chunk => { | ||
// Buffer.concat below will complain if the chunks are strings. | ||
// this should only ever happen in the nock https mock, | ||
// but let's make sure. | ||
if (typeof chunk === 'string') { | ||
chunk = Buffer.from(chunk); | ||
} | ||
|
||
body.push(chunk); | ||
}); | ||
|
||
res.on('end', () => { | ||
if (!errored) { | ||
cb(null, { statusCode }, Buffer.concat(body).toString()); | ||
} | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How i would have written it: let body = ''
for await (const chunk of res) {
body += chunk
}
cb(null, { statusCode }, body) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would also take the opportunity to use Promise instead |
||
}); | ||
|
||
req.on('error', (e) => { | ||
errored = true; | ||
cb(e); | ||
}); | ||
|
||
// Write data to request body | ||
req.write(`json=${encodeURIComponent(data)}`); | ||
req.end(); | ||
}; | ||
|
||
module.exports = { | ||
post, | ||
}; |
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
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.
Promise + throw instead? Do think this is kind of verbose