feat: add typing for request params

[JustinBeckwith]

Scroll to the bottom for the good part 🐱

[acchou]

This is exactly what I've been looking for... and was about to investigate working on :) Thanks and hopefully it can be vetted and merged soon.

[JustinBeckwith]

@google/google-node-team what, nobody wants to review my 300,000 line change?

[acchou]

I took a look at the generated types and have a few comments. Mostly I was reviewing the compute api v1. Consider compute.instances.insert and the parameter type Params$Resource$Instances$Insert:

export interface Params$Resource$Instances$Insert {
    auth?: string|OAuth2Client|JWT|Compute|UserRefreshClient;
    project: string;
    requestId?: string;
    sourceInstanceTemplate?: string;
    zone: string;
    resource?: Schema$Instance;
  }

project and zone could be set at the global or api level so should probably be optional?

The resource of type Schema$Instance is optional, though it appears to be required. Looking at its type:

export interface Schema$Instance {
    ...
    /**
     * [Output Only] The CPU platform used by this instance.
     */
    cpuPlatform?: string;

Not sure if there's a way to separate out output-only fields that clutter this type when using it as an input. Using Pick<T,K> is not going to cut it for nested objects, so unfortunately doing this might require generating a whole other set of types. So probably overkill...

On the other hand, having unified input and output Schema types means making pretty much all fields need to be optional, even if the responses from the API always have certain fields. This makes using the results somewhat less convenient because it will be hard to tell where error checks are really needed. I expect to see client code littered with !'s and/or checks for missing fields that can't actually be missing. Splitting it into input and output instances might look more like:

export interface Schema$Instance {
   cpuPlatform: string;   // not optional, always returned by server
   name: string;  // always returned, not optional.
   ...
}
export interface Schema$InstanceInsert {
    // cpuPlatform isn't even mentioned here because it's output-only
    name: string;   // required for creating an instance
    ...
}
export interface Params$Resource$Instances$Insert {
   resource: Schema$InstanceInsert;   // Not optional, required to create an instance
   ...
}

Of course this is only possible if the source API definition file has the detail required. Maybe this is reaching for the stars... I'd be very happy with just the types you already have generated.

[JustinBeckwith]

This is really great feedback.

project and zone could be set at the global or api level so should probably be optional?

Yeah - that's really tough. If we go down that road, I just need to make every property optional, and do required field checking at runtime (which I need to do anyway). The tradeoff here is that you won't really know what's required or not statically. I could really go either way on this one 🤷‍♂️ Totally open for debate.

The resource of type Schema$Instance is optional, though it appears to be required.

Sadly, this is one of those places where I'm limited by what information is available in the discovery docs I'm using to generate these APIs. Lets use compute.instances.insert as an example:
https://www.googleapis.com/discovery/v1/apis/compute/v1/rest

The discovery doc tells me that the body of the request can take an Instance object, but doesn't tell me if it's required. The schema object it points to also doesn't give me any information on which fields within that object are actually required either. Sadly, this is one of those places where I just don't have enough information to do what I'd like.

[acchou]

Yeah - that's really tough. If we go down that road, I just need to make every property optional, and do required field checking at runtime (which I need to do anyway). The tradeoff here is that you won't really know what's required or not statically. I could really go either way on this one 🤷‍♂️ Totally open for debate.

One argument for making them optional is that it's idiomatic usage to specify the zone and project at the global or api level in JavaScript. Making it a static type error for TypeScript means anyone migrating from JS to TS gets a spurious type error on almost every api call.

The biggest value in having types comes from type safety and convenience for autocomplete. The latter is what really helps discoverability and what most developers are after for this API. Type safety is nice too but it's already unreliable in this API because of the missing detail in the schema file. So my opinion is that you might as well shoot for convenience all the way, instead of a haphazard mix of the two. Runtime checks are a good enough fallback for the lack of type safety, IMHO.

Sadly, this is one of those places where I'm limited by what information is available in the discovery docs I'm using to generate these APIs.

The discovery doc appears to be itself generated from something else... perhaps a protobuf definition? I ask because there are comments like "[Output Only]" which are used in a somewhat mechanically consistent way -- perhaps something in the original source definition format that wasn't expressed in the JSON? In any case thanks for checking.

[JustinBeckwith]

You've changed my mind :) I made all the params optional. On how the discovery doc is generated... I'm not sure! I will find out.

[acchou]

Awesome. Really looking forward to using these APIs with types.

[alexander-fenster]

@acchou re: compute - we actually have another Node.js client with manually written (not generated) methods, want to check it out? @google-cloud/compute

[alexander-fenster]

(but it's plain JavaScript, no TypeScript yet while it's planned :) )

[JustinBeckwith]

Thanks for the vibrant discussion and ideas folks! I think we got it :) Take a look and let me know what you think.

[acchou]

Looks good to me. Thanks for taking this one step further to make it more convenient for clients of the api.