search

joepie91 / node-bhttp

A sane HTTP client library for Node.js with Streams2 support.
62 stars 12 forks source link

Compatibility with old streams? #17

Closed stevenvachon closed 8 years ago

stevenvachon commented 8 years ago

Perhaps robots-txt-parse#2 is related.

joepie91 commented 8 years ago

This may be an issue with Node streams themselves - I've had trouble with pause and resume in certain 0.10.x versions, which led me to implementing this patch internally (replacing pause with a pipe to the dev-null module).

Your application is erroring out at this line, triggered by this line in robots-txt-parse, which calls the original resume method on the stream. I can't really see anything in bhttp that would cause that kind of behaviour.

To me, it looks like the issue is with unavailability of resume in (some) 0.10 versions of Node, and that's an issue that can only be worked around by the authors of robots-txt-parse.

stevenvachon commented 8 years ago

Thanks for the response!

The robots-txt-parse dev asked about readable-stream. It sounds interesting.

Janpot commented 8 years ago

But the stream originates from bhttp if I understand correctly?

Wouldn't it be enough to use readable-stream here in your module to make it work properly across node versions? This should be just a drop-in replacement for you.

joepie91 commented 8 years ago

It's not that easy, unfortunately - the stream that bhttp provides is the original response stream from the http core module, with some extra properties attached. I don't have any control over what underlying stream implementation it uses.

Fundamentally, robots-txt-parse can't really rely on the version of a stream that is being passed into it anyhow, whether that is a stream from bhttp or another module entirely. It therefore seems to me that this would have to be fixed in robots-txt-parse, to really accomplish predictable behaviour.

Perhaps this would be of use for robots-txt-parse? I'm unsure whether it would solve this particular issue, but it claims to upgrade streams.

EDIT: Looking at the code again, it's not entirely clear to me why resume is being called to begin with?

Janpot commented 8 years ago

Looking at the code again, it's not entirely clear to me why resume is being called to begin with?

If I recall correctly, but it's been a while, it had to do with stream quirks in the event-stream package.

stevenvachon commented 8 years ago

Thank you both!! Issue resolved in latest robots-txt-parse release with event-stream package resume removed.