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.

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

.pipe is not available in the response object #10

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

.pipe is not available in the response object #10

rstacruz opened this issue Mar 2, 2015 · 6 comments
Assignees

Comments

@rstacruz
Copy link

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
Copy link
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
Copy link
Author

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
Copy link
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
Copy link
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
Copy link
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
Copy link
Author

awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants