Why not using the much better request module?

[yeya]

Instead of http-request:
https://www.npmjs.com/package/request

No need to install visual studio + python just for mmmagic

[SaltwaterC]

If by "much better" you mean that it's bit more easier to install than http-request, then yeah. However, this isn't something that occurred to me as I never used Windows as development or target platform. Never will.

Otherwise, you're not making any friends by claiming that my work is inferior without actually checking all the facts / providing any proof. Blank statements don't qualify as constructive criticism. Also, in relation to #4, what makes request being "the standard" module? There's only one standard module, http.js (and by extension https.js).

It's unfortunate that nobody bothered to write a native JavaScript library for libmagic databases, which makes mmmagic the only (sane) option when it comes to proper MIME type detection which makes mmmagic painful for some users, but http-request isn't providing crippled functionality for that reason.

I guess virustotal.js may be refactored to use request. The MIME detection is not used by virustotal.js while the transparent gzip / deflate compression doesn't bring a lot of improvements for this particular use case, therefore missing this feature from the HTTP library isn't a deal breaker.

[yeya]

Well, maybe not standard, but 10,464,158 downloads last month vs 984 make
'some' difference. (npm info)

As you see - virustotal.js has been
#4 refactored to use
request.
You don't have to merge it, you are the boss.

On Tue, Jan 26, 2016 at 6:28 PM Ștefan Rusu [email protected]
wrote:

If by "much better" you mean that it's bit more easier to install than
http-request, then yeah. However, this isn't something that occurred to me
as I never used Windows as development or target platform. Never will.

Otherwise, you're not making any friends by claiming that my work is
inferior without actually checking all the facts / providing any proof.
Blank statements don't qualify as constructive criticism. Also, in relation
to #4 #4, what makes
request being "the standard" module? There's only one standard module,
http.js (and by extension https.js).

It's unfortunate that nobody bothered to write a native JavaScript library
for libmagic databases, which makes mmmagic the only (sane) option when it
comes to proper MIME type detection which makes mmmagic painful for some
users, but http-request isn't providing crippled functionality for that
reason.

I guess virustotal.js may be refactored to use request. The MIME detection
is not used by virustotal.js while the transparent gzip / deflate
compression doesn't bring a lot of improvements for this particular use
case, therefore missing this feature from the HTTP library isn't a deal
breaker.

—
Reply to this email directly or view it on GitHub
#3 (comment)
.

[danielgindi]

@SaltwaterC I don't see why you are offended so much. It's nothing personal...
npm packages are intended for easy install, in a style that is unlike the unix standard for compiling everything and breaking because of different dependencies...
The greatest binary npm packages out there come with binaries.

Also MIME detection is not something that is widely used, as there are mime-type declarations for that over the HTTP standard, and unless there's a very specific case where you have a file without an extension and without a mime type and have to know the true format - you don't need libmagic.

[SaltwaterC]

@danielgindi I don't like the crappy attitude, that's why. Makes me less likely to help people who don't show minimal etiquette.

If you read my comment carefully, my last paragraph actually agrees that request may be a good fit.

To give you some background, MIME detection is simply a dependency because http-request, libxml-to-js, virustotal.js, and aws2js are all part of a large file-based service and they kinda depend to each other. This is something which I started five years ago when the node.js ecosystem wasn't very mature.

I'll need to test this before pulling the changes.

[SaltwaterC]

@danielgindi PS: "where you have a file without an extension and without a mime type" - unless you can control the input, that's almost never the case. "file extension" is a Windows thing, while the declared content-type may be a blatant lie.

[danielgindi]

@SaltwaterC So when you receive a declared HTML from a web server, you use libmagic to make sure it's HTML?..

[SaltwaterC]

@danielgindi downloading HTML using a library has little use. Most of the time it used to be the other way around: expecting a different content type while getting HTML instead with no content-type at all as it's not mandated by HTTP (i.e request http://example.com/foobar.exe and get an HTML page with status 200). It's amazing what users can supply when you have lots of URLs in your database. Another example: really badly behaved servers that reply with content-encoding: gzip and plain text response body. While this isn't handled by libmagic, it's an example of how bad the web may be.

Or, my all time favourite, sending an over 100 megs file instead of an XML document (which in turn should reference that file) which crashes libxml as it's not a streaming parser and node.js is too memory limited. content-length is also not guaranteed (the download may be supplied as attachment), but in retrospect I guess I could have checked the file size before sending it to the parser.

Grabbing one chunk of data and reading the header with libmagic makes it a lot easier to abort on bad input, or figuring out that the expected type is the actual type.

[danielgindi]

So as I said, there is a very specific use case where you would find libmagic useful :-)

Anyway, why not merge this, as this makes your package install on any machine without native bindings?.. The title may be something you disagree on as you are the author of http-request, but it still makes this package unusable on non-linux machines.

On a side note - a suggestion:
Make the libmagic bindings for your http-request optional, in a different module that "extends" or "subclasses" http-request. Or even request. This would allow you to use more modules on more machines, and to reduce installation time for existing modules (compiling takes time).

[SaltwaterC]

To quote myself: I'll need to test this before pulling the changes. aka I'll need to run the integration tests suite. I'll do it now.

As for optional dependencies, I already tried this with anther module (aws2js 0.7.x) and it was a huge failure. People have various use cases which break installations with optional modules in very interesting ways. Let's say that it solves less problems than the issues having optional dependencies introduced.

Probably this should be a function of the package manager itself. I remember talks about supporting Windows binaries in npm, but I'm not very active in the node community these days.

[SaltwaterC]

After dealing with minor annoyances (pull request sent against wrong branch aka merge failure, and lint failures), there's few issues which need to be addressed:

• getIpReport() triggers some sort of race condition. This is unrelated to the node.js or (http-)request version. I can't pinpoint the exact source, therefore I have to dig deeper. This issue appears regardless of the used HTTP lib. Needless to say, it wasn't an issue last time when I touched the code
• getFileReport() and getUrlReport() fail with the "much better standard" request lib (Uncaught SyntaxError: Unexpected end of input). They do not fail with http-request. My guess is: this is related to the removal of the empty buffer check in jsonHandler.

[danielgindi]

@yeya Maybe you should check on that missing empty check? Given that request is giving you the responseBody as a string, then a simple if (!responseBody) { would do

[yeya]

The PR have been fixed and should now work the same as with the original http-request module

[yeya]

Fixed and rebased.

On Wed, Jan 27, 2016, 16:26 Daniel Cohen Gindi [email protected]
wrote:

@yeya https://github.com/yeya Maybe you should check on that missing
empty check? Given that request is giving you the responseBody as a
string, then a simple if (!responseBody) { would do

—
Reply to this email directly or view it on GitHub
#3 (comment)
.