Returning arrays for everything makes for awkward and confusing code

[esprehn]

Environment details

• google-cloud-node version: 1.2.0

Steps to reproduce

1. require google-cloud
2. Do lots of operations using the API
3. Need to unpack an array with a [thing, ApiResponse] from every promise.

ex. Almost every API looks like this:

    download(options?: DownloadOptions): Promise<[Buffer]>;
    exists(): Promise<[boolean]>;
    get(): Promise<[File, ApiResponse]>;
    getMetadata(): Promise<[FileMetadata, ApiResponse]>;

but that means you end up doing [0] all over the place. The extra ApiResponse should be returned through some out of band system (ex. another argument that requests it, or a separate callback) instead of in an array with every response (and sometimes it seems there's an array even when there is no other data returned.

I end up having to write wrappers around every Google Cloud API function like this:

const fileExists = async (file: gcs.File) => {
  return (await file.exists())[0];
};

since doing:

if (!(await file.exists())[0])) ...

is hard to read and confusing. In general all the arrays all over the API don't feel like idiomatic Promise code. You end up leaving comments all over explaining what the arrays are about.

[stephenplusplus]

Do you have destructuring available in your code base?

file.exists()
  .then([exists] => {})

There is also bluebird.spread for Bluebird users.

The reason we always return an array is to be consistent, so you're not checking the docs for each method to determine which returns an array and which doesn't.

We always provide the raw API response when it's available, because we can't be sure our simplified version is relevant to the user. We learned early on that deciding for them was a mistake, which is why it is always available from the same place and at the same time.

For reference, here are related issues where this has been decided and discussed:

• Why don't we use promises in our API? #551 (comment)
• Include overview documentation on all API pages, and talk about Promises/Callbacks #2167

edit: updated example snippet

[esprehn]

We use async and await instead of callbacks. Having to write:

const [exists] = await file.exists(name);
if (exists) {
 // ...
}

Is a lot more noise than:

if (await file.exists(name)) {
 // ...
}

Yes we could do:

if (await file.exists(name).then([exists] => exists))

But that's getting even less idiomatic.

The guiding principles of promises + async/ await is that code should look like natural sync code with some await keywords sprinkled into it.

In general the cloud apis feel awkward because of the arrays. This pattern doesn't match how the web platform or how node apis usually work.

A more idiomatic pattern would have been to have separate apis for the lower level return types (exists and existsWithApi()), or have a separate callback for it (callback, lowLevelCallback), or have an argument that opts into getting the ApiResponse in addition to the simpler result (exists(..., {withApi: true}).

As it stands right now we never want it in our code and need to work around it by building up wrapper libraries around cloud which is unfortunate. The common case definitely seems to be to not want the extra values, so you end up unwrapping [0] in 99% of cases.

The principle we followed in the web apis was "allow the hard cases, favor the common ones". The current api honors the "allow the hard cases", but doesn't favor the common ones.

Fwiw I totally understand the problem you're having here. We debated it lots too. :)

(Anyway that's my 2c as someone who designed lots of the web platform's apis.)

[stephenplusplus]

Thanks for the input and that motto is repeated throughout our discussions as well. In our case, we have repeatedly been a frustration for users when we do any level of hiding the raw data/not allowing raw input. While I might think users want the shorthand version, they actually seem to appreciate the full thing.

I'm always interested in new solutions, however we are a bit past the point of being able to implement them, since we've reached GA with a few of our APIs. We would have to kick them to v2, which isn't impossible if we come up with something we all like.

Separate APIs is probably my preferred route from your suggestions. We are introducing the concept of a "partial veneer" (i.e. a small helper interface on top of a generated API client). These always expose each and every raw API method there is, and the I/O to them is without any decoration or simplification.

However, that won't be all of our APIs. Some need a thicker layer, like Datastore, Storage, and Spanner. They will have many custom methods, and duplicating them instead of just providing an extra argument feels heavy-handed. And accepting a flag that changes the response argument signature is a concept that gets a lot of rejection from my peers on this project.

Just my thoughts, I'm curious what the others think. Let's re-open and see where we get!

@callmehiphop @lukesneeringer

[lukesneeringer]

I'm always interested in new solutions, however we are a bit past the point of being able to implement them, since we've reached GA with a few of our APIs. We would have to kick them to v2, which isn't impossible if we come up with something we all like.

Before I say anything else, a thought about v2's timeline: It is considered a semver-major change to drop support for old versions of Node, and it is my desire to drop Node 4 when it goes EOL in April 2018. So, I am up for making a change like this in April, but not before that. (The leadup to that change, of course, could be significant.)

I am on the record as believing that returning an array all the time was a mistake. I understand why it was done, but returning an object with meaningful keys in the cases where multiple items were returned would have made more sense. (In cases where multiple items of the same type were returned, {results: []} could have been used.) As noted, though, this ship as sailed for v1.

In our case, we have repeatedly been a frustration for users when we do any level of hiding the raw data/not allowing raw input. While I might think users want the shorthand version, they actually seem to appreciate the full thing.

Yeah, we very much want to move away from this practice. We should accept full requests (usually) and return full responses (always).

However, that won't be all of our APIs. Some need a thicker layer, like Datastore, Storage, and Spanner. They will have many custom methods, and duplicating them instead of just providing an extra argument feels heavy-handed. And accepting a flag that changes the response argument signature is a concept that gets a lot of rejection from my peers on this project.

What we can do in these cases is move (most of) them toward a partial Veneer model. I did this in Python for Pub/Sub, actually (!!). Realistically, most of the GAPIC methods in even Pub/Sub are entirely sufficient; it is only the banner methods like publish and subscribe that need thick layers over them to handle the concurrency model.

So, it might be reasonable to have Spanner v2 be a much, much thinner client, and only override things where we absolutely need to. Where that will not work well is APIs like BigQuery that have no GAPIC layer.

There is also a question of time. Making this change would be pretty enormous, and will require keeping both versions of the codebase in sync from whenever we start (probably January or so) to April 2019, as we have to support the v1s for a year after v2 comes out. So, this is a significant undertaking if we decide to do it.

[lukesneeringer]

Note that I do not want this to evolve into a "reinvent everything" discussion. It is worth getting ideas out, and then after that we need to close this issue. If we want to have a desired action item, I will have to see about getting it in the priority queue in early 2018.

[garcianavalon]

My two cents as a user, object instead of array would be nicer as is more readable

[stephenplusplus]

@callmehiphop thoughts here would be great :)

[callmehiphop]

I think we could argue if objects or arrays are better all day, IMO with destructuring it doesn't really matter. Even if we returned objects we would still hit the same problems being described here.

The real issue that needs to be discussed is how do we accommodate users who want slimmed down responses as well as users who don't want us to hide anything from them. I think the only reliable way to accomplish something like this would be to provide both a high and low level method for each RPC.

[stephenplusplus]

That approach makes sense to me, unless there's something fancier we could do to avoid the headache, e.g.:

const buckets = await storage.getBuckets()
const apiResponse = buckets[storage.API_RESPONSE_SYMBOL] // naming TBD

In the case of bucket.exists(), it would ideally only resolve to true. For other methods like that, it might make sense to simply not return any extra arguments. exists() is a helper method, as opposed to an API one, and we might be over-complicating/un-helpifying it by adding extra flavor to it. If there are other cases like those, where apiResponse is overkill, and/or the resolved value is a primitive (not something we can decorate with an apiResponse accessor), it might be better to leave it out, or create a duplicate method as suggested above, if we really have to.

Stream methods will always emit response when possible, so we don't have to worry about those. Otherwise, I believe we are always returning types that we can add the apiResponse symbol to.

Hopefully I'm not too far off track with that thought process.

[callmehiphop]

IMO if using symbols work we should totally do it, however I am unsure if we can accommodate all RPCs using that method, although I would be fine with just not supporting it for certain methods (like exists).

[kolodny]

Considering that most users probably wouldn't want the array behavior, what about adding some suffix to api calls for the fuller response?

const googleAPI = require('some-google-library');
// for most users
if (await file.exists()) {
  // ...
}

// for "power" users
const googleAPI = require('some-google-library');
const [exists, apiResponse] = await file.existsFull()
if (await file.exists()) {
  // ...
}

This is similar to how Bluebird.promisifyAll works by adding an Async to every method on an object. The Full suffix is just a placeholder for a better name

[JustinBeckwith]

For those interested, I want to lay out a plan of how we are thinking of approaching it. Thoughts welcomed!

1. We add new Async methods for all functions that return a Promise<Array<T>>. For example, bucket.exists() becomes bucket.existsAsync(). We make this change across all of the available libraries.

2. We update all of the documentation and samples for all the libraries to point folks towards the new Async variants.

3. We add process.emitWarning warnings for anyone using the old functions in a way that would return an array of values from the promise.

4. We wait for a year.

5. We remove the support for promise-arrays from the non-Async methods, and only support callbacks.

6. We wait another year 😱

7. We add support for async with the original methods back. We update all the docs, and the samples again, back to the original method, but with the new syntax.

8. We add deprecation notices to the Async methods.

9. We wait another year.

10. We remove the Async methods.

Thoughts? @ofrobots @crwilcox @alexander-fenster @kinwa91 @stephenplusplus @callmehiphop

[ofrobots]

SGTM, with the 'year' replaced with ${appropriateSignificantAmountOfTime}. I would suggest ½ year, as that is what we use in Node.js core for semver majors.

For clarity, it would be good to edit your post to clearly indicate which steps are semver majors. Is adding a process.emitWarning semver major, for example?

[ofrobots]

My personal opinion, and precedent from Node.js core is that adding a process.emitWarning is semver major.

[stephenplusplus]

@JustinBeckwith now that we aren't as scared of semver-major bumps anymore, does that change your preferred approach from your last suggestion?

[JustinBeckwith]

Closing this as a dup of #2896 🙃