core: add support for promises

[callmehiphop]

Fixes #551

This PR aims to add support for Promises.

This will also introduce a breaking change moving all stream related APIs to a separate method.

Because this will be such an enormous change and GitHub probably won't be able to display everything, logical commits will be used to make reviewing the changes a little easier.

TODO:

• Core support
• Service support - this includes unit tests, system tests and documentation.
  • BigQuery
  • Bigtable
  • Compute
  • Datastore
  • DNS
  • Logging
  • Prediction
  • PubSub
  • Resource Manager
  • Speech
  • Storage
  • Translate
  • Vision
• Documentation site support - for now we're going to rely on examples.

---

Testing Locally

To test locally you need to link the @google-cloud/common package to all other modules. To do so you can run

$ npm run link-common

Several modules also have a dependency on @google-cloud/storage so additional links may be required.

$ cd packages/storage
$ npm link
$ cd ../bigquery
$ npm link @google-cloud/storage

---

Rollout Plan

• Merge @google-cloud/common streaming and promise updates. (common: add promise support #1693)
• Release @google-cloud/common 0.7.0
• Merge @google-cloud/storage streaming and promise updates. (storage: add promise support #1697)
• Release @google-cloud/storage 0.3.0
• Merge @google-cloud/pubsub and @google-cloud/bigquery updates.
• Release @google-cloud/pubsub and @google-cloud/bigquery updates
• Merge everything else!
  • Bigtable (bigtable: add promise support #1713)
  • Compute (compute: add promise support #1703)
  • Datastore (datastore: add promise support #1704)
  • DNS (dns: add promise support #1705)
  • Language (language: add promise support #1706)
  • Logging (logging: add promise support #1707)
  • Prediction (prediction: add promise support #1708)
  • Resource (resource: add promise support #1709)
  • Speech (speech: add promise support #1710)
  • Translate (translate: add promise support #1711)
  • Vision (vision: add promise support #1712)
• Release everything else!
  • Bigtable
  • Compute
  • Datastore
  • DNS
  • Language
  • Logging
  • Prediction
  • Resource
  • Speech
  • Translate
  • Vision
• Merge umbrella package updates (umbrella: add promise support #1714)
• Release umbrella package

[stephenplusplus]

Go team promises, go!

[jasonswearingen]

re:

"This PR aims to remove callbacks and replace them with Promises."

if possible you can allow [optional] callbacks and return Promises, as that will help users with migration.

[stephenplusplus]

@jasonswearingen -- we talked about this starting here: #551 (comment) and landed on just promises.

[stephenplusplus]

@callmehiphop I think we should update this with the state of the world now. We are no longer removing callback support, right?

[callmehiphop]

@stephenplusplus that is correct, the plan is now to move streams behind a stream property which will then allow us to safely return a promise if the callback is omitted.

// stream mode
bucket.getFiles({ stream: true })
  .on('data', function() {});

// promise mode
bucket.getFiles()
  .then(function() {});

// callback mode
bucket.getFiles(function() {});

I'll update the PR description accordingly.

[huan]

It's so great to see this PR after read the long ISSUE #551

Do we have a date of when the gcloud-node with promise support will be published on NPM? I can't waiting for it. 👍

[jgeewax]

@callmehiphop , I'm a bit worried about the { stream: true } syntax. Is there anyway we can make this a bit more readable, maybe...

// Promise mode
bucket.getFiles()
  .then(function(files) { });

// Maybe we can let people create streams from promises (?)
bucket.getFiles().createStream()
  .on('data', function(file) { });

// Or if we can make it even shorter, promises also act like streams?
bucket.getFiles()
  .on('data', function(file) { });

[callmehiphop]

@zixia I share your enthusiasm! Hopefully we'll get Promises published within the next couple of months.

@jgeewax I can't help but agree that { stream: true } isn't the most elegant solution. I originally wanted to do something similar to your second example, but it meant that we had to streamify our methods at object creation time, which I think both @stephenplusplus and I didn't feel was very practical.

The third example is pretty interesting, I'm not aware of any objects that inherit from both Promises and Streams. However, it wouldn't be very difficult to concat the stream data together and resolve via Promises - there are already several packages that do this.

@stephenplusplus what are your feelings on decorating the stream object we return with a then method that essentially returns a promise and calls concat-stream under the hood?

[jgeewax]

@callmehiphop : The only issue I can think of here is what happens when you consume stuff with a stream and then decide you want a promise?

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

What should that do? Error somewhere? or is it technically valid?

[callmehiphop]

I just tested that, technically it's valid and does not throw an error. We could unbind events if a user adds a promise, assuming that's a concern.

[stephenplusplus]

what are your feelings on decorating the stream object we return with a then method that essentially returns a promise and calls concat-stream under the hood?

This?

typeof bucket.getFiles(callback) // undefined
typeof bucket.getFiles() // Stream
typeof bucket.getFiles().then() // Promise

Aren't there more Promise methods, like catch (maybe others?). Are those available as well via bucket.getFiles().catch()?

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

I think this could definitely lead to confusion, but where there is flexibility, that's a known side effect. This is up to @callmehiphop's intended implementation, but it could either start two requests to fetch fresh data, or fight with each other for which stream (the one the user is holding or the internal concat stream we run when it's a promise) to receive each result from the API.

[callmehiphop]

Aren't there more Promise methods, like catch (maybe others?). Are those available as well via bucket.getFiles().catch()?

Currently the only methods on the Promise prototype chain are then and catch. However catch is pretty much just a convenience method for calling then with a null value in place of the success callback.

That does raise a valid concern that this approach would not be very future proof if additional methods were introduced.

[callmehiphop]

but it could either start two requests to fetch fresh data

That would be bad, the idea would be to re-use the already existing stream.

or fight with each other for which stream (the one the user is holding or the internal concat stream we run when it's a promise) to receive each result from the API.

Technically the results would be emitted to both, so I'm not sure I understand how they would be fighting with each other?

If it helps to illustrate how it works, locally (for testing) I added this to the runAsStream_ method.

  stream.then = function(success, failure) {
    return new Promise(function(resolve, reject) {
      stream.on('error', reject).pipe(concat(resolve));
    }).then(success, failure);
  };

[stephenplusplus]

Sure it would be an extra request, but since the user is asking for a stream and a promise, it might be expected.

I believe it would conflict when more complicated stream use cases are considered, like the user isn't just draining a stream with on(data) listeners, but manipulating or throttling the incoming messages.

[callmehiphop]

I think trying to accommodate multiple interfaces in this fashion is bad idea. Which unfortunately doesn't make any of those examples very practical for us. An alternative that we haven't really discussed is making a separate method for the streaming versions

// callback
bucket.getFiles(function() {});

// promise
bucket.getFiles().then(function() {});

// stream
bucket.getFilesStream().on('data', function(file) {});

@stephenplusplus @jgeewax not sure what your feelings on an approach like this are - IMO the only real downside here is that the documentation would not be consolidated like it is today, but that might actually be a selling point since the user would no longer have to search through several examples to see what the streaming syntax looks like.

[stephenplusplus]

I do like the docs simplification part. What about declaring up front, at
the time of requiring gcloud, that we want the streams api or the promise
of? Then the methods don't have to support both. If the user needs both in
the same app (really, though?), they could re-instantiate gcloud or at the
service level, override their global preference.

On Thu, May 19, 2016, 10:47 AM Dave Gramlich notifications@github.com
wrote:

I think trying to accommodate multiple interfaces in this fashion is bad
idea. Which unfortunately doesn't make any of those examples very practical
for us. An alternative that we haven't really discussed is making a
separate method for the streaming versions

// callbackbucket.getFiles(function() {});
// promisebucket.getFiles().then(function() {});
// streambucket.getFilesStream().on('data', function(file) {});

@stephenplusplus https://github.com/stephenplusplus @jgeewax
https://github.com/jgeewax not sure what your feelings on an approach
like this are - IMO the only real downside here is that the documentation
would not be consolidated like it is today, but that might actually be a
selling point since the user would no longer have to search through several
examples to see what the streaming syntax looks like.

—
You are receiving this because you were mentioned.

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

[jgeewax]

I was going to suggest this one also, but had trouble with the sS, and figured that since we combine callbacks and streams with the same API call, it seems pretty reasonable to do the same with promises and streams. I also assumed that if we were going to do this, it seemed kind of reasonable to offer the .createStream() method...

@stephenplusplus : I'm not a huge fan of forcing folks to determine their usage at the start... that seems... scary.

What if we tried doing this the same that fs does something like this?

// callback
bucket.getFiles(function() {});

// promise
bucket.getFiles().then(function() {});

// stream
gcloud.createReadStream(bucket.getFiles())
  .on('data', function(file) { });

[callmehiphop]

since we combine callbacks and streams with the same API call, it seems pretty reasonable to do the same with promises and streams.

When dealing with callbacks and Streams it's really obvious what a user wants, since they have to supply us with the actual callback. With Promises and Streams however, we don't have anything that allows us to distinguish one from the other - hence why we were suggesting { stream: true }.

I also assumed that if we were going to do this, it seemed kind of reasonable to offer the .createStream() method...

This is a bit more feasible than trying to combine Streams with Promises, however I think we might come across some of the same pitfalls. This does have the added benefit of not stubbing Promise methods though.

From a technical standpoint, I see two options to achieve this:

1. Create a request and return a promise, if createStream() is called, abort the previous request made and create a new request with a stream. I think the issue here is that we would potentially be making multiple requests, which is more $ for our users.
2. Create the stream up front and return a promise that resolves via stream-concating under the hood. When createStream() is called, we simply return the pre-existing stream. I think this might have the potential to get weird in the same ways that combining Streams and Promises got weird, in that the user could still use both APIs simultaneously.

e.g.

var promise = bucket.getFiles();
var stream = promise.createStream();

promise.then(function(files) {});
stream.on('data', function(file) {});

We could unpipe the stream (I think) so that the promise never resolves, but I think that might be even more confusing.

What if we tried doing this the same that fs does

IMO this would have to be implemented identically to createStream(), with some kind of Promise/Stream hybrid.

As for getFilesStream we could rename it to be something more intuitive createFileStream and simply reference it in getFiles documentation. This has the added benefit of being able to live on the prototype chain and giving us a clear indication of what the developer is trying to accomplish.

[stephenplusplus]

@jgeewax what's scary about the streams: true option?

I think mixing streams and promises is confusing... even if we figure out how to solve it, the user has to figure out what we figured out, y'know what I mean?

Here's my personal preference of what is the least intrusive on the user, and the most predictable:

1. require('gcloud')({ streams: true }) (or some other name for basically "I don't need promises") -- promises will be the default
2. bucket.getFiles({ stream: true }) -- @jgeewax why don't you like this option?
3. bucket.getFilesStream() -- regarding this naming, I'm totally fine with smushing s's. It's comforting for a user to know that "all I have to do is append 'Stream' and it'll work", without having to relearn our API with a new method or combinator

[callmehiphop]

require('gcloud')({ streams: true }) (or some other name for basically "I don't need promises") -- promises will be the default

I'm not a big fan of this. Lets assume a dev would want to use both streams and promises in the same app, they would either have to go back and forth between setting that flag or manage multiple objects that are more or less the same. I think this might actually be more confusing than trying to make a Stream/Promise hybrid.

[stephenplusplus]

Maybe it can be treated as a global setting of stream: true, where the most local setting is on the method itself. So it's like saying, "I'm not using promises this time... or next time... or the time after that... so let me just tell you one time I'm not going to use the promise option... ever."

[jasonswearingen]

I skimmed the recent posts, and I think an explicit somemethodStream() is the way to go if someone needs streams. It's a bad idea to inject streaming features/functions into Promises. Separation of concerns!

also, I can imagine a scenario where you mostly use promises, but in some cases need to use streams. For example: I use promises with datastore right now (a wrapper I made) and it works great, except if I have a datastore query that returns tons of results. for that I'd use streams.

regarding some global stream:true flag, if I'm understanding it correctly, that sounds like a bad idea to me as it effectively changes the function signatures based on a runtime variable. I know javascript isn't explicitly typed, but this seems like it's abusing that "feature".

[stephenplusplus]

Thanks, @jasonswearingen. I think we should sit back until we have an implentation to review and let @callmehiphop get started on his ideal vision. This is his baby, and I trust him fully to deliver.

[coveralls]

Changes Unknown when pulling f1ff315 on promise-support into * on master*.

[c0b]

for how long can we see a Promise API?

[callmehiphop]

@c0b I don't think this is our top priority at the moment, so it's hard to nail down an accurate estimate, but I would guess sometime within the next several months.

[jmdobry]

Looking at the implementation of gax-nodejs, I'm thinking there would be a rather unfortunate loss of information/loss of functionality if the hand-written methods were to return a Promise, as opposed to returning the event emitter. For example:

var Language = require('@google-cloud/language');

var language = Language();
var api = Language.v1beta1();

var document = {
  type: 'PLAIN_TEXT',
  content: 'Hello, world!'
};

// The hand-written method returns just a promise
var promise = language.detectSentiment(document.content);

// gax-nodejs returns an event emitter
var apiRequest = api.analyzeSentiment(document);

try {
  // uh oh, how to I cancel the request
  promise.cancel();
} catch (err) {
  // Uncaught TypeError: promise.cancel is not a function
}

// Nice, this works
apiRequest.cancel();

promise
  .then((sentiment) => {
    // ...
  })
  .catch((err) => {
    // ...
  });

// This works today
apiRequest.result
  .then((response) => {
    // ...
  })
  .catch((err) => {
    // ...
  });

I'm thinking the hand-written methods should just return the event emitter returned by gax-nodejs:

var languageRequest = language.detectSentiment(document.content);
var apiRequest = api.analyzeSentiment(document);

// Yay, I can cancel the request!
languageRequest.cancel();

// Nice, this works
apiRequest.cancel();

// Access the promise on the "result" property
languageRequest.result
  .then((sentiment) => {
    // ...
  })
  .catch((err) => {
    // ...
  });

// This works today
apiRequest.result
  .then((response) => {
    // ...
  })
  .catch((err) => {
    // ...
  });

Use the event emitter in combination with callback-style:

var languageRequest = language.detectSentiment(document.content, function (err, sentiment) {
  // ...
});
var apiRequest = api.analyzeSentiment(document, function (err, response) {
  // ...
});

// Yay, I can cancel the request!
languageRequest.cancel();

// Nice, this works
apiRequest.cancel();

For those methods that optionally return a stream, the method will have to be split into two methods:

Table#getRows => Table#getRows and Table#getRowsStream:

// callback-style
table.getRows(function (err, rows) {
  // ...
});

// promise-style
table.getRows().result.then(function (rows) {
  // ...
}).catch(function (err) {
  // ...
});

// stream
table.getRowsStream()
  .on('error', console.error)
  .on('data', function (row) {})
  .on('end', function () {
    // All rows have been retrieved.
  });

[stephenplusplus]

@callmehiphop can you update the TODOs to outline our roll out plan?

Also, just throwing these out:

• Where and how do we explain a user has multiple choices to run this? Yes, the method examples show it, but we might want to explain more globally that our library supports promises and callbacks. Maybe the README? An Overview doc?
• Should we update each API's README examples to show promises in use?

[callmehiphop]

can you update the TODOs to outline our roll out plan?

Done!

Where and how do we explain a user has multiple choices to run this?

I think obviously we should emphasize this in the release notes, otherwise I'm not too sure.. the main README is fine. I've updated the param list on the umbrella package as well showing how to integrate Bluebird, so it should also be available on the default docs page. Otherwise I'm not sure where else it fits?

Should we update each API's README examples to show promises in use?

Yar, I'll update the READMEs as I create PRs for each service.

[callmehiphop]

Promises have been released!

[sebelga]

great job guys! thanks!

[kevhill]

This is wonderful, just what I was looking for. I bet there are others who were looking for this but never found it because 'Promise' or '.then' are nowhere to be found in the readme.md and all of those examples use the callback style.

[stephenplusplus]

We'd like to fix this as well: #1699, #2167 -- PRs are very welcome :)

[kevhill]

Well, let me make sure I can use it correctly, then I'll give it a shot ;)