core: adding promise support

[callmehiphop]

Waiting on #1665

When it comes time to ship this we'll have to ship the code out in phases. To test this locally you'll need to run

$ npm run link-common

To run all the system tests, additionally links may need to be made where modules have devDependencies on modules like Storage/PubSub/etc.

[stephenplusplus]

@callmehiphop should we close #1142?

[callmehiphop]

@stephenplusplus I think this is the cleanest route to promisifiy our library. PTAL a let me know what you think about this basic implementation.

Assuming there are no red flags with the implementation, my next question would be in regards to system tests - do we want to write a system test for every method that can leverage promises? I think this would almost double the number of system tests.

[callmehiphop]

should we close #1142?

Actually this PR is requesting to merge into that branch :)

[stephenplusplus]

Ah, fancy.

I think this implementation looks good. For system tests, I would say we don't have to system test, but we do need to smoke test / docs test. I think we're already planning on showing "@­examples" that use both callbacks and promises, right?

[callmehiphop]

I think we're already planning on showing "@­examples" that use both callbacks and promises, right?

Sure do, I plan to handle them in the same fashion as we used to document streams.

[stephenplusplus]

That should be good enough for me then to show that our promises work.

[callmehiphop]

@stephenplusplus I've updated a few of the examples, WDYT?

How do you want me to organize all the things? First commit is util.promisify() then each commit after is a single API? Or should I put the common stuff in a separate PR?

[stephenplusplus]

I'm not sure about the multiple params vs single args. That is:

// single
bigquery.createDataset()
  .then(function(dataset) {})

// multiple
bigquery.getDatasets()
  .then(function(response) {
    var dataset = response[0]
    var apiResponse = response[1]
  })

I know we're going to convert "operation-like" APIs (BigQuery Operation / gRPC operations) into promises (#1306), which I believe makes apiResponse the only reason we'll need multiple params (let me know if that's wrong).

If that's the case, I wonder if we can always return a single argument and make apiResponse accessible in a different way. Just an idea:

bigquery.getDatasets()
  .then(function(datasets) {
    console.log(this.apiResponse)
  })

[stephenplusplus]

How do you want me to organize all the things? First commit is util.promisify() then each commit after is a single API? Or should I put the common stuff in a separate PR?

Whatever is easiest for your workflow.

[callmehiphop]

I wonder if we can always return a single argument and make apiResponse accessible in a different way

There will still be scenarios where we have to return multiple arguments (autoPaginate: false). However we could either make the api response the context of the callback or we could make it a property of whatever we pass in? I think this is how we planned on handling errors.

bigquery.getDatasets()
  .then(function(datasets) {
    console.log(datasets.apiResponse);
  })
  .catch(function(err) {
    console.log(err.apiResponse);
  });

WDYT?

[stephenplusplus]

err.apiResponse is okay. I think we already have response available on ApiError objects, so we should stick with that for custom errors.

I don't want to collide any extra properties like apiResponse with the returned argument, as that might conflict with other data / unintentionally be logged/printed/looped over by the user.

Keeping in mind this is describing the less common use case of manual pagination & needing the apiResponse, I think this would be the best separation.

bigquery
  .getDatasets({ autoPaginate: false })
  .then(function(datasets) {
    var apiResponse = this.request.response
    var nextQuery = this.request.nextQuery

    if (nextQuery) {
      return bigquery.getDatasets(nextQuery)
    }
  })

So we're fighting between good UX for the common case or good UX for the uncommon. If multiple args combined in one object is completely fine and expected in an average promise workflow, then just ignore me on all of this. That does bother me, because "getDatasets()" sounds like it's going to return "datasets", not an object that has datasets. But the other bother is that we'll have some methods that return single and others that return multiple.

[callmehiphop]

If multiple args combined in one object is completely fine and expected in an average promise workflow, then just ignore me on all of this.

I think it is pretty common to take that approach, however we had previously decided that using Arrays + destructuring would be the way to go. Then in the event that a user is using Bluebird in some fashion, they can take advantage of the spread() method.

bigquery.getDatasets()
  .then(function([datasets, apiResponse]) {});

bigquery.getDatasets()
  .spread(function(datasets, apiResponse) {});

The only downside of this of course is that 0.12.x users still have to access items via index, however I'm not sure if object notation is really much better.

[stephenplusplus]

Okay, that makes sense. I forgot about destructuring making the lives of devs on modern stacks easier. It looks like we didn't get votes on arrays vs objects in that linked post. Can you show what the spread workflow looks like for someone using Bluebird? Also, why would they be using Bluebird in the first place-- does it do something we aren't doing with our promise support that is required to conform to a promise spec-- or is it just for features like spread that aren't supported by native promises?

[callmehiphop]

Can you show what the spread workflow looks like for someone using Bluebird

All spread does is transform an Array returned from an earlier call it separate parameters, very similarly to the spread operator. So assuming a user makes Bluebird the global Promise object (I don't think that's very uncommon).

global.Promise = require('bluebird');

bigquery.getDatasets({ autoPaginate: false })
  .spread(function(datasets, nextQuery, apiResponse) {});

however, even if they don't re-assign the Promise object, it's still plausible they'll use some of their convenience methods.

Bluebird.delay(1000)
  .then(function() {
    return bigquery.getDatasets({ autoPaginate: false });
  })
  .spread(function(datasets, nextQuery, apiResponse) {
    // ...
  });

Also, why would they be using Bluebird in the first place-- does it do something we aren't doing with our promise support that is required to conform to a promise spec-- or is it just for features like spread that aren't supported by native promises?

I would guess the latter. It's also possible that they aren't using Bluebird with our library specifically, but perhaps with another as it offers promisify() methods of it's own.

[stephenplusplus]

Ah, okay. So would you lean towards using the array syntax instead of the boxed in object so that Bluebird users are happy?

[callmehiphop]

So would you lean towards using the array syntax instead of the boxed in object so that Bluebird users are happy?

I think that's fair to say. If you think we're better off with the object syntax, I think I'll be pretty easily swayed. It's still destructurable and perhaps slightly better for users who can't take advantage of that feature yet.

However I think this is going to be a bit trickier to implement because we'll need to map each returned item to a specific key.

[stephenplusplus]

we'll need to map each returned item to a specific key.

Can you elaborate on that? I'm not sure I follow.

[callmehiphop]

Can you elaborate on that? I'm not sure I follow.

Certainly! I think what I'm trying to say is that using an object format will require more code changes. Our methods would probably have to resemble something like this..

BigQuery.prototype.getDatasets = function(query, callback) {
  var that = this;

  if (is.fn(query)) {
    callback = query;
    query = {};
  }

  query = query || {};

  this.request({
    uri: '/datasets',
    qs: query
  }, function(err, resp) {
    var response = {
      apiResponse: resp
    };

    if (err) {
      callback(err, response);
      return;
    }

    response.nextQuery = null;

    if (resp.nextPageToken) {
      response.nextQuery = extend({}, query, {
        pageToken: resp.nextPageToken
      });
    }

    response.datasets = (resp.datasets || []).map(function(dataset) {
      var ds = that.dataset(dataset.datasetReference.datasetId);
      ds.metadata = dataset;
      return ds;
    });

    callback(null, response);
  });
};

Because we have no way of mapping objects to an appropriate key (datasets, nextQuery) otherwise.

[stephenplusplus]

Oh, right. Thanks :) Arrays it is! Seems to benefit the most amount of people.

[callmehiphop]

I've been looking through a lot of promisify libraries, seeing if any of them suited our needs and a common theme among them is specifying whether or not you want multiple arguments.

I think we can all agree that for older Node users who do not use some kind of Promise#spread convenience code, returning an Array is annoying, which makes me a little trepidatious to do so.

I think in majority of cases, the user is generally only going to care about the first argument. On rare occasion they may wish to control pagination themselves, but otherwise I can't really think of a scenario when they'll want multiple args. That being said, I think we should offer a similar parameter within our methods that would enhance the user experience.

bigquery.createDataset('my-dataset')
  .then(function(dataset) {});

bigquery.createDataset('my-dataset', { multiArgs: true })
  .then(function([dataset, apiResponse]) {});

In most cases we assign the apiResponse as the metadata field on the objects being returned, so I can't really think of a single scenario where the user would actually want to do this.

In the cases of paginated lists, we could treat autoPaginate: false in the same fashion.

bigquery.getDatasets()
  .then(function(datasets) {});

bigquery.getDatasets({ autoPaginate: false })
  .then(function([datasets, nextQuery, apiResponse]) {});

The only place this doesn't feel quite right is for APIs that return operations. However, I think those APIs may be changing sometime in future to return operations directly from a method as opposed to from within a callback. (#1306)

Thoughts?

/cc @jgeewax @omaray @jmdobry

[stephenplusplus]

In most cases we assign the apiResponse as the metadata field on the objects being returned, so I can't really think of a single scenario where the user would actually want to do this.

There are API calls like "query", "detectText", etc. where apiResponse is very useful to the power users because we "shut out" or simplify certain bits of the raw API response in our default/decorated response. Some users need that other stuff, which is why we have apiResponse at all. Historically, we implemented it after receiving "Hey, where's the ___ property?" then a month later "Okay, what about the ___ property?" It was easiest just to return the whole thing, untouched.

I'm 100% with multiArgs as an option and autoPaginate: false === multiArgs: true. I think it would be amazing if we figured out how to handle operations at the same time we're landing promise support. I think what we really want is to eliminate operations altogether and just execute the then callbacks when the operation is done.

[jmdobry]

We're going to make the event emitter a property of the promise right? In that case we need to return the promise regardless of whether the user provides a callback, because a user who passes callbacks might still want to access the event emitter, e.g. to cancel the request.

[stephenplusplus]

That's definitely relevant, because we've really just been thinking of the non-gapic APIs. I'm not sure what @callmehiphop thinks, but if we have to do something goofy with the UX just to return the event emitter, I'm still okay with not offering the EE through the handwritten layer.

[jmdobry]

I don't think we'd have to do anything goofy. If I recall the plan is for @jmuk to change gax-nodejs to return the promise with the event emitter attached.

[sindresorhus]

I don't think a multiArgs option here is a good user experience. You should be consistent with what you return. If apiResponse can sometimes be useful, then it should always be returned. If you don't like returning an array of arguments, you could always return an object, although I would strongly urge you to optimize for destructuring, many consumers are already on Node.js 6 and can take advantage of it. In reality, most consumers will be using Bluebird anyways, so they'll have Promise#spread even though they don't have destructuring.

Another approach could be to have a dedicated API to get the apiResponse. You could either embed the apiResponse in the dataset with a Symbol:

bigquery.getApiResponse = x => x[Symbol.ApiResponse];

bigquery.getApiResponse(dataset);
//=> the API response

Or use a Weak Map (probably best solution):

// the key is the `dataset` itself
bigquery.getApiResponse = x => bigquery._someApiResponseWeakMap.get(dataset);

bigquery.getApiResponse(dataset);
//=> the API response

[stephenplusplus]

@jgeewax @jmdobry @omaray

Do you guys think we should show error handling in our example docs, e.g.:

// If the callback is omitted, we'll return a Promise.

bigquery.createDataset('my-dataset')
  .then(function(data) {
    var dataset = data[0];
    var apiResponse = data[1];
  })
  .catch(function(err) {
    // err is an API error if one occurred.
  });

[callmehiphop]

IMO I think it's implied that if an error occurs it will be catchable via catch(func) or then(null, func).

[stephenplusplus]

Do settings like maxApiCalls and maxRetries still work when using promises?

[callmehiphop]

Do settings like maxApiCalls and maxRetries still work when using promises?

Sure do, I can add a system test or two for good measure/peace of mind.

[stephenplusplus]

Sure do, I can add a system test or two for good measure/peace of mind.

That's good news! I suppose that wouldn't hurt, since we randomly test stream functionality in places.

[callmehiphop]

@stephenplusplus I just pushed some updates.

• A few system tests (they live in BigQuery)
• Removed filter function in favor of exclude array.
• Updated logic/examples to always return arrays as opposed to single arguments.
• Option to allow user to override the Promise constructor.