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

Why don't we use promises in our API? #551

Closed
jgeewax opened this issue May 8, 2015 · 70 comments
Closed

Why don't we use promises in our API? #551

jgeewax opened this issue May 8, 2015 · 70 comments
Assignees
Labels
type: question Request for information or clarification. Not an issue.

Comments

@jgeewax
Copy link
Contributor

jgeewax commented May 8, 2015

UPDATE (4/25/16)

Promises coming soon. Track progress here: #1142


Seems like we're passing callbacks in rather than getting promises back -- was this an explicit choice?

/cc @idosela

@jgeewax jgeewax added the type: question Request for information or clarification. Not an issue. label May 8, 2015
@jgeewax jgeewax added this to the Core Future milestone May 8, 2015
@stephenplusplus
Copy link
Contributor

Why should we use promises? We talked about it before, but Node is callbacks, promises are an abstraction layer that not everybody enjoys using.

@jgeewax
Copy link
Contributor Author

jgeewax commented May 8, 2015

I'll leave it to @idosela to chime in on why he thinks they're great :)

I actually think they're nice, but don't think it's a make-or-break.

@ryanseys
Copy link
Contributor

ryanseys commented May 8, 2015

Yeah I think it'd be way too much work now (for us and for our users) to switch. That's a design decision definitely made up front. Also, all our dependencies and the large majority of npm packages in general use callbacks, so while I can understand the appeal for Promises for certain cases, they don't exactly sit well with what people come to expect from a Node library.

@jgeewax
Copy link
Contributor Author

jgeewax commented May 8, 2015

And I'm guessing that if someone wanted to, they could wrap what we have and add promises... I'm going to close this out as I think we've answered the question:

  • Yes, we talked about it before, but it wasn't a consistent with the rest of the Node libraries out there.
  • Yes, it's nice, but we probably won't add it.
  • If someone really loves promises, they're welcome to fork and maintain their own wrapper?

Feel free to re-open @idosela if you think we're making a huge mistake, otherwise we'll continue with our trusty ol' callbacks :)

@jgeewax
Copy link
Contributor Author

jgeewax commented May 8, 2015

... and @silvolu just pointed me at https://www.npmjs.com/package/gcloud-promise (hosted at https://github.com/sramam/gcloud-promise)

Thanks @sramam ! 👍

@idosela
Copy link

idosela commented May 10, 2015

TL;DR; I like promises, but I understand it would take a lot of work to switch to promises, so feel free to leave the issue closed.

I find promises to be very convenient for handling async operations. The most common use cases in which promises provide significant advantage over callbacks are combination, chaining, and error handling.

Let's say I want to fire off 10 Datastore queries, and then do something when they are all done.
With callbacks I would have to write a bunch of code to track when each query is complete. With promises I can combine all of the promises returned by each individual query into a single promise which will be resolved when all of the queries are done. All it takes is a single function call.

Promise chaining allows you to break down sequential async operations which depend on each other's return values, without having to resort to nesting of callbacks. This leads to a more readable and testable code.

Promises also allow the user of your API to write less code, and have more flexibility. For example, with callbacks I constantly have to check whether an error happened, and can't have a single error handler for multiple related operations.

Callbacks

dataset.save({key: blogPostKey, data: blogPostData}, function(err, response) {
  // Handle error in step 1.
  if (err) {
    return;
  }

  dataset.save({key: blogPostKey, data: {isDraft: false}}, function(err, response) {
    // Handle error in step 1.
    if (err) {
      return;
    }

    // Do something on success.
  });  
});

Promises

dataset.save({key: blogPostKey, data: blogPostData})
    .then(function(response) {
      return dataset.save({key: blogPostKey, data: {isDraft: false}});
    })
    .then(function(response) {
      // Do something on success.
    })
    .catch(function(err) {
      // Handle errors from step 1 or 2.
    });

Q (https://github.com/kriskowal/q) provides much more detailed comparison of callbacks and promises, and I know of several Node packages that use it. I'm no Node expert, but it seems to me Promises are a pattern that is not language or environment specific.

Sorry for going on and on :) Have a great weekend!

@jgeewax
Copy link
Contributor Author

jgeewax commented May 10, 2015

Out of curiosity -- is there a way we could easily support both ?

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 6, 2015

I'm reopening this issue mainly because ... I've been writing more stuff in Node and promises are becoming nicer and nicer.

Some libraries that use promises that seem really nice:

I think our library would be... kind of amazing if we could offer promises as well as the callback style we have today.

If we were to invest in this, I'd love if it we could do the work to support promises, without documenting them, and once they are ubiquitous in the library, we post the documentation.

Thoughts?

@jgeewax jgeewax reopened this Jun 6, 2015
@ryanseys
Copy link
Contributor

ryanseys commented Jun 8, 2015

What value does it add? What do you mean by "nicer and nicer"? Callbacks vs. Promises is mainly, I feel, just a personal preference. I don't like the idea of mixing both callbacks and promises because it can confuse people if they mix them. Do you know of any popular node libraries that support both callbacks and promises out of the box?

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 8, 2015

What value does it add? What do you mean by "nicer and nicer"? Callbacks vs. Promises is mainly, I feel, just a personal preference.

I'd consider the nesting of callbacks to be an anti-pattern... Further, "we accept a callback" is rather restrictive -- and "we return a promise, do what you want" is much more open.

I don't like the idea of mixing both callbacks and promises because it can confuse people if they mix them.

This could be true, but what's wrong with saying in the docs: "this method accepts a callback, and returns a promise" ? Does that really confuse you?

Do you know of any popular node libraries that support both callbacks and promises out of the box?

@idosela : Do you know of any popular node libraries that support both? I don't know of any off the top of my head...

@sramam
Copy link

sramam commented Jun 8, 2015

If all that is needed is a promise interface to gcloud-node,
a couple of lines and you're there!

Check out the excellent bluebird library and it's delightful
promisification capabilities:
https://github.com/petkaantonov/bluebird/blob/master/API.md#promisification

-shishir

On Mon, Jun 8, 2015 at 4:32 AM, JJ Geewax notifications@github.com wrote:

What value does it add? What do you mean by "nicer and nicer"? Callbacks
vs. Promises is mainly, I feel, just a personal preference.

I'd consider the nesting of callbacks to be an anti-pattern... Further,
"we accept a callback" is rather restrictive -- and "we return a promise,
do what you want" is much more open.

I don't like the idea of mixing both callbacks and promises because it can
confuse people if they mix them.

This could be true, but what's wrong with saying in the docs: "this method
accepts a callback, and returns a promise" ? Does that really confuse you?

Do you know of any popular node libraries that support both callbacks and
promises out of the box?

@idosela https://github.com/idosela : Do you know of any popular node
libraries that support both? I don't know of any off the top of my head...


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

Imagine there were no hypothetical situations. ಠ_ಠ

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 8, 2015

Very cool -- is it crazy to either document this (how you promisify our stuff using this library) or actually provide some code that does this (taking a dependency on this library)?

My worry is that

  • people will struggle to promisify stuff and get frustrated (so docs would help here)
  • we will do something that breaks the promisification, and have scary bugs (adding code + tests would help here)

@sramam
Copy link

sramam commented Jun 8, 2015

On Mon, Jun 8, 2015 at 6:12 AM, JJ Geewax notifications@github.com wrote:

Very cool -- is it crazy to either document this (how you promisify our
stuff using this library) or actually provide some code that does this
(taking a dependency on this library)?

My worry is that

  • people will struggle to promisify stuff and get frustrated (so docs
    would help here)

Perhaps documentation might be an option, but strictly speaking that is
outside the scope of the project.
That said, if one is interested in promises and node, it's not really that
hard to stumble upon Bluebird.

  • we will do something that breaks the promisification, and have scary
    bugs (adding code + tests would help here)

All promisify() requires is that the wrapped node function *"should
conform to node.js convention of accepting a callback as last argument and
calling that callback with error as the first argument and success value on
the second argument." *
Within those confines, the underlying library's features, bugs and warts do
not discriminate based on race, gender or interface! :)

Basically all I'm saying is "It's exceptionally easy and safe to use the
Bluebird promisify interface". Please try it out.


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

Imagine there were no hypothetical situations. ಠ_ಠ

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 8, 2015

Perhaps documentation might be an option, but strictly speaking that is outside the scope of the project.

Sorry -- I meant that we would document how you would use bluebird with gcloud-node. Not that bluebird should document how our library would work with it! :)

Basically all I'm saying is "It's exceptionally easy and safe to use the Bluebird promisify interface". Please try it out.

Totally - what I was saying here is that if we do add support for this, we should have tests for it so that our users aren't the ones figuring out that things are broken. We should know when we make changes if it breaks our bluebird-enabled promise usage.

@kudos
Copy link

kudos commented Jul 8, 2015

Promises allow this kind of syntax with modern javascript:

var fileData = yield bucket.file('path/to/file').download();

This syntax is already possible in node/iojs without the help of any transpilers such as babel or traceur. It's also possible to support both styles simultaneously, by switching to a promise API when no callback is passed. The Stripe node library is an example of this.

@stephenplusplus
Copy link
Contributor

This syntax is already possible in node/iojs without the help of any transpilers such as babel or traceur.

Node only supports generators behind the --harmony flag.

In some places in our API, we use the missing callback to enable stream mode:

bucket.getFiles(function(err, files) {});
bucket.getFiles().on('data', function(file) {});

I would rather see the promisifying live in another module, so everything is clear for the maintainers and the users. An example would be request and request-promise.

@jgeewax
Copy link
Contributor Author

jgeewax commented Jul 9, 2015

I have to say, the Stripe way of doing this is really nice... And it doesn't look like (from my limited perspective) the stream vs promise thing is all that problematic... right?

For example,

// .on('data') returns a new promise.
bucket.getFiles().on('data', function(file) {
  // this is called on each file in the list
  done(); // or complete() or whatever it is...
}).then(function() {
  // this is called when the stream is complete
});

bucket.getFiles().then(function(files) {
  // handles files in chunks/pages/whatever
});

bucket.getFiles(function(files) {
  // same as above.
});

@stephenplusplus
Copy link
Contributor

The stream api uses .on in a chaining manner which returns the stream to allow further piping. We would be hacking native APIs to do something magical to support 3 use cases. It's too much for our library. We can add a promise module pretty easily in the manner of request-promise, and users can just use that to get their promise on.

@jgeewax
Copy link
Contributor Author

jgeewax commented Jul 9, 2015

Are we worried at all about gcloud-promise lagging gcloud ?

@stephenplusplus
Copy link
Contributor

When we add new methods or APIs here, it would likely be a deliberate action to add support there. Can't say for sure though, I'd have to think a little more on how we would do this. I think it would be safe to make waterfall releases, where we can add in anything necessary to the promise library immediately after a release here. And as long as we follow semver, no surprise breakages would arise.

@callmehiphop any thoughts on the best way to add support for promises?

@jgeewax
Copy link
Contributor Author

jgeewax commented Jul 9, 2015 via email

@callmehiphop
Copy link
Contributor

@stephenplusplus I think supporting callbacks, promises and streams in a single API is probably possible, but is very likely to end up being kind of hacky. A separate project leveraging Bluebird is probably the cleanest way to go.

@jmdobry
Copy link
Contributor

jmdobry commented Sep 29, 2015

For what it's worth, the very first thing I did after pulling gcloud-node into my project was to figure out how to promisify it.

@jgeewax
Copy link
Contributor Author

jgeewax commented Sep 29, 2015 via email

@jmdobry
Copy link
Contributor

jmdobry commented Sep 29, 2015

Bluebird. Is there anything else? ;)

sofisl pushed a commit that referenced this issue Oct 8, 2022
- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 474338479

Source-Link: googleapis/googleapis@d5d35e0

Source-Link: googleapis/googleapis-gen@efcd3f9
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZWZjZDNmOTM5NjJhMTAzZjY4ZjAwM2UyYTFlZWNkZTZmYTIxNmEyNyJ9
sofisl pushed a commit that referenced this issue Oct 12, 2022
🤖 I have created a release *beep* *boop*
---


## [3.1.0](googleapis/nodejs-bigquery-data-transfer@v3.0.0...v3.1.0) (2022-06-29)


### Features

* support regapic LRO ([#550](googleapis/nodejs-bigquery-data-transfer#550)) ([20e2d96](googleapis/nodejs-bigquery-data-transfer@20e2d96))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this issue Oct 13, 2022
🤖 I have created a release *beep* *boop*
---


## [3.1.0](googleapis/nodejs-bigquery-data-transfer@v3.0.0...v3.1.0) (2022-06-29)


### Features

* support regapic LRO ([#550](googleapis/nodejs-bigquery-data-transfer#550)) ([20e2d96](googleapis/nodejs-bigquery-data-transfer@20e2d96))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this issue Nov 9, 2022
gcr.io/repo-automation-bots/owlbot-nodejs:latest@sha256:f4734af778c3d0eb58a6db0078907a87f2e53f3c7a6422363fc37ee52e02b25a
sofisl pushed a commit that referenced this issue Nov 10, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/87b131b2-8b10-4a99-8f57-5fac42bba415/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@b10590a
sofisl pushed a commit that referenced this issue Nov 10, 2022
chore: relocate owl bot post processor
sofisl pushed a commit that referenced this issue Nov 11, 2022
* docs(samples): add example tags to generated samples

PiperOrigin-RevId: 408439482

Source-Link: googleapis/googleapis@b9f6184

Source-Link: googleapis/googleapis-gen@eb888bc
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZWI4ODhiYzIxNGVmYzdiZjQzYmY0NjM0YjQ3MDI1NDU2NWE2NTlhNSJ9

* 🦉 Updates from OwlBot

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Nov 11, 2022
…o support relationship search Committer: yuwangyw@ (#551)

* feat: Release of relationships in v1, Add content type Relationship to support relationship search Committer: yuwangyw@

PiperOrigin-RevId: 394579113

Source-Link: googleapis/googleapis@9c7eb1f

Source-Link: googleapis/googleapis-gen@5934384

* 🦉 Updates from OwlBot

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Nov 11, 2022
🤖 I have created a release \*beep\* \*boop\*
---
## [3.19.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.18.0...v3.19.0) (2021-09-07)


### Features

* add OSConfigZonalService API Committer: [@jaiminsh](https://www.github.com/jaiminsh) ([#553](https://www.github.com/googleapis/nodejs-asset/issues/553)) ([1ab2458](https://www.github.com/googleapis/nodejs-asset/commit/1ab2458b63ebe46b8aa8edbd2b1837e793d531f1))
* Release of relationships in v1, Add content type Relationship to support relationship search Committer: yuwangyw@ ([#551](https://www.github.com/googleapis/nodejs-asset/issues/551)) ([56526b0](https://www.github.com/googleapis/nodejs-asset/commit/56526b02d14d15fd6fc469cd614bff9098f3789b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this issue Nov 11, 2022
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@types/mocha](https://togithub.com/DefinitelyTyped/DefinitelyTyped) | [`^8.0.0` -> `^9.0.0`](https://renovatebot.com/diffs/npm/@types%2fmocha/8.2.3/9.1.1) | [![age](https://badges.renovateapi.com/packages/npm/@types%2fmocha/9.1.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/@types%2fmocha/9.1.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/@types%2fmocha/9.1.1/compatibility-slim/8.2.3)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/@types%2fmocha/9.1.1/confidence-slim/8.2.3)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: "after 9am and before 3pm" (UTC).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-kms).
sofisl pushed a commit that referenced this issue Jan 17, 2023
This PR includes changes from googleapis/gapic-generator-typescript#317
that will move the asynchronous initialization and authentication from the client constructor
to an `initialize()` method. This method will be automatically called when the first RPC call
is performed.

The client library usage has not changed, there is no need to update any code.

If you want to make sure the client is authenticated _before_ the first RPC call, you can do
```js
await client.initialize();
```
manually before calling any client method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests