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

promise array results #2896

Closed
callmehiphop opened this issue Jan 22, 2019 · 3 comments
Closed

promise array results #2896

callmehiphop opened this issue Jan 22, 2019 · 3 comments
Labels
semver: major Hint for users that this is an API breaking change. status: won't fix type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@callmehiphop
Copy link
Contributor

The age old question - why do Promises always resolve with tuples (Arrays)?

There have been a few threads where we receive complaints about Promises resolving with tuples. More often than not the suggestion to resolve with objects is made, but personally I don't see why we would want to do that. With destructuring the end result is the same and tuples are slightly easier to work with.

My overall opinion is that if we want to do away with tuples, we should change all methods to not return the response and if there are use cases for the raw response data, we create an alternate version of the method that exposes it in some way. For some types of methods this is pretty easy, but with others not so much. Here's a possible solution for the various types of methods we have

resource creators

For resource creating methods I think it would be safe to just drop the response completely since it always gets set as the metadata property.

Before

const [bucket, apiResponse] = await storage.createBucket('my-bucket');

After

const bucket = await storage.createBucket('my-bucket');
const apiResponse = bucket.metadata; // if the user wants it

paginated lasts

For paginated lists I think we should split up the functionality into 2 separate methods.

  • a method for returning all resources (autoPaginate: true)
  • a method that returns a single page of resources (autoPaginate: false)

This has an added benefit of following the single responsibility principle since the current version changes the response tuple based on an option.

Before

const [buckets] = await storage.getBuckets();
// or
const [buckets, nextQuery, response] = await storage.getBuckets({autoPaginate: false});

After

type BucketList = Array<Bucket>;

interface BucketPage extends BucketList {
  nextQuery?: GetBucketsRequest;
  apiResponse: ListBucketsResponse
}

const buckets: BucketList = await storage.getAllBuckets();
// or
const buckets: BucketPage = await storage.getBuckets();
const {nextQuery, apiResponse} = buckets;

convenience methods

We have several convenience methods that essentially wrap other methods (exists -> getMetadata). I think we should ditch the response on these kinds of methods and add JSDoc {@link} tags to whatever is called under the hood.

Before

const [exists] = await bucket.exists();
if (exists) doSomething();

After

if (await bucket.exists()) doSomething();

operation methods

For resources that require a lro we should separate that logic into 2 different methods (BigQuery already does this).

  • a method for creating the operation
  • a method that creates an operation under the hood and taps into 'complete' to return the requested resource.

Since we would be returning 2 resources, each response would be available via metadata property.

Before

const [operation, instance] = await spanner.createInstance(instanceConfig);
await operation.promise();
// `instance` is ready to use

After

const operation = await spanner.createInstanceJob(instanceConfig);
const instance = await operation.promise();

// or
const instance = await spanner.createInstance(instanceConfig);

queries

Methods that perform a query often contain both data and statistics about the query itself. Unlike paginated lists, I feel like this information should always be returned.

Before

const [rows, stats] = await transaction.run('SELECT * FROM `MyTable`');

After

interface RowList extends Array<Row> {
  stats: RowStats
}

const rows: RowList = await transaction.run('SELECT * FROM `MyTable`');
const {stats} = rows;
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jan 23, 2019
@callmehiphop callmehiphop added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. semver: major Hint for users that this is an API breaking change. and removed triage me I really want to be triaged. labels Jan 23, 2019
@JustinBeckwith
Copy link
Contributor

For housekeeping purposes - should we combine this with #2556? Not sure which one to keep since you have all this lovely detail here :)

@bcoe
Copy link
Contributor

bcoe commented Jul 4, 2022

We haven't had many customer complaints about this issue in recent years. I'm of the opinion that array destructuring + fairly good sample coverage, has made the design decision of returning a tuple not too disruptive.

@bcoe bcoe closed this as completed Jul 4, 2022
@callmehiphop
Copy link
Contributor Author

callmehiphop commented Jul 4, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major Hint for users that this is an API breaking change. status: won't fix type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

4 participants