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

Promisified Prediction API. #1151

Closed
wants to merge 1 commit into from
Closed

Promisified Prediction API. #1151

wants to merge 1 commit into from

Conversation

jmdobry
Copy link
Contributor

@jmdobry jmdobry commented Mar 2, 2016

The purpose of this PR is to show how to promisify the gcloud-node API while still maintaining support for callbacks. See #551

npm test succeeds for me. I did not try the system tests (but I can, depending on how serious we are about this pull request).

Summary of changes

Breaking API changes

  • Service#request, the Prediction API methods, and the ServiceObject methods now only return a single errback value (which is more idiomatic, as opposed to also passing random positional success arguments to the callback). The positional argument (e.g. apiResponse) which was previously passed to the callback as a separate argument is now attached as a property of the single err argument which is now the only value returned in the event of an error/rejection. Now, in the event of an error/rejection, err is all you need to inspect.

Backwards compatible API changes

  • Service#request now returns a promise, but makes use of .asCallback to maintain support for callbacks.
  • The Prediction API methods now return promises, which make use of .asCallack to maintain support for callbacks.
  • The ServiceObject methods now return promises, which make use of .asCallback to maintain support for callbacks.
  • Added dependency on bluebird

Other

  • Refactored Prediction API and ServiceObject unit tests, which frequently mock Service#request, to take into account the new promisified methods.
  • Made a second version of a number of the refactored tests. One version interacts with the API via promises, and the other via callbacks, to show that the API now supports both styles.

Note: Many methods return multiple values. If you use callbacks, the multiple values will still be passed to your callback as multiple positional arguments. But if you use Promises, .then will receive an array containing the return values, so make use of .spread instead.

Note 2: My first commit did not do anything about streams, which still need to be addressed.

Note 3: I'm not sure I completely promisified ServiceObject, as I haven't spent enough time studying the code. In fact I'm sure I didn't promisify it completely, because I would have expected to have needed to refactor more tests than I did.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 2, 2016
@stephenplusplus
Copy link
Contributor

Awesome, thanks for doing this!

@callmehiphop
Copy link
Contributor

@jmdobry thanks for taking the time to put this together! I started to implement Promises via #1142, but with the recent decision to continue callback support I was a little thrown off/set back.

@stephenplusplus and I have been having a lot of discussions about promises and how we should go about implementing them into gcloud-node. One thing we decided on was not to use Bluebird, but instead a lightweight polyfill, although .spread() does make Bluebird very appealing.

@stephenplusplus
Copy link
Contributor

Thanks again for the PR @jmdobry. As @callmehiphop said, he's working on adding support in #1142 -- we'll be sure to come back and reference the changes here as we go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants