google-cloud-node: Returning arrays for everything makes for awkward and confusing code
Environment details
- google-cloud-node version: 1.2.0
Steps to reproduce
- require
google-cloud
- Do lots of operations using the API
- 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.
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Reactions: 1
- Comments: 16 (14 by maintainers)
For those interested, I want to lay out a plan of how we are thinking of approaching it. Thoughts welcomed!
We add new
Async
methods for all functions that return aPromise<Array<T>>
. For example,bucket.exists()
becomesbucket.existsAsync()
. We make this change across all of the available libraries.We update all of the documentation and samples for all the libraries to point folks towards the new
Async
variants.We add
process.emitWarning
warnings for anyone using the old functions in a way that would return an array of values from the promise.We wait for a year.
We remove the support for promise-arrays from the non-
Async
methods, and only support callbacks.We wait another year 😱
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.We add deprecation notices to the
Async
methods.We wait another year.
We remove the
Async
methods.Thoughts? @ofrobots @crwilcox @alexander-fenster @kinwa91 @stephenplusplus @callmehiphop
@JustinBeckwith now that we aren’t as scared of semver-major bumps anymore, does that change your preferred approach from your last suggestion?