.pipe is not available in the response object #10

Closed
rstacruz opened this Issue Mar 2, 2015 · 6 comments

Projects

None yet

2 participants

@rstacruz
rstacruz commented Mar 2, 2015

In this code below, res.pipe is undefined when VCR_MODE=cache is declared. It is otherwise available when Sepia is not used.

require('sepia');
var http = require('http');

var req = http.request({ hostname: 'google.com' }, function (res) {
  console.log(res.pipe); //=> undefined
});
req.end();

This breaks libraries such as then-request. then/then-request#12

@avik-das
Contributor
avik-das commented Mar 2, 2015

This is another symptom of issue #5, and issue is that sepia provides dummy request and response objects that don't have all the methods present in the original ones.

I'll provide a fix where I add the pipe method, and I'll write a new test case that exercises then-request (which looks like a cool module that I didn't know about). If you'd like to provide a pull request in the meantime, I'll be happy to look at it, but otherwise, I'll work on this soon.

@avik-das avik-das self-assigned this Mar 2, 2015
@rstacruz
rstacruz commented Mar 2, 2015

I haven't tried to dig into Sepia's code yet, but http.ClientRequest is a subclass of a Stream.

Couldn't the mock response also be implemented as a subclass of Stream?

@avik-das
Contributor
avik-das commented Mar 3, 2015

The request (which indeed has type http.ClientRequest) is not the one we need to mock out to fix this; it's the response, which is an http.ClientResponse, a subclass of http.IncomingMessage).

I'm currently switching the mock response to be an http.IncomingMessage. The problem is that http.IncomingMessage is a Stream.ReadableStream, and normally, the way its internal buffer is populated is an internal detail of node. The request module hooks onto the data event on the response, and everything is fine, but the trick here is to figure out how to ensure how to interoperate with stream libraries, in this case the concat-stream library that's used by then-request.

@avik-das
Contributor
avik-das commented Mar 3, 2015

Ok, I looked through how the node core library populated the IncomingMessage, and I think I have a fix. I'm going to clean it up and do some testing tomorrow before I release it.

@avik-das avik-das pushed a commit that closed this issue Mar 9, 2015
Avik Das Fix #10: Support for node 0.10.x-style APIs
When sepia was tested with the then-request module, it turned out that
sepia was not compatible with that module due not conforming to certain
updated node behavior in the standard library.

Of course, normally, this type of behavior is transparent to users of
node, but sepia has to emulate that behavior, hence the breakage.

Unfortunately, the updated behavior is not compatible with 0.8.x, which
is an existing use case. I'll deprecate it eventually, but not yet. To
support both 0.8.x and 0.10.x, I added some conditional code.
96e2093
@avik-das avik-das closed this in 96e2093 Mar 9, 2015
@avik-das
Contributor
avik-das commented Mar 9, 2015

@rstacruz: Sorry for the delay. I had to test the fix against an existing codebase that's still on node 0.8.x, which meant some conditional code to support both versions of node (0.8.x and 0.10.x).

You should be able to update to 2.0.1. Note that if you're using 1.x currently, you'll want to check the release notes for 2.0.0, which was technically a backwards-incompatible change.

Please let me know if there's any issue.

@rstacruz

awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment