Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove request library #275

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Remove request library #275

Show file tree
Hide file tree
Changes from 8 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 Feb 13, 2020
c754134
sendToCoveralls: use local request instead of npm lib
jaeh Feb 13, 2020
d59343f
update tests for local request
jaeh Feb 13, 2020
35ebed5
pkg: remove request dependency
jaeh Feb 13, 2020
3250b10
eql to equal
jaeh Feb 13, 2020
32968ba
use application/json instead of x-www-form-urlencoded, doh.
jaeh Feb 13, 2020
dca99b9
sendToCoveralls.js: send json object
jaeh Feb 13, 2020
5b6551e
http adds json for us
jaeh Feb 13, 2020
1a98bec
request data gets encodeURIComponent encoded
jaeh Mar 10, 2020
fff6024
request: body of response is an array now, unicode character boundaries.
jaeh Mar 11, 2020
2d35226
make sure chunks in body array are buffers for Buffer.concat.
jaeh Mar 14, 2020
443621e
rename http to https to remove ambiguity
jaeh Mar 14, 2020
8572f74
add early bailout errors if url or data are undefined or not strings
jaeh Mar 14, 2020
9d8fc07
add nock to dev dependencies
jaeh Mar 14, 2020
0f10de9
tests: request.post, only happy path and obvious errors
jaeh Mar 14, 2020
108d01a
Merge pull request #1 from jaeh/remove-request
jaeh Mar 23, 2020
aefbf19
merge upstream
jaeh Mar 23, 2020
66bf91a
deps: update
jaeh Mar 23, 2020
cfafe62
git is empty if localGit.branch is not set
jaeh Apr 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions lib/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';

const http = require('https');

const post = (url, data, cb) => {
const options = {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'Content-Length': Buffer.byteLength(data),
},
};

let errored = false;

const req = http.request(url, options, (res) => {
res.setEncoding('utf8');

const { statusCode } = res;

let body = '';
res.on('data', (chunk) => {
body += chunk.toString();
});

res.on('end', () => {
if (!errored) {
cb(null, { statusCode }, body);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is brittle, because you'll get mochibake if any character boundaries get split. I'd recommend doing it this way instead:

const body = []
res.on('data', chunk => body.push(chunk))
res.on('end', () => {
  if (!errored) {
    cb(null, { statusCode }, Buffer.concat(body).toString())
  }
})

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! i read about this years ago and forgot again, now i will remember it as the pattern to use.

}
});
Copy link

Choose a reason for hiding this comment

The 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)

Copy link

Choose a reason for hiding this comment

The 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(data);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, the request was POST-ing as a urlencoded form, but here, you're just posting the JSON data raw without any prefix.

So, eg, if the object was {foo: 'bar'}, then previously, the post body would've been json=%7B%22foo%22%3A%22bar%22%7D, but now it's {"foo":"bar"}.

That's almost certainly why it's failing.

Try something like this:

req.write(`json=${encodeURIComponent(data)}`)

req.end();
};

module.exports = {
post,
};
11 changes: 2 additions & 9 deletions lib/sendToCoveralls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

const request = require('request');
const request = require('./request');
const index = require('..');

const sendToCoveralls = (obj, cb) => {
Expand All @@ -16,14 +16,7 @@ const sendToCoveralls = (obj, cb) => {
process.stdout.write(str);
cb(null, { statusCode: 200 }, '');
} else {
request.post({
url,
form: {
json: str
}
}, (err, response, body) => {
cb(err, response, body);
});
request.post(url, str, cb);
}
};

Expand Down
Loading