Remove request library

[jaeh]

please do not merge this without testing,

i have not yet tested this in any of my libraries,
just made the unit tests pass,
will do so in the course of today then repost here.

there also are some function calls and returns that could be made simpler,
i tried to keep the changes minimal for now.

this would close #204, #267 and likely others i did not see.

[jaeh]

this does not work yet, throws 400, something about the content-type i guess, looking at it.

[jaeh]

could someone with deeper insight in the coveralls api please have a look at this?

in my tests it returned 422: failed to parse package.json.

i have a deadline on the 22.2. that i will likely miss, so i can not really investigate this earlier than that.

thanks.

[isaacs]

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())
  }
})

[jaeh]

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

[isaacs]

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)}`)

[isaacs]

Hey, I'd like to see this fixed as soon as possible, since request's deprecation just starting causing tap to raise deprecation notices at install time, because of this dependency.

Added some comments to where I think the patch is going wrong. I'd recommend adding some tests around it, it's pretty ironic for this of all modules to not have full test coverage :)

[jaeh]

thanks! having a look at this now, looks like the obvious reason.

[jaeh]

i assume this confirms that the http request works:

https://coveralls.io/builds/28723334

coverage went down, the request file can be seen in the sources.

will see if i can get some tests done quickly.

[jaeh]

had a look at this and it's a bit more complicated than i would be comfortable to just implement.

have not worked with mocha/sinon for years, the seemingly logical choice to use would be nock as a mock for http.request.

i will revisit tomorrow, it's almost 2 in the morning here anyways.

[jaeh]

added nock to dev dependencies, this makes the test on node 6 break,
maybe using sinon to mock the requests would be a better idea.

added a few tests for the request file,
only the happy path and some obvious errors that now get caught before even sending the request (url or data empty or not strings)

made sure the body chunks are buffers, Buffer.concat and nock did not play well together,
because nock seems to send strings.

[XhmikosR]

@jaeh can you rebase, squash and remove any unrelated changes? You have a change in test/getOptions.js that is causing tests to fail. Also you have updated all devDependencies which is also causing tests to fail.

As for nock and Node.js, can't you use an older nock version?

[XhmikosR]

NVM, I rebased it and pushed it on my fork master...XhmikosR:remove-request

Only issue left is Node.js 6.x compatibility.

@nickmerwin how about dropping support for Node.js 6.x and even 8.x so that we can move on and make the new release a major version bump?

[XhmikosR]

I tried nock 10.x without any luck. Maybe we just look into using another library to replace request and keep it simple?

[jtwebman]

Anyone looking to fix this issue, I released a fork of this library called coveralls-next as we were not getting the support we needed on this repo. https://github.com/jtwebman/coveralls-next

[jimmywarting]

How i would have written it:

let body = ''
for await (const chunk of res) {
  body += chunk
}

cb(null, { statusCode }, body)

[jimmywarting]

would also take the opportunity to use Promise instead

[jimmywarting]

Promise + throw instead? Do think this is kind of verbose

[jtwebman]

Since this library doesn't seem to be supported anymore I fix a bunch of things on a fork if you want to check it out and are still pulling the library into your packages: https://github.com/jtwebman/coveralls-next IE no more request library and no security issues.