request(...).on is not a function #1

New Issue

Ghost · 2020-02-20T15:48:41Z

Ghost commented

2020-02-20 15:48:41 +00:00

I tried just replacing request with @root/request in html2canvas-proxy, but it seems like @root/request isn't feature-complete enough to allow this.

This library uses request(...).on, which doesn't seem to be supported by @root/request.

Full code :

const express = require('express');
const url = require('url');
const cors = require('cors');
const request = require('@root/request');

function validUrl(req, res, next) {
    if (!req.query.url) {
        next(new Error('No url specified'));
    } else if (typeof req.query.url !== 'string' || url.parse(req.query.url).host === null) {
        next(new Error(`Invalid url specified: ${req.query.url}`));
    } else {
        next();
    }
}

module.exports = () => {
    const app = express.Router();
    app.get('/', cors(), validUrl, (req, res, next) => {
        switch (req.query.responseType) {
    case 'blob':
        req.pipe(request(req.query.url).on('error', next)).pipe(res);
        break;
    case 'text':
    default:
        request({url: req.query.url, encoding: 'binary'}, (error, response, body) => {
            if (error) {
                return next(error);
            }
            res.send(
            `data:${response.headers['content-type']};base64,${Buffer.from(
                body,
                'binary'
            ).toString('base64')}`
        );
    });
    }
});

    return app;
};

I tried just replacing `request` with `@root/request` in [`html2canvas-proxy`](https://www.npmjs.com/package/html2canvas-proxy), but it seems like `@root/request` isn't feature-complete enough to allow this. This library uses `request(...).on`, which doesn't seem to be supported by `@root/request`. Full code : ````js const express = require('express'); const url = require('url'); const cors = require('cors'); const request = require('@root/request'); function validUrl(req, res, next) { if (!req.query.url) { next(new Error('No url specified')); } else if (typeof req.query.url !== 'string' || url.parse(req.query.url).host === null) { next(new Error(`Invalid url specified: ${req.query.url}`)); } else { next(); } } module.exports = () => { const app = express.Router(); app.get('/', cors(), validUrl, (req, res, next) => { switch (req.query.responseType) { case 'blob': req.pipe(request(req.query.url).on('error', next)).pipe(res); break; case 'text': default: request({url: req.query.url, encoding: 'binary'}, (error, response, body) => { if (error) { return next(error); } res.send( `data:${response.headers['content-type']};base64,${Buffer.from( body, 'binary' ).toString('base64')}` ); }); } }); return app; }; ````

Ghost commented

2020-02-20 15:54:17 +00:00

I tried just dropping the .on('error', next) part, since all it seemed to be doing, was error handling anyway...

Unfortunately, this just produced a different error :

_stream_readable.js:832
      dest.emit('unpipe', this, unpipeInfo);
           ^

TypeError: dest.emit is not a function
    at IncomingMessage.Readable.unpipe (_stream_readable.js:832:12)
    at unpipe (D:\projects\Adshot Development\src\core-adshot\node_modules\unpipe\index.js:47:12)
    at send (D:\projects\Adshot Development\src\core-adshot\node_modules\finalhandler\index.js:306:3)
    at D:\projects\Adshot Development\src\core-adshot\node_modules\finalhandler\index.js:133:5
    at D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:635:15
    at next (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:260:14)
    at Layer.handle_error (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\layer.js:67:12)
    at trim_prefix (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:315:13)
    at D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:284:7
    at Function.process_params (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:335:12)
    at next (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:275:10)
    at Layer.handle_error (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\layer.js:67:12)
    at trim_prefix (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:315:13)
    at D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:284:7
    at Function.process_params (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:335:12)
    at Immediate.next (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:275:10)

I tried just dropping the `.on('error', next)` part, since all it seemed to be doing, was error handling anyway... Unfortunately, this just produced a different error : ```` _stream_readable.js:832 dest.emit('unpipe', this, unpipeInfo); ^ TypeError: dest.emit is not a function at IncomingMessage.Readable.unpipe (_stream_readable.js:832:12) at unpipe (D:\projects\Adshot Development\src\core-adshot\node_modules\unpipe\index.js:47:12) at send (D:\projects\Adshot Development\src\core-adshot\node_modules\finalhandler\index.js:306:3) at D:\projects\Adshot Development\src\core-adshot\node_modules\finalhandler\index.js:133:5 at D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:635:15 at next (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:260:14) at Layer.handle_error (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\layer.js:67:12) at trim_prefix (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:315:13) at D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:284:7 at Function.process_params (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:335:12) at next (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:275:10) at Layer.handle_error (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\layer.js:67:12) at trim_prefix (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:315:13) at D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:284:7 at Function.process_params (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:335:12) at Immediate.next (D:\projects\Adshot Development\src\core-adshot\node_modules\express\lib\router\index.js:275:10) ````

coolaj86 commented

2020-02-20 16:38:49 +00:00

it seems like @root/request isn’t feature-complete enough to allow this

That is correct. It does NOT handle streams.

If you want a light-weight alternative to http streams, using require("http") really is the best way to go.

If someone could figure out a nice way to add it in less than 100 LoC, I'd definitely consider including it (and that may be very possible).

My goal with this was to copy the features of request that make it easier to use than http. Since streams are already easier to work with in http than in request, and it would just bloat the API, I didn't include it.

> it seems like @root/request isn’t feature-complete enough to allow this That is correct. It does NOT handle streams. If you want a light-weight alternative to http streams, using `require("http")` really is the best way to go. If someone could figure out a nice way to add it in less than 100 LoC, I'd definitely consider including it (and that may be very possible). My goal with this was to copy the features of `request` that make it easier to use than `http`. Since streams are _already_ **easier** to work with in `http` than in `request`, and it would just bloat the API, I didn't include it.

coolaj86 commented

2020-02-20 16:59:52 +00:00

Oh, just noticed the second part of your message:

_stream_readable.js:832
      dest.emit('unpipe', this, unpipeInfo);
           ^

This also looks like a stream issue with req.pipe. Again, I'd suggest using request('http') if you need a lightweight solution.

Also, you can use promises:

request({url: req.query.url, encoding: 'binary'}).then(function (response) {
  // response.body...
}).catch(function (err) {
  // handle error... or don't :)
});

Oh, just noticed the second part of your message: ``` _stream_readable.js:832 dest.emit('unpipe', this, unpipeInfo); ^ ``` This also looks like a stream issue with `req.pipe`. Again, I'd suggest using `request('http')` if you need a lightweight solution. Also, you can use promises: ``` request({url: req.query.url, encoding: 'binary'}).then(function (response) { // response.body... }).catch(function (err) { // handle error... or don't :) }); ```

Ghost commented

2020-02-20 17:22:56 +00:00

@solderjs :

IMO, the whole point of a drop-in replacement is that you can replace one dependency with another everywhere and kind of presume that everything will continue working, because they share the same API and behavior.

If a significant part of projects out there can't use @root/request because it does not handle streams, it becomes quite useless for most people.

Also, I tried replacing request with require("http").request & require("https").request here, but I couldn't get that to work either.

I suppose maybe I need to replace the first request call with @root/request and the second once with require("http").request or require("https").request?

Either way, this is already causing me way more of a headache than I had initially expected. I do think @root/request really needs piping to be able to deliver on its promise of being a drop-in replacement of request.

If these features are already present in require("http").request & require("https").request, can't you just write wrappers for them and add those to @root/request?

Anyway, I just replaced request with postman-request, which is a fork of request used by Postman. That one does in fact act as a drop-in replacement. And while it's probably not nearly as lightweigth as @root/request, at least it solves my issue of getting rid of the deprecation message and not having to worry about breaking some obscure part of my app!

@solderjs : IMO, the whole point of a drop-in replacement is that you can replace one dependency with another everywhere and kind of presume that everything will continue working, because they share the same API and behavior. If a significant part of projects out there can't use `@root/request` because it does not handle streams, it becomes quite useless for most people. Also, I tried replacing `request` with `require("http").request` & `require("https").request` here, but I couldn't get that to work either. I suppose maybe I need to replace the first `request` call with `@root/request` and the second once with `require("http").request` or `require("https").request`? Either way, this is already causing me way more of a headache than I had initially expected. I do think `@root/request` really needs piping to be able to deliver on its promise of being a drop-in replacement of `request`. If these features are already present in `require("http").request` & `require("https").request`, can't you just write wrappers for them and add those to `@root/request`? Anyway, I just replaced `request` with `postman-request`, which is a fork of `request` used by Postman. That one does in fact act as a drop-in replacement. And while it's probably not nearly as lightweigth as `@root/request`, at least it solves my issue of getting rid of the deprecation message and not having to worry about breaking some obscure part of my app!

coolaj86 commented

2020-02-20 17:29:54 +00:00

the whole point of a drop-in replacement

Yes, but it's only a drop-in for the 20% of features that fit what 80% of people use.

I don't disagree that streams are important, but they're in that 10-20% that most people don't use.

can’t you just write wrappers for them

Yeah, it's just complicated. Like I said, if someone wanted to add it, I'd be open to it, but this project is meant to be small and simple.

Out of the 50-200 MILLION downloads I get each month from the projects I've published to npm, NONE of them put food on my table or a roof over my head.

If you wanted to pay me to figure out those wrappers, I'd love to do it, but for a small project like this... I literally can't afford to spend the time on it.

Sad truth of open source. :-/

> the whole point of a drop-in replacement Yes, but it's only a drop-in for the 20% of features that fit what 80% of people use. I don't disagree that streams are important, but they're in that 10-20% that most people don't use. > can’t you just write wrappers for them Yeah, it's just complicated. Like I said, if someone wanted to add it, I'd be open to it, but this project is meant to be small and simple. Out of the 50-200 **MILLION** downloads I get each month from the projects I've published to npm, NONE of them put food on my table or a roof over my head. If you wanted to pay me to figure out those wrappers, I'd love to do it, but for a small project like this... I literally can't afford to spend the time on it. Sad truth of open source. :-/

coolaj86 commented

2020-02-20 17:35:00 +00:00

What do you think about this?

ef3183e984

What do you think about this? https://git.coolaj86.com/coolaj86/request.js/commit/ef3183e984c156cee8ecd4c1b61b0b1915ca8003

Ghost commented

2020-02-20 17:45:49 +00:00

Yes, but it’s only a drop-in for the 20% of features that fit what 80% of people use.

There's +41.000 packages using request.

20% of 41.000 is still 8.200!

If you wanted to pay me to figure out those wrappers, I’d love to do it, but for a small project like this… I literally can’t afford to spend the time on it.

I get that.
I respect that.
I don't have any time left for open source work myself these days.

Just saying... I don't think this project qualifies as a drop-in replacement if it only supports 20% of the features!

What do you think about this?

I'd be fine with that if you add a reference to postman-request for people who do care about the missing 80%!

> Yes, but it’s only a drop-in for the 20% of features that fit what 80% of people use. There's +41.000 packages using `request`. 20% of 41.000 is still 8.200! > If you wanted to pay me to figure out those wrappers, I’d love to do it, but for a small project like this… I literally can’t afford to spend the time on it. I get that. I respect that. I don't have any time left for open source work myself these days. Just saying... I don't think this project qualifies as a drop-in replacement if it only supports 20% of the features! > What do you think about this? I'd be fine with that if you add a reference to `postman-request` for people who do care about the missing 80%!

coolaj86 commented

2020-02-20 19:05:41 +00:00

20% of 41.000 is still 8.200!

Well, there's nothing broken in request. It works the same that it ever did.

(also, those are published packages using request, which is probably closer to 1% of the packages that actually use it out on the web today)

The author announced that he isn't going to do any more updates to it - nothing more, nothing less.

He actually announced that a year ago (I won't dig through the issues to find it), but only put the message about it in npm recently.

In any case, I'm certainly not trying to take on the burden that... didn't pay his bills either. :P

I’d be fine with that if you add a reference to postman-request for people who do care about the missing 80%!

request is complete. It's done. It's reached the final stage of maturity - the author is walking away. That's a great place to be - it's pretty much perfect.

If you need it, just use it.

> 20% of 41.000 is still 8.200! Well, there's nothing broken in `request`. It works the same that it ever did. (also, those are published packages using request, which is probably closer to 1% of the packages that actually use it out on the web today) The author announced that he isn't going to do any more updates to it - nothing more, nothing less. He actually announced that a year ago (I won't dig through the issues to find it), but only put the message about it in npm recently. In any case, I'm certainly not trying to take on the burden that... didn't pay his bills either. :P > I’d be fine with that if you add a reference to postman-request for people who do care about the missing 80%! `request` is complete. It's done. It's reached the final stage of maturity - the author is walking away. That's a great place to be - it's pretty much perfect. If you need _it_, just use _it_.

Ghost commented

2020-02-21 13:08:32 +00:00

Well, there’s nothing broken in request. It works the same that it ever did.

Deprecation messages are not supposed to be ignored. They are intended as a call to action. And they should not be used for any other purpose.

Deprecating eg. a part of your API or a library basically means that you officially designate it as "obsolete" and you actively encourage users to opt for something else.

Deprecation is typically used as an intermediary stage between officially supporting something and officially dropping support for something, to give developers the time to replace whatever the thing is that you've deprecated until you that thing is no longer available or backwards compatible.

Basically, the point of deprecation is to offer developers a "grace period", allowing them update to their code before someone pulls the plug.

(also, those are published packages using request, which is probably closer to 1% of the packages that actually use it out on the web today)

Those are published packages using request. If you add the packages & apps depending on at least one of those 41K dependencies, the real number is probably in the millions!

Just he platform I'm working on has 4 different dependencies that have request as a direct dependency!

The author announced that he isn’t going to do any more updates to it - nothing more, nothing less.

That's not what deprecation is supposed to be used for.

And it's plain ugly to have your npm install logs full of deprecation warnings. It looks sloppy. It's kind of a red flag and creates a bad first impression to people trying out your library or framework. Especially if people are actually paying you to use your library / framework, you want to give them a nice / clean install process with zero warnings.

request is complete. It’s done. It’s reached the final stage of maturity - the author is walking away. That’s a great place to be - it’s pretty much perfect.

If it is indeed "pretty much perfect", that's a very good reason for NOT officially deprecating a library and just officially ending maintenance & support for it.

It seems the author - and many others out there - are confused about what deprecation is for and what to do when you no longer want to or are able to provide support for a project.

Also, I'd have much less of an issue with the deprecation if authors would officially recommend postman-request as an official feature-complete drop-in replacement and possibly your @root/request project as a lightweight alternative with just a partial implementation. This would allow any package maintainer to drop request and get rid of the annoying deprecation message without spending more than a few minutes of dev time on this issue, and without having to refactor their entire library or app.

Also, besides being feature-complete postman-request is still actively maintained AND contains some bug-fixes that never made it to request for some reason...

> Well, there’s nothing broken in `request`. It works the same that it ever did. Deprecation messages are not supposed to be ignored. They are intended as a call to action. And they should not be used for any other purpose. Deprecating eg. a part of your API or a library basically means that you officially designate it as "obsolete" and you actively encourage users to opt for something else. Deprecation is typically used as an intermediary stage between officially supporting something and officially dropping support for something, to give developers the time to replace whatever the thing is that you've deprecated until you that thing is no longer available or backwards compatible. Basically, the point of deprecation is to offer developers a "grace period", allowing them update to their code before someone pulls the plug. > (also, those are published packages using request, which is probably closer to 1% of the packages that actually use it out on the web today) Those are published packages using request. If you add the packages & apps depending on at least one of those 41K dependencies, the real number is probably in the millions! Just he platform I'm working on has 4 different dependencies that have `request` as a direct dependency! > The author announced that he isn’t going to do any more updates to it - nothing more, nothing less. That's not what deprecation is supposed to be used for. And it's plain ugly to have your `npm install` logs full of deprecation warnings. It looks sloppy. It's kind of a red flag and creates a bad first impression to people trying out your library or framework. Especially if people are actually paying you to use your library / framework, you want to give them a nice / clean install process with zero warnings. > `request` is complete. It’s done. It’s reached the final stage of maturity - the author is walking away. That’s a great place to be - it’s pretty much perfect. If it is indeed "pretty much perfect", that's a very good reason for NOT officially deprecating a library and just officially ending maintenance & support for it. It seems the author - and many others out there - are confused about what deprecation is for and what to do when you no longer want to or are able to provide support for a project. Also, I'd have much less of an issue with the deprecation if authors would officially recommend `postman-request` as an official feature-complete drop-in replacement and possibly your `@root/request` project as a lightweight alternative with just a partial implementation. This would allow any package maintainer to drop request and get rid of the annoying deprecation message without spending more than a few minutes of dev time on this issue, and without having to refactor their entire library or app. Also, besides being feature-complete `postman-request` is still actively maintained AND contains some bug-fixes that never made it to `request` for some reason...

coolaj86 commented

2020-03-12 08:38:11 +00:00

FYI: v1.5.0 adds missing stream support for requests.

body may now be an instance of ReadableStream as documented.

I still think that request().on() would be a bad idea, but I'm thinking that something like this might be a good fit:

request({
  ...,
  stream: true
}).then(function (resp) {
  return new Promise(function (resolve, reject) {
    resp.body.pipe(w);
    w.on('finished', resolve);
    w.on('error', reject);
  });
})

FYI: v1.5.0 adds missing stream support for requests. `body` may now be an instance of `ReadableStream` as documented. I still think that `request().on()` would be a bad idea, but I'm thinking that something like this might be a good fit: ```js request({ ..., stream: true }).then(function (resp) { return new Promise(function (resolve, reject) { resp.body.pipe(w); w.on('finished', resolve); w.on('error', reject); }); }) ```

Ghost commented

2020-03-12 09:26:48 +00:00

Awesome!

So, does that mean that your drop-in replacement is now feature complete?

if not, which parts are still missing?

Awesome! So, does that mean that your drop-in replacement is now feature complete? if not, which parts are still missing?

coolaj86 commented

2020-03-12 10:39:46 +00:00

No, I still don't have support for request().on() and I'm not sure how I would or could add that in a clean way.

request did, literally, hundreds of (unnecessary) things. This will never be a 100% drop-in replacement, but as I encounter issues that can be fixed with very few code changes, I will add them.

No, I still don't have support for `request().on()` and I'm not sure how I would or could add that in a clean way. `request` did, literally, hundreds of (unnecessary) things. This will never be a 100% drop-in replacement, but as I encounter issues that can be fixed with very few code changes, I will add them.

coolaj86 commented

2020-04-21 05:59:04 +00:00

Since I don't know of a reasonable way to solve this without complicating and/or breaking the API, I'm just closing this out.

coolaj86 closed this issue

2020-04-21 05:59:04 +00:00

coolaj86 commented

2021-08-06 20:08:42 +00:00

Update: Full stream support was added in v1.7.0.

In order to keep the code small and simple, there is one tiny difference when using it as a stream - you must first await the request:

-var request = require('request');
+var request = require('@root/request');
 
-var stream = request({ url, headers });
+var stream = await request({ url, headers });

 let attachment = await new MailgunAPI.Attachment({
   data: stream
 })

**Update**: Full stream support was added in v1.7.0. In order to keep the code small and simple, there is one _tiny_ difference when using it as a stream - you must first `await` the request: ```diff -var request = require('request'); +var request = require('@root/request'); -var stream = request({ url, headers }); +var stream = await request({ url, headers }); let attachment = await new MailgunAPI.Attachment({ data: stream }) ```

Sign in to join this conversation.

No Label

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: root/request.js#1