LinkedInAttic / sepia

Sepia is a VCR-like module for node.js that records HTTP interactions, then plays them back exactly like the first time they were invoked
Apache License 2.0
279 stars 32 forks source link

.pipe is not available in the response object #10

Closed rstacruz closed 9 years ago

rstacruz commented 9 years ago

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 commented 9 years ago

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.

rstacruz commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

awesome!