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

Include overview documentation on all API pages, and talk about Promises/Callbacks #2167

Closed
quentin-sommer opened this issue Mar 31, 2017 · 12 comments
Assignees
Labels
type: docs Improvement to the documentation for an API.

Comments

@quentin-sommer
Copy link

Environment details

  • OS: linux
  • Node.js version: 7.7.3
  • npm version: 4.4.4
  • google-cloud-node version: @google-cloud/datastore: 0.7.1

Steps to reproduce

  1. require @google-cloud/datastore
  2. get(keyObj)

Using a callback approach with 1 key
get(key, (err, entity) => {} will output an object

whereas using a promise approach with 1 key
get(key).then(entity => {}) will output a 1 sized array containing the object

@stephenplusplus
Copy link
Contributor

This is true for all of our methods, and that's why we do it-- for the consistency. Most of our methods return multiple arguments. datastore.get() is a rare exception. It's understandably confusing that we're using an array where only one value is expected, but we have to do it so users don't always have to confirm with our docs which methods resolve a promise with an array and which ones don't. Sorry for the confusion.

@stephenplusplus stephenplusplus added api: datastore Issues related to the Datastore API. type: question Request for information or clarification. Not an issue. labels Mar 31, 2017
@gunar
Copy link

gunar commented Apr 10, 2017

@stephenplusplus Hi there! A bit of feedback: this is confusing and hard to find an answer for. You should improve docs there.

Moreover, I don't think your answer addresses the question at all. Why doesn't the callback API return an array too then? That would consistent (although weird).

@stephenplusplus
Copy link
Contributor

We have two ways of using every method, and I think if we dual-documented every method, we'd end up confusing everybody. In our documentation, each method has an examples section. If it can be run as a promise, it will show the response arguments to the then handler. That's where we show that an array is returned.

Why doesn't the callback API return an array too then?

We're trying to be consistent for each user. A promise user should expect us to be consistent with how we return responses in promise mode, and a callback user should expect us to be consistent with how we return responses in callback mode. We can't be consistent between callback and promise because of the promise spec, which refuses multiple arguments to a promise's resolution. In other words, we can't do this:

method().then(function(items, apiResponse) {})

We have to do put them into a single structure of either an array or object

method().then(function(resp) { resp[0] === items && resp[1] === apiResponse })

You should improve docs there.

I want this, too. How?

@quentin-sommer
Copy link
Author

An introductory paragraph explaining the thing you just explained :) most libraries I've used in node return array for multiple results and objects for single results.

So I jumped to this conclusion and was confused

@callmehiphop
Copy link
Contributor

@stephenplusplus this brings up a good point - if we're deprecating the meta packages, perhaps we'll need to include instructions for things that would have typically belonged inside of the meta package overview everywhere else (e.g. interceptors, promises, config object, etc.)

@stephenplusplus
Copy link
Contributor

Good thinking, maybe it should be part of the Overview template.

@stephenplusplus stephenplusplus changed the title Inconsistency between cb and promise with @google-cloud/datastore nodejs Include overview documentation on all API pages, and talk about Promises/Callbacks Apr 12, 2017
@gunar
Copy link

gunar commented Apr 13, 2017

I see progress and I see no need for further commenting on my part.
Thank you very much and congrats on thorough feedback process.

@mrkevinze
Copy link

(Coming from #1768). I have developed a few apps on App Engine for two years but not on Node. In my current project, I am using the Node datastore library for the first time as I am integrating it with Cloud Functions beta.

As a new user to the library, I have to check the docs and tutorials on how to use different methods. I didn't realize that there was consistency for "promise users" and "callback users", and I didn't realize that promise result arguments were designed to be consistent across various APIs until I read this thread.

To put it in another way, as a new user, the point on consistency for promise arguments across different APIs was not that important for me. I spent some time debugging why my batch get only had one element when I iterated over the outer array. Looking back, the docs did show an example that showed the need to access the first element of the array first.

That being said, experienced users may find the consistency to be highly beneficial. Establishing consistency is valuable to library developers as well. Just my two cents. Thanks!

@DazWilkin
Copy link

DazWilkin commented Aug 9, 2017

I encountered this too and was confused until I read through this issue. Second the idea of adding some clarity in the documentation.

With regards @stephenplusplus comment about 'single structure of either an array or an object', why not then an object rather than array?

Instead of:

file.getMetadata().then(function(data) {
  var metadata = data[0];
  var apiResponse = data[1];
});

I could then:

file.getMetadata().then(function(data) {
  var metadata = data.metadata;
  var apiResponse = data.apiResponse;
});

@callmehiphop
Copy link
Contributor

@DazWilkin Ultimately we decided to go with arrays over objects to benefit users who use promise libraries that have a spread method.

@stephenplusplus stephenplusplus removed api: datastore Issues related to the Datastore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue. labels Feb 20, 2018
@JustinBeckwith JustinBeckwith added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. 🚨 This issue needs some love. type: docs Improvement to the documentation for an API. and removed type: enhancement labels Jun 13, 2018
@JustinBeckwith JustinBeckwith removed the 🚨 This issue needs some love. label Jun 25, 2018
@sduskis sduskis removed the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Dec 4, 2018
@sduskis
Copy link
Contributor

sduskis commented Dec 4, 2018

@callmehiphop, is there anything actionable on this issue?

@callmehiphop
Copy link
Contributor

@sduskis I'm going to say no, our documentation format has completely changed since the time this issue was raised and the new(er) format clearly documents that promises are returned. I'm going to close this unless @stephenplusplus thinks we shouldn't

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: docs Improvement to the documentation for an API.
Projects
None yet
Development

No branches or pull requests

10 participants