[wip] return a promise when there is no callback in concat

[mcollina]

I've added support for async/await during get.concat().

This is a wip, and more intended to discuss rather than a proper direction.

The main concern is on the return type, which is in the form of { res, data }, so it uses a full object instead of positional arguments. This is also the main reason why util.promisify() cannot be used with get.concat().

It also make the module pass the > 100 lines :(.

(we can also ship require('simple-get/promise') as a second entry point).

[feross]

Thanks for the PR! I was planning to look into Promise support for the next major version. A couple thoughts:

1. What do you think about dropping the callback interface entirely and releasing a new major version? I'm hesitant to support both interfaces and duplicate the full test suite across both patterns.

2. Rather than returning an object { res, data }, let's just set the data on the response object as res.body. Then we can just return res.

Thoughts?

[mcollina]

What do you think about dropping the callback interface entirely and releasing a new major version?

I think supporting both is the way to go for any existing library. Switching from one to the other implies that all of your users will have to migrate from one to the other, or be stuck on a minor updates.

I'm hesitant to support both interfaces and duplicate the full test suite across both patterns.

I typically don't duplicate the tests. I go additive and just test the promise layer on top. I know it's more messy but it gets the job done and it does not break anybody.

I'm also +1 to add some async-await specific tests, just to validate how the user will see the API (see https://github.com/fastify/fastify/blob/master/test/async-await.test.js and https://github.com/fastify/fastify/blob/master/test/async-await.js).

Another option is to switch to res.body anyway (this is the main offender), and then recommend user to rely on https://nodejs.org/api/util.html#util_util_promisify_original.

[emilbayes]

+1 for this, as well as keeping both interfaces. Use simple-get as my main http request library, and using async/await in a callback context and vice versa is awkward at best

[roblav96]

+1 for this merge! :D

[mcollina]

@feross would you like me to update this to use response.body instead? We might want to keep the wrapping object anyway, because we might want to return the req as well.

[feross]

@mcollina I want to think about this a bit more before deciding what to do.

[tahercool1]

Any updates on this?

[yaneony]

Still nothing since 2 years?!