fix(types): use Metadata types for apiResponse

[JustinBeckwith]

@bcoe I'm a little torn on if this one should be semver major or not. We removed a config property on the constructor that should have never been exposed publicly. Technically we "fixed the glitch", but there's a chance (albeit a small one) someone was depending on the behavior. Would love a gut check :)

[codecov]

Codecov Report

Merging #277 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #277   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           3      3           
=====================================
  Hits            3      3

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95e7cf9...5181f71. Read the comment docs.

[stephenplusplus]

@JustinBeckwith is this the thing you meant we removed that shouldn't have been there? Did request ever actually exist on GoogleAuthOptions? I don't see it here: https://github.com/googleapis/google-auth-library-nodejs/blob/37bb8c7cd0a6501103274284d9cddd6816cc881e/src/auth/googleauth.ts

If it was just a pretend setting, then I would say non-breaking is okay, since it never had functionality anyway. Sorry if I missed something!

[JustinBeckwith]

Aye, that was the thing. This allowed the user to technically override the implementation of the request module used. We did this in other modules in a way that was totally internal, but for some reason it was exposed here publicly. Theoretically - someone could have been passing in the full request module here, hoping to use that instead of teeny-request. It's a stretch.

[stephenplusplus]

Sorry in advance, because I'm trying to follow along with the history of the changes

We removed the requestModule option from common a couple of months ago, which was a breaking change. So did this library actually intend to support the feature, but we forgot to remove the code here?

Here's the code as it is today in this repo:

const config = {
  baseUrl,
  scopes: ['https://www.googleapis.com/auth/cloud-platform'],
  packageJson: require('../../../package.json'),
  projectIdRequired: false,
  requestModule: teenyRequest as typeof r, // impenetrable! 
};

super(config, options);

So, while the user's options.request is passed to Service, it would have always been ignored in favor of the config.requestModule that we set. Here was the code in common, before we deleted it (source):

this.requestModule = config.requestModule;

const reqCfg = extend({}, config, {
  projectIdRequired: this.projectIdRequired,
  projectId: this.projectId,
  credentials: options.credentials,
  keyFile: options.keyFilename,
  email: options.email,
  token: options.token,
  request: this.requestModule,
  token: options.token
});

Admittedly, trying to follow along with request vs requestModule and options vs config and the ordering of assignments in the constructor of this module creates loopholes that I can't really find my way out of. But to me, it seems like even though we said we accepted options.request, it never did anything anyway-- especially since even if we had all of our settings correct, it wouldn't have made a difference since the feature was killed upstream in common.

[callmehiphop]

Isn't Metadata just an alias to any? It would be great to offer actual type safety on these objects. Can we generate types like we do with other libs?

[JustinBeckwith]

Aye, it is just an alias to any. Is there an easy way to get the types for the requests/responses here? I seem to remember you crafting a library for this at one point :)

At any right - this was less wrong than what we have currently 😆

[callmehiphop]

Oh! I didn't realize this still used the REST API. discovery-tsd is published to npm and we currently use it in BigQuery.

[bcoe]

I'm torn as well ...

Given that 4.x.x has only been out the door for a week (so it probably doesn't have a huge user-base yet, I think you could argue dropping the leaky option requestModule should have been part of the last major.

Here's a compromise, what if you release this as a patch, but edit the CHANGELOG manually, and add dropping this option to the list of breaking changes in 4.x.x?