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

chore(deps): upgrade to gts v2 #232

Merged
merged 11 commits into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 1 addition & 1 deletion .eslintignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
**/node_modules
src/**/doc/*
**/.coverage
build/
docs/
protos/
3 changes: 3 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"extends": "./node_modules/gts"
}
15 changes: 0 additions & 15 deletions .eslintrc.yml

This file was deleted.

8 changes: 5 additions & 3 deletions .prettierignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
node_modules/*
samples/node_modules/*
src/**/doc/*
**/node_modules
**/.coverage
build/
docs/
protos/
8 changes: 0 additions & 8 deletions .prettierrc

This file was deleted.

3 changes: 3 additions & 0 deletions .prettierrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"singleQuote": true
}
8 changes: 5 additions & 3 deletions browser-test/browser-test-runner.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2019, Google, LLC.
// Copyright 2019 Google, LLC
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
Expand Down Expand Up @@ -51,12 +51,14 @@ async function main() {

const server = await listen(app, port);
console.log(`[http server] I'm listening on port ${port}! Starting karma.`);
const result = await execa('karma', ['start'], {stdio: 'inherit'});
const result = await execa('karma', ['start'], { stdio: 'inherit' });
server.close();
console.log(
`[http server] Karma has finished! I'm no longer listening on port ${port}!`
);
process.exit(result.failed ? 1 : 0);
if (result.failed) {
throw new Error('Tests failed.');
}
}

main().catch(err => {
Expand Down
21 changes: 17 additions & 4 deletions browser-test/test.browser.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,32 @@
import assert from 'assert';
// Copyright 2019 Google, LLC
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import {request} from '../src/index';
import assert from 'assert';
import { describe, it } from 'mocha';
import { request } from '../src/index';
const port = 7172; // should match the port defined in `webserver.ts`

describe('💻 browser tests', () => {
it('should just work from browser', async () => {
const result = await request({url: `http://localhost:${port}/path`});
const result = await request({ url: `http://localhost:${port}/path` });
assert.strictEqual(result.status, 200);
assert.strictEqual(result.data, 'response');
});

it('should pass querystring parameters from browser', async () => {
const result = await request({
url: `http://localhost:${port}/querystring`,
params: {query: 'value'},
params: { query: 'value' }
});
assert.strictEqual(result.status, 200);
assert.strictEqual(result.data, 'value');
Expand Down
2 changes: 1 addition & 1 deletion karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ process.env.CHROME_BIN = fs.existsSync('/usr/bin/chromium-browser')
? '/usr/bin/chromium-browser'
: require('puppeteer').executablePath();

module.exports = function (config) {
module.exports = function(config) {
config.set({
// base path that will be used to resolve all patterns (eg. files, exclude)
basePath: '',
Expand Down
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
"test": "c8 mocha build/test",
"presystem-test": "npm run compile",
"system-test": "mocha build/system-test --timeout 40000",
"clean": "gts clean",
"compile": "tsc -p .",
"fix": "gts fix",
"prepare": "npm run compile",
Expand Down Expand Up @@ -52,7 +51,7 @@
"codecov": "^3.2.0",
"execa": "^4.0.0",
"express": "^4.16.4",
"gts": "^1.0.0",
"gts": "2.0.0-alpha.3",
"is-docker": "^2.0.0",
"karma": "^4.0.0",
"karma-chrome-launcher": "^3.0.0",
Expand Down
4 changes: 0 additions & 4 deletions prettier.config.js

This file was deleted.

3 changes: 3 additions & 0 deletions samples/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
{
"name": "gaxios-samples",
"private": true,
"files": [
"*.js"
],
"scripts": {
"test": "mocha"
},
Expand Down
4 changes: 2 additions & 2 deletions samples/quickstart.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@
// limitations under the License.

// [START gaxios_quickstart]
const {request} = require('gaxios');
const { request } = require('gaxios');
Copy link
Contributor

Choose a reason for hiding this comment

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

So... how about disabling this option in prettier config to make the result much closer to what we have now?

        "bracketSpacing": {
          "description": "Print spaces between brackets.",
          "default": true,
          "type": "boolean"
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nods we absolutely could do that. I was thinking of reducing the amount of prettier customizations we do, and keep it as close to the default config as possible. That includes dropping bracket spacing, and the trailing commas.

If we want to optimize for lowest changes, we could keep adding those customizations in.

Copy link
Contributor

@alexander-fenster alexander-fenster Feb 4, 2020

Choose a reason for hiding this comment

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

Oh. There is a serious risk of starting a discussion about style here :)

I get your point about keeping minimal config, but one thing about gts default style is that it more or less did what's explained in Google JavaScript Style Guide. For some reason I feel it might make sense to adhere to that style guide :) It puts no space between brackets, and uses trailing commas.

image

image

But again, I just thought that keeping the formatting (if not too hard) is a good idea, but I'm OK with either decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good :) gts update here:
google/gts#440


/**
* Perform a simple `GET` request to a JSON API.
*/
async function quickstart() {
const url = 'https://www.googleapis.com/discovery/v1/apis/';
const res = await request({url});
const res = await request({ url });
console.log(`status: ${res.status}`);
console.log(`data:`);
console.log(res.data);
Expand Down
8 changes: 4 additions & 4 deletions samples/test/test.samples.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.

const {execSync} = require('child_process');
const {assert} = require('chai');
const {describe, it} = require('mocha');
const { execSync } = require('child_process');
const { assert } = require('chai');
const { describe, it } = require('mocha');

const exec = cmd => execSync(cmd, { encoding: 'utf8'});
const exec = cmd => execSync(cmd, { encoding: 'utf8' });

describe(__filename, () => {
it('should run the quickstart', () => {
Expand Down
5 changes: 5 additions & 0 deletions src/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"env": {
"browser": true
}
}
10 changes: 5 additions & 5 deletions src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {AbortSignal} from 'abort-controller';
import {Agent} from 'http';
import {URL} from 'url';
import { AbortSignal } from 'abort-controller';
import { Agent } from 'http';
import { URL } from 'url';

// tslint:disable no-any
/* eslint-disable @typescript-eslint/no-explicit-any */

export class GaxiosError<T = any> extends Error {
code?: string;
Expand Down Expand Up @@ -86,7 +86,7 @@ export interface GaxiosOptions {
maxRedirects?: number;
follow?: number;
params?: any;
paramsSerializer?: (params: {[index: string]: string | number}) => string;
paramsSerializer?: (params: { [index: string]: string | number }) => string;
timeout?: number;
onUploadProgress?: (progressEvent: any) => void;
responseType?: 'arraybuffer' | 'blob' | 'json' | 'text' | 'stream';
Expand Down
28 changes: 15 additions & 13 deletions src/gaxios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@
// limitations under the License.

import extend from 'extend';
import {Agent} from 'http';
import nodeFetch, {Response as NodeFetchResponse} from 'node-fetch';
import { Agent } from 'http';
import nodeFetch, { Response as NodeFetchResponse } from 'node-fetch';
import qs from 'querystring';
import stream from 'stream';
import isStream from 'is-stream';
import url from 'url';

Expand All @@ -24,11 +23,12 @@ import {
GaxiosOptions,
GaxiosPromise,
GaxiosResponse,
Headers,
Headers
} from './common';
import {getRetryConfig} from './retry';
import { getRetryConfig } from './retry';

// tslint:disable no-any
/* eslint-disable @typescript-eslint/no-explicit-any */
/* eslint-disable node/no-unsupported-features/node-builtins */

const URL = hasURL() ? window.URL : url.URL;
const fetch = hasFetch() ? window.fetch : nodeFetch;
Expand All @@ -45,7 +45,6 @@ function hasFetch() {
return hasWindow() && !!window.fetch;
}

// tslint:disable-next-line variable-name
let HttpsProxyAgent: any;

// Figure out if we should be using a proxy. Only if it's required, load
Expand Down Expand Up @@ -116,7 +115,7 @@ export class Gaxios {
} catch (e) {
const err = e as GaxiosError;
err.config = opts;
const {shouldRetry, config} = await getRetryConfig(e);
const { shouldRetry, config } = await getRetryConfig(e);
if (shouldRetry && config) {
err.config.retryConfig!.currentRetryAttempt = config.retryConfig!.currentRetryAttempt;
return this._request<T>(err.config);
Expand All @@ -132,12 +131,15 @@ export class Gaxios {
switch (opts.responseType) {
case 'stream':
return res.body;
case 'json':
case 'json': {
let data = await res.text();
try {
data = JSON.parse(data);
} catch (e) {}
} catch {
// continue
}
return data as {};
}
case 'arraybuffer':
return res.arrayBuffer();
case 'blob':
Expand Down Expand Up @@ -229,7 +231,7 @@ export class Gaxios {
* Encode a set of key/value pars into a querystring format (?foo=bar&baz=boo)
* @param params key value pars to encode
*/
private paramsSerializer(params: {[index: string]: string | number}) {
private paramsSerializer(params: { [index: string]: string | number }) {
return qs.stringify(params);
}

Expand All @@ -253,8 +255,8 @@ export class Gaxios {

// XMLHttpRequestLike
request: {
responseURL: res.url,
},
responseURL: res.url
}
};
}
}
8 changes: 4 additions & 4 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {GaxiosOptions} from './common';
import {Gaxios} from './gaxios';
import { GaxiosOptions } from './common';
import { Gaxios } from './gaxios';

export {
GaxiosError,
GaxiosPromise,
GaxiosResponse,
Headers,
RetryConfig,
RetryConfig
} from './common';
export {Gaxios, GaxiosOptions};
export { Gaxios, GaxiosOptions };

/**
* The default instance used when the `request` method is directly
Expand Down
12 changes: 6 additions & 6 deletions src/retry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {GaxiosError} from './common';
import { GaxiosError } from './common';

export async function getRetryConfig(err: GaxiosError) {
let config = getConfig(err);
if (!err || !err.config || (!config && !err.config.retry)) {
return {shouldRetry: false};
return { shouldRetry: false };
}
config = config || {};
config.currentRetryAttempt = config.currentRetryAttempt || 0;
Expand All @@ -27,7 +27,7 @@ export async function getRetryConfig(err: GaxiosError) {
'HEAD',
'PUT',
'OPTIONS',
'DELETE',
'DELETE'
];
config.noResponseRetries =
config.noResponseRetries === undefined || config.noResponseRetries === null
Expand All @@ -46,7 +46,7 @@ export async function getRetryConfig(err: GaxiosError) {
// 5xx - Retry (Server errors)
[100, 199],
[429, 429],
[500, 599],
[500, 599]
];
config.statusCodesToRetry = config.statusCodesToRetry || retryRanges;

Expand All @@ -56,7 +56,7 @@ export async function getRetryConfig(err: GaxiosError) {
// Determine if we should retry the request
const shouldRetryFn = config.shouldRetry || shouldRetryRequest;
if (!(await shouldRetryFn(err))) {
return {shouldRetry: false, config: err.config};
return { shouldRetry: false, config: err.config };
}

// Calculate time to wait with exponential backoff.
Expand All @@ -78,7 +78,7 @@ export async function getRetryConfig(err: GaxiosError) {

// Return the promise in which recalls Gaxios to retry the request
await backoff;
return {shouldRetry: true, config: err.config};
return { shouldRetry: true, config: err.config };
}

/**
Expand Down
Empty file removed src/web.ts
Empty file.
16 changes: 15 additions & 1 deletion system-test/fixtures/sample/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,18 @@
import {request} from 'gaxios';
// Copyright 2019 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import { request } from 'gaxios';
async function main() {
await request({
url: 'https://www.googleapis.com/discovery/v1/apis/'
Expand Down
Loading