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

Added destroy method for stream #12

Merged
merged 1 commit into from Jul 16, 2012

Conversation

koenpunt
Copy link
Contributor

Implemented missing method destroy on stream and added simple test for streaming API

Simple test for streaming API
Replaced hard-tabs with with soft-tabs
@koenpunt koenpunt closed this Jul 16, 2012
impronunciable added a commit that referenced this pull request Jul 16, 2012
@impronunciable impronunciable merged commit 75a6477 into impronunciable:master Jul 16, 2012
@koenpunt
Copy link
Contributor Author

Ok, but maybe initiating a Stream object is better than creating a custom destroy... Trying that right now

@impronunciable
Copy link
Owner

I thought about that. First I merged this to have things working and then need to implement the stream as a real stream...

@koenpunt
Copy link
Contributor Author

I guess the custom Stream object isn't even necessary, because req is already an instance of Stream.

@impronunciable
Copy link
Owner

I don't want to return back to the user the whole request to handle. I like this approach with a custom stream object containing only the methods and data that is relevant to the user.

@koenpunt
Copy link
Contributor Author

Ok fair enough, maybe initiating Stream like:

var stream = new Stream(req);

and setting the request in Stream as instance variable

function Stream(req){
    this.request = req;
}
Stream.prototype.destroy = function(){
    this.request.destroy();
}

so the original request is also available?

I've submitted a PR for this: #13

@impronunciable
Copy link
Owner

There is no need to do that. The only thing I want to implement is a function that destroy the stream, so I can start a new one for a possible reconnection.

Also if I'm starting using a stream from the Stream module I need to use another name to the 'error' event, that means I need to change the API of the module and I don't want that.

I think that adding the new destroy function it's good, at least for now.

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

Successfully merging this pull request may close these issues.

None yet

2 participants