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

Add simultaneous promise and callback support #523

Closed
jmdobry opened this issue Mar 4, 2016 · 17 comments
Closed

Add simultaneous promise and callback support #523

jmdobry opened this issue Mar 4, 2016 · 17 comments
Assignees
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@jmdobry
Copy link
Contributor

jmdobry commented Mar 4, 2016

No description provided.

@jmdobry jmdobry added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. complexity: medium labels Mar 4, 2016
@jmdobry jmdobry self-assigned this Mar 4, 2016
jmdobry added a commit that referenced this issue Apr 22, 2016
@GitTom
Copy link

GitTom commented Jun 29, 2016

+1

For example, see how they added promise support to AWS node library:
https://blogs.aws.amazon.com/javascript/post/Tx3BZ2DC4XARUGG/Support-for-Promises-in-the-SDK

@lirbank
Copy link

lirbank commented Jul 30, 2016

See also MongoDBs node driver. It very nicely supports both callbacks and promises in the same driver.
http://mongodb.github.io/node-mongodb-native/2.2/api/

@thepatrick
Copy link

It's incredibly confusing that the "JavaScript client library" supports promises but this library doesn't.

How can progress be made on this? We have async/await now, and those work brilliantly with libraries with promise support.

@Robert-Ernst
Copy link

Seems like Google doesn't love NodeJs/Javascript 💔💔💔

@simonh1000
Copy link

simonh1000 commented Nov 21, 2017

Googles Docs recommend using promises but I cannot find how to use them with this library

@Mozcatel
Copy link

+1
Seems we cant use the full asynchronous control Node allows us on this library, either Promises or async/await

@machineghost
Copy link

machineghost commented Feb 7, 2018

So this ticket is nearly two years old and hasn't had any dev response in that entire time ... does that mean it's dead?

If not, Node's util.promisify seems like it would make solving this issue trivial. It "takes a function following the common error-first callback style, i.e. taking a (err, value) => ... callback as the last argument, and returns a version that returns promises." ... and all of the API calls I've seen in this library follow that format (ie. callback is the last arg and the callback expects an error as the first arg and result as the second).

I would be happy to submit a PR that simply calls util.promisify on every callback-using function in this library (and updates the consumers appropriately as well of course).

@JustinBeckwith
Copy link
Contributor

Nope, just slow moving :) This project is largely maintained by volunteers on free time, so sometimes stuff like this can take a while. We still very much want to do it, but a few things need to happen first.

In the meantime - you can use tools like pify today to promisify the API pretty easily.

We also love PRs :)

@machineghost
Copy link

machineghost commented Feb 7, 2018

We also love PRs :)

As I said (in an edit, so you might have missed it) I'd be happy to submit one, but only if it will actually be used. I certainly wouldn't want to go through and promisify everything if the library's maintainers don't want that.

@JustinBeckwith
Copy link
Contributor

JustinBeckwith commented Feb 7, 2018

Apologies! I did miss the edit. I don't think we want to run through and util.promisfy everything. We're using TypeScript, which lets us write our code with promises or Async/Await, and then transpile down to es5 so we can support node 4 and 6. I believe util.promisfy is only in Node 8, so that's sadly off the table. That's where (if we need to, which we shouldn't) we could use pify.

We already went through the painful process in #891 of swapping out from request to axios in the underlying auth library, so that means the table is actually set for supporting both callbacks and promises. To support back-compat, I really think we need to do both. Here's an example of how we do it in the auth library:
https://github.com/google/google-auth-library-nodejs/blob/master/src/auth/oauth2client.ts#L464

The work here is to introduce a similar pattern. We need function overloads for public methods that support both the callback and promise based workflows. Famous last words... but I think we've already done most of the heavy lifting here :)

I would be ecstatic to review a PR that takes this approach!

@machineghost
Copy link

We're using TypeScript

Ugh. My apologies but I'm afraid I have to rescind my offer of a PR, as I'm really not a fan of TypeScript.

I do hope someone else will pick up the task though, as I would ❤️ being able to use this library with async/await.

@acoll
Copy link

acoll commented Feb 16, 2018

Would something as simple as this work? Basically return the called request functions (which already have conditional promise support) and remove the call to normalize the callback function.

acoll@998e1f6

I was just playing around with this quickly on my local but it seemed to work for the couple of apis I'm using. I can send a PR with some tests if this seems like the right direction.

@naturalethic
Copy link

@acoll Thanks a lot for that I adapted it for my own use.

@naturalethic
Copy link

Well that was a lot of work. Nice job.

@qraynaud
Copy link

There is an issue with how this was implemented.

Before the APIs returned a stream and now they don't. Which means some examples on the online document are broken (like here, first Node exemple).

What is more, in this use case, we want to download a file. It might weight Gbs or even Tbs. Using a promise that waits until the request completes (or a callback for that matter) means it will load the whole file in RAM. This is totally unacceptable right?! I want to pipe the request in something and be rid of it!

It would be great to return a proxy instead of the actual axios promise. It could accept to act like a stream or like a promise depending on how it's used!

@qraynaud
Copy link

The other solution would be to expose the responseType attribute of axios. So I could set it to stream and it would be perfect too!

@qraynaud
Copy link

Ok, reading the code I found out it is indeed exposed as the second parameter...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests