Skip to content
This repository was archived by the owner on Oct 17, 2023. It is now read-only.

fix(types): use Metadata types for apiResponse #277

Merged
merged 3 commits into from
May 29, 2019
Merged

fix(types): use Metadata types for apiResponse #277

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0eac54
fix(types): use Metadata types for apiResponse
JustinBeckwith May 23, 2019
21fb478
Merge branch 'master' into tips
JustinBeckwith May 28, 2019
5181f71
Merge branch 'master' into tips
JustinBeckwith May 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 16 additions & 25 deletions src/v2/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {Service, util} from '@google-cloud/common';
import {Service, util, Metadata} from '@google-cloud/common';
import {promisifyAll} from '@google-cloud/promisify';
import arrify = require('arrify');
import * as extend from 'extend';
Expand All @@ -21,8 +21,6 @@ import * as is from 'is';

const isHtml = require('is-html');
import {DecorateRequestOptions, BodyResponseCallback} from '@google-cloud/common/build/src/util';
import * as r from 'request';
import {teenyRequest} from 'teeny-request';

const PKG = require('../../../package.json');

Expand All @@ -34,7 +32,7 @@ export interface TranslateRequest {
}

export interface TranslateCallback<T extends string|string[]> {
(err: Error|null, translations?: T|null, apiResponse?: r.Response): void;
(err: Error|null, translations?: T|null, apiResponse?: Metadata): void;
}

export interface DetectResult {
Expand All @@ -44,7 +42,7 @@ export interface DetectResult {
}

export interface DetectCallback<T extends DetectResult|DetectResult[]> {
(err: Error|null, results?: T|null, apiResponse?: r.Response): void;
(err: Error|null, results?: T|null, apiResponse?: Metadata): void;
}

export interface LanguageResult {
Expand All @@ -54,14 +52,13 @@ export interface LanguageResult {

export interface GetLanguagesCallback {
(err: Error|null, results?: LanguageResult[]|null,
apiResponse?: r.Response): void;
apiResponse?: Metadata): void;
}

export interface TranslateConfig extends GoogleAuthOptions {
key?: string;
autoRetry?: boolean;
maxRetries?: number;
request?: typeof r;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand Down Expand Up @@ -135,19 +132,17 @@ export class Translate extends Service {
scopes: ['https://www.googleapis.com/auth/cloud-platform'],
packageJson: require('../../../package.json'),
projectIdRequired: false,
requestModule: teenyRequest as typeof r,
};

super(config, options);
this.options = options || {};
this.options.request = config.requestModule;
if (this.options.key) {
this.key = this.options.key;
}
}

detect(input: string): Promise<[DetectResult, r.Response]>;
detect(input: string[]): Promise<[DetectResult[], r.Response]>;
detect(input: string): Promise<[DetectResult, Metadata]>;
detect(input: string[]): Promise<[DetectResult[], Metadata]>;
detect(input: string, callback: DetectCallback<DetectResult>): void;
detect(input: string[], callback: DetectCallback<DetectResult[]>): void;
/**
Expand Down Expand Up @@ -236,8 +231,8 @@ export class Translate extends Service {
detect(
input: string|string[],
callback?: DetectCallback<DetectResult>|
DetectCallback<DetectResult[]>): void|Promise<[DetectResult, r.Response]>|
Promise<[DetectResult[], r.Response]> {
DetectCallback<DetectResult[]>): void|Promise<[DetectResult, Metadata]>|
Promise<[DetectResult[], Metadata]> {
const inputIsArray = Array.isArray(input);
input = arrify(input);
this.request(
Expand Down Expand Up @@ -275,7 +270,7 @@ export class Translate extends Service {
});
}

getLanguages(target?: string): Promise<[LanguageResult[], r.Response]>;
getLanguages(target?: string): Promise<[LanguageResult[], Metadata]>;
getLanguages(target: string, callback: GetLanguagesCallback): void;
getLanguages(callback: GetLanguagesCallback): void;
/**
Expand Down Expand Up @@ -318,7 +313,7 @@ export class Translate extends Service {
getLanguages(
targetOrCallback?: string|GetLanguagesCallback,
callback?: GetLanguagesCallback):
void|Promise<[LanguageResult[], r.Response]> {
void|Promise<[LanguageResult[], Metadata]> {
let target: string;
if (is.fn(targetOrCallback)) {
callback = targetOrCallback as GetLanguagesCallback;
Expand Down Expand Up @@ -356,11 +351,11 @@ export class Translate extends Service {
}

translate(input: string, options: TranslateRequest):
Promise<[string, r.Response]>;
Promise<[string, Metadata]>;
translate(input: string[], options: TranslateRequest):
Promise<[string[], r.Response]>;
translate(input: string, to: string): Promise<[string, r.Response]>;
translate(input: string[], to: string): Promise<[string[], r.Response]>;
Promise<[string[], Metadata]>;
translate(input: string, to: string): Promise<[string, Metadata]>;
translate(input: string[], to: string): Promise<[string[], Metadata]>;
translate(
input: string, options: TranslateRequest,
callback: TranslateCallback<string>): void;
Expand Down Expand Up @@ -473,7 +468,7 @@ export class Translate extends Service {
translate(
inputs: string|string[], optionsOrTo: string|TranslateRequest,
callback?: TranslateCallback<string>|TranslateCallback<string[]>):
void|Promise<[string, r.Response]>|Promise<[string[], r.Response]> {
void|Promise<[string, Metadata]>|Promise<[string[], Metadata]> {
const inputIsArray = Array.isArray(inputs);
const input = arrify(inputs);
let options: TranslateRequest = {};
Expand Down Expand Up @@ -533,9 +528,6 @@ export class Translate extends Service {
});
}

request(reqOpts: DecorateRequestOptions): Promise<r.Response>;
request(reqOpts: DecorateRequestOptions, callback: BodyResponseCallback):
void;
/**
* A custom request implementation. Requests to this API may optionally use an
* API key for an application, not a bearer token from a service account. This
Expand All @@ -547,8 +539,7 @@ export class Translate extends Service {
* @param {object} reqOpts - Request options that are passed to `request`.
* @param {function} callback - The callback function passed to `request`.
*/
request(reqOpts: DecorateRequestOptions, callback?: BodyResponseCallback):
void|Promise<r.Response> {
request(reqOpts: DecorateRequestOptions, callback: BodyResponseCallback): void {
if (!this.key) {
super.request(reqOpts, callback!);
return;
Expand Down