Consistently handle gax call options across methods.

[callmehiphop]

Relates to googleapis/nodejs-pubsub#35

Something I consider a little bit of a code smell is how we've handled gax options. In some places it is its own parameter and in others it is a property of a parameter. This is because when we switched to gax we wanted to avoid surface changes as well as making the user specify null arguments. Since that time some of our policies have changed and it seems like we're ok with making surface breaking changes for good reasons. I think that this is an excellent reason to make surface breaking changes across our APIs.

I think the next logical question would be "How will the surface change?". The changes I'm going to propose would only affect methods that accept gax options in the form of a property, we would then make it its own parameter.

We accept it as a property in 2 different types of methods

1. create methods
2. paginated methods

Create Methods

Using PubSub#createSubscription as a create example, we often use a pattern where we initially accept a name (string) for the first parameter with an optional second parameter containing additional metadata for the resource in question. To avoid trying to inspect the various keys of the metadata or making the user pass in null it seemed simpler to just allow gax options to specified via gaxOpts property. I think we should change this pattern to make the first parameter either the name OR the metadata which would include the name. Effectively this would reduce the number of optional objects from 2 to 1, which would make checking for an optional call options parameter simple and clean.

await topic.createSubscription('my-sub', {ackDeadlineSeconds: 10});

would become

await topic.createSubscription('my-sub');
// or
await topic.createSubscription({name: 'my-sub', ackDeadlineSeconds: 10});

Paginated Methods

The second kind of method, paginated methods, are a bit trickier because technically they have no required parameters, which makes it harder to differentiate between gax options and request parameters. However AFAIK request options for lists generally only have 1 (sometimes 2) properties. My opinion is that it is reasonable to check for said properties to determine what kind of object it is.

Thoughts?

[callmehiphop]

/cc @googleapis/yoshi-nodejs

[stephenplusplus]

Change one sounds good. One hurdle might be when users accustomed to the "name, [options]" style upgrade to a version that re-purposes their "options" object into a "gaxOptions" object. On the surface, it might appear to still be working as intended, because we won't have any errors to throw. This is opposed to other breaking changes, where the code simply might not run if, for example, they try to call a method that no longer exists or provide a string where an object is required.

For change two, I'm on board as well, although it would be good to confirm what options those methods always share.

[callmehiphop]

@stephenplusplus We could always make a util method that whitelists gax options and use it to confirm the object in both types of methods. Upon initial release we could throw an error if the user attempts to pass metadata the old way and remove the error throwing at a later date.

[fhinkel]

Greetings, we're closing this due to inactivity. Please let us know if the issue needs to be reopened.