From b872f2bb03e315967c1d9eec3fd3186f2062bf23 Mon Sep 17 00:00:00 2001 From: Alexander Fenster Date: Fri, 3 May 2019 19:29:00 -0700 Subject: [PATCH] feat!: use @grpc/grpc-js instead of grpc (#484) BREAKING CHANGE * feat: use @grpc/grpc-js instead of grpc * no need to have client libs test since end-to-end exists * use any instead of ts-ignore * no need to console.error * fix windows builds --- .kokoro/test.bat | 10 +- README.md | 11 ++ package.json | 5 +- src/bundlingCalls/bundleExecutor.ts | 2 +- src/bundlingCalls/task.ts | 2 +- src/call.ts | 2 +- src/gax.ts | 2 +- src/googleError.ts | 2 +- src/grpc.ts | 52 +++---- src/index.ts | 1 - src/longRunningCalls/longrunning.ts | 2 +- src/normalCalls/retries.ts | 2 +- .../src/index.js | 8 +- system-test/test.clientlibs.ts | 145 ------------------ test/apiCallable.ts | 2 +- test/bundling.ts | 2 +- test/grpc.ts | 46 +----- test/longrunning.ts | 2 +- 18 files changed, 53 insertions(+), 245 deletions(-) delete mode 100644 system-test/test.clientlibs.ts diff --git a/.kokoro/test.bat b/.kokoro/test.bat index 767320757..fddff7570 100644 --- a/.kokoro/test.bat +++ b/.kokoro/test.bat @@ -17,7 +17,15 @@ cd /d %~dp0 cd .. -call npm install -g npm@latest || goto :error +@rem The image we're currently running has a broken version of Node.js enabled +@rem by nvm (v10.15.3), which has no npm bin. This hack uses the functional +@rem Node v8.9.1 to install npm@latest, it then uses this version of npm to +@rem install npm for v10.15.3. +call nvm use v8.9.1 || goto :error +call node C:\Users\kbuilder\AppData\Roaming\nvm-ps\versions\v8.9.1\node_modules\npm-bootstrap\bin\npm-cli.js i npm -g || goto :error +call nvm use v10.15.3 || goto :error +call node C:\Users\kbuilder\AppData\Roaming\nvm-ps\versions\v8.9.1\node_modules\npm\bin\npm-cli.js i npm -g || goto :error + call npm install || goto :error call npm run test || goto :error diff --git a/README.md b/README.md index ab5e28fbb..ea6484ba8 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,17 @@ Application code will rarely need to use most of the classes within this library $ npm install google-gax ``` +## Supporting older version of Node.js + +This library uses [grpc-js](https://www.npmjs.com/package/@grpc/grpc-js) package for communicating with API server, and it uses HTTP/2 functionality +that is only available in Node.js v8.13.0 or newer. If you need to use this library with older versions of Node.js, you need to make your code depend +on a legacy gRPC library ([grpc](https://www.npmjs.com/package/grpc)) and pass the instance of gRPC to the client constructor: + +```js +const grpc = require('grpc'); +const client = new APIClient({ grpc }); // APIClient is the client class you use, e.g. SpeechClient, etc. +``` + ## Contributing Contributions to this library are always welcome and highly encouraged. See the [CONTRIBUTING][contributing] documentation for more information on how to get started. diff --git a/package.json b/package.json index b1939c8d5..962fb36e4 100644 --- a/package.json +++ b/package.json @@ -13,8 +13,6 @@ "@grpc/proto-loader": "^0.5.0", "duplexify": "^3.6.0", "google-auth-library": "^3.0.0", - "grpc": "^1.20.2", - "grpc-gcp": "^0.1.1", "is-stream-ended": "^0.1.4", "lodash.at": "^4.6.0", "lodash.has": "^4.5.2", @@ -41,7 +39,6 @@ "@types/through2": "^2.0.33", "chai": "*", "codecov": "^3.1.0", - "cross-env": "^5.2.0", "eslint": "^5.9.0", "eslint-config-prettier": "^4.0.0", "eslint-plugin-node": "^9.0.0", @@ -71,7 +68,7 @@ "docs": "compodoc src/", "gen-parser": "pegjs src/pathTemplateParser.pegjs", "pretest": "npm run prepare", - "test": "cross-env GOOGLE_CLOUD_USE_GRPC_JS=1 nyc mocha build/test && mocha build/test", + "test": "nyc mocha build/test", "lint": "gts check && eslint samples/*.js samples/**/*.js", "clean": "gts clean", "compile": "tsc -p . && cp src/*.json build/src && cp src/*.js build/src", diff --git a/src/bundlingCalls/bundleExecutor.ts b/src/bundlingCalls/bundleExecutor.ts index 87da6641b..e9efd5cc6 100644 --- a/src/bundlingCalls/bundleExecutor.ts +++ b/src/bundlingCalls/bundleExecutor.ts @@ -29,7 +29,7 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; import {SimpleCallbackFunction} from '../apitypes'; import {GoogleError} from '../googleError'; diff --git a/src/bundlingCalls/task.ts b/src/bundlingCalls/task.ts index 777837e9c..27558feff 100644 --- a/src/bundlingCalls/task.ts +++ b/src/bundlingCalls/task.ts @@ -29,7 +29,7 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; import {APICallback, GRPCCallResult, SimpleCallbackFunction} from '../apitypes'; import {GoogleError} from '../googleError'; diff --git a/src/call.ts b/src/call.ts index 9c57d5e0a..c698815fb 100644 --- a/src/call.ts +++ b/src/call.ts @@ -29,7 +29,7 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; import { APICallback, diff --git a/src/gax.ts b/src/gax.ts index e3f24ff60..8d4bce884 100644 --- a/src/gax.ts +++ b/src/gax.ts @@ -667,7 +667,7 @@ export function constructSettings( serviceName: string, clientConfig: ClientConfig, configOverrides: ClientConfig, - retryNames: {[index: string]: number}, + retryNames: {}, otherArgs?: {}, promise?: PromiseConstructor ) { diff --git a/src/googleError.ts b/src/googleError.ts index 76b42e7aa..56a6d68fc 100644 --- a/src/googleError.ts +++ b/src/googleError.ts @@ -29,7 +29,7 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; export class GoogleError extends Error { code?: status; diff --git a/src/grpc.ts b/src/grpc.ts index c4315fd69..fba926c03 100644 --- a/src/grpc.ts +++ b/src/grpc.ts @@ -30,11 +30,10 @@ * */ -import * as grpcProtoLoaderTypes from '@grpc/proto-loader'; // for types only +import * as grpcProtoLoader from '@grpc/proto-loader'; import * as fs from 'fs'; import {GoogleAuth, GoogleAuthOptions} from 'google-auth-library'; -import * as grpcTypes from 'grpc'; // for types only -import * as grpcGcp from 'grpc-gcp'; +import * as grpc from '@grpc/grpc-js'; import {OutgoingHttpHeaders} from 'http'; import * as path from 'path'; import * as protobuf from 'protobufjs'; @@ -56,8 +55,6 @@ const COMMON_PROTO_FILES = walk .filter(f => path.extname(f) === '.proto') .map(f => path.normalize(f).substring(googleProtoFilesDir.length + 1)); -export {GrpcObject} from 'grpc'; - export interface GrpcClientOptions extends GoogleAuthOptions { auth?: GoogleAuth; promise?: PromiseConstructor; @@ -76,17 +73,17 @@ export interface Metadata { get: (key: {}) => {}; } -export type GrpcModule = typeof grpcTypes & { - status: {[index: string]: number}; -}; +export type GrpcModule = typeof grpc; export interface ClientStubOptions { servicePath: string; port: number; - sslCreds?: grpcTypes.ChannelCredentials; + // TODO: use sslCreds?: grpc.ChannelCredentials; + // tslint:disable-next-line no-any + sslCreds?: any; } -export class ClientStub extends grpcTypes.Client { +export class ClientStub extends grpc.Client { [name: string]: Function; } @@ -95,7 +92,6 @@ export class GrpcClient { promise: PromiseConstructor; grpc: GrpcModule; grpcVersion: string; - grpcProtoLoader: typeof grpcProtoLoaderTypes; /** * A class which keeps the context of gRPC and auth for the gRPC. @@ -122,20 +118,16 @@ export class GrpcClient { this.grpc = options.grpc!; this.grpcVersion = ''; } else { - // EXPERIMENTAL: If GOOGLE_CLOUD_USE_GRPC_JS is set, use the JS-based - // implementation of the gRPC client instead. Requires http2 (Node 8+). - if ( - semver.gte(process.version, '8.13.0') && - !!process.env.GOOGLE_CLOUD_USE_GRPC_JS - ) { - this.grpc = require('@grpc/grpc-js'); + if (semver.gte(process.version, '8.13.0')) { + this.grpc = grpc; this.grpcVersion = require('@grpc/grpc-js/package.json').version; } else { - this.grpc = require('grpc'); - this.grpcVersion = require('grpc/package.json').version; + const errorMessage = + 'To use @grpc/grpc-js you must run your code on Node.js v8.13.0 or newer. Please see README if you need to use an older version. ' + + 'https://github.com/googleapis/gax-nodejs/blob/master/README.md'; + throw new Error(errorMessage); } } - this.grpcProtoLoader = require('@grpc/proto-loader'); } /** @@ -166,8 +158,8 @@ export class GrpcClient { * @param filename The path to the proto file. * @param options Options for loading the proto file. */ - loadFromProto(filename: string, options: grpcProtoLoaderTypes.Options) { - const packageDef = grpcProtoLoaderTypes.loadSync(filename, options); + loadFromProto(filename: string, options: grpcProtoLoader.Options) { + const packageDef = grpcProtoLoader.loadSync(filename, options); return this.grpc.loadPackageDefinition(packageDef); } @@ -289,20 +281,12 @@ export class GrpcClient { async createStub(CreateStub: typeof ClientStub, options: ClientStubOptions) { const serviceAddress = options.servicePath + ':' + options.port; const creds = await this._getCredentials(options); - const grpcOptions: {[index: string]: {}} = {}; + const grpcOptions: {[index: string]: string} = {}; Object.keys(options).forEach(key => { - if (key.indexOf('grpc.') === 0) { - grpcOptions[key] = options[key]; + if (key.startsWith('grpc.')) { + grpcOptions[key.replace(/^grpc\./, '')] = options[key]; } }); - const apiConfigDefinition = options['grpc_gcp.apiConfig']; - if (apiConfigDefinition) { - const apiConfig = grpcGcp.createGcpApiConfig(apiConfigDefinition); - grpcOptions['channelFactoryOverride'] = grpcGcp.gcpChannelFactoryOverride; - grpcOptions['callInvocationTransformer'] = - grpcGcp.gcpCallInvocationTransformer; - grpcOptions['gcpApiConfig'] = apiConfig; - } const stub = new CreateStub(serviceAddress, creds, grpcOptions); return stub; } diff --git a/src/index.ts b/src/index.ts index ad6f0457f..231c4c87d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -57,7 +57,6 @@ export { GrpcClient, GrpcClientOptions, GrpcModule, - GrpcObject, Metadata, MetadataValue, } from './grpc'; diff --git a/src/longRunningCalls/longrunning.ts b/src/longRunningCalls/longrunning.ts index ba0089ec8..301935d8c 100644 --- a/src/longRunningCalls/longrunning.ts +++ b/src/longRunningCalls/longrunning.ts @@ -30,7 +30,7 @@ */ import {EventEmitter} from 'events'; -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; import {GaxCallPromise, ResultTuple} from '../apitypes'; import {CancellablePromise} from '../call'; diff --git a/src/normalCalls/retries.ts b/src/normalCalls/retries.ts index f1e22435f..aa204b355 100644 --- a/src/normalCalls/retries.ts +++ b/src/normalCalls/retries.ts @@ -29,7 +29,7 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; import { APICallback, diff --git a/system-test/fixtures/google-gax-packaging-test-app/src/index.js b/system-test/fixtures/google-gax-packaging-test-app/src/index.js index 8434e6cc5..f23ba443b 100644 --- a/system-test/fixtures/google-gax-packaging-test-app/src/index.js +++ b/system-test/fixtures/google-gax-packaging-test-app/src/index.js @@ -15,13 +15,7 @@ 'use strict'; const assert = require('assert'); - -let grpc; -if (process.env['GOOGLE_CLOUD_USE_GRPC_JS']) { - grpc = require('@grpc/grpc-js'); -} else { - grpc = require('grpc'); -} +const grpc = require('@grpc/grpc-js'); // Import the clients for each version supported by this package. const gapic = Object.freeze({ diff --git a/system-test/test.clientlibs.ts b/system-test/test.clientlibs.ts deleted file mode 100644 index 82ee0e379..000000000 --- a/system-test/test.clientlibs.ts +++ /dev/null @@ -1,145 +0,0 @@ -/* - * Copyright 2019 Google LLC - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following disclaimer - * in the documentation and/or other materials provided with the - * distribution. - * * Neither the name of Google Inc. nor the names of its - * contributors may be used to endorse or promote products derived from - * this software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ - -import * as execa from 'execa'; -import * as fs from 'fs'; -import * as path from 'path'; -import * as rimraf from 'rimraf'; -import * as util from 'util'; - -const mkdir = util.promisify(fs.mkdir); -const rmrf = util.promisify(rimraf); -const readFile = util.promisify(fs.readFile); -const writeFile = util.promisify(fs.writeFile); - -const baseRepoUrl = 'https://github.com/googleapis/'; -const testDir = path.join(process.cwd(), '.system-test-run'); -const gaxDir = path.resolve(__dirname, '..', '..'); - -// We will pack google-gax using `npm pack`, defining some constants to make it -// easier to consume that tarball -const pkg = require('../../package.json'); -const gaxTarball = path.join(gaxDir, `${pkg.name}-${pkg.version}.tgz`); - -async function latestRelease(cwd: string): Promise { - const {stdout} = await execa('git', ['tag', '--list'], {cwd}); - const tags = stdout - .split('\n') - .filter(str => str.match(/^v\d+\.\d+\.\d+$/)) - .sort( - (tag1: string, tag2: string): number => { - const match1 = tag1.match(/^v(\d+)\.(\d+)\.(\d+)$/); - const match2 = tag2.match(/^v(\d+)\.(\d+)\.(\d+)$/); - if (!match1 || !match2) { - throw new Error(`Cannot compare git tags ${tag1} and ${tag2}`); - } - // compare major version, then minor versions, then patch versions. - // return positive number, zero, or negative number - for (let idx = 1; idx <= 3; ++idx) { - if (match1[idx] !== match2[idx]) { - return Number(match1[idx]) - Number(match2[idx]); - } - } - return 0; - } - ); - // the last tag in the list is the latest release - return tags[tags.length - 1]; -} - -async function preparePackage(packageName: string): Promise { - await execa( - 'git', - ['clone', `${baseRepoUrl}${packageName}.git`, packageName], - {stdio: 'inherit'} - ); - const tag = await latestRelease(packageName); - await execa('git', ['checkout', tag], {cwd: packageName, stdio: 'inherit'}); - - const packageJson = path.join(packageName, 'package.json'); - const packageJsonStr = (await readFile(packageJson)).toString(); - const packageJsonObj = JSON.parse(packageJsonStr); - packageJsonObj['dependencies']['google-gax'] = `file:${gaxTarball}`; - await writeFile(packageJson, JSON.stringify(packageJsonObj, null, ' ')); - await execa('npm', ['install'], {cwd: packageName, stdio: 'inherit'}); -} - -async function runSystemTest(packageName: string): Promise { - await execa('npm', ['run', 'system-test'], { - cwd: packageName, - stdio: 'inherit', - }); -} - -describe('Run system tests for some libraries', () => { - before(async () => { - console.log('Packing google-gax...'); - await execa('npm', ['pack'], {cwd: gaxDir, stdio: 'inherit'}); - - if (!fs.existsSync(gaxTarball)) { - throw new Error(`npm pack tarball ${gaxTarball} does not exist`); - } - - await rmrf(testDir); - await mkdir(testDir); - process.chdir(testDir); - console.log(`Running tests in ${testDir}.`); - }); - // Video intelligence API has long running operations - describe('video-intelligence', () => { - before(async () => { - await preparePackage('nodejs-video-intelligence'); - }); - it('should pass system tests', async () => { - await runSystemTest('nodejs-video-intelligence'); - }); - }); - // Pub/Sub has streaming methods and pagination - describe('pubsub', () => { - before(async () => { - await preparePackage('nodejs-pubsub'); - }); - it('should pass system tests', async function() { - // Pub/Sub tests can be slow since they check packaging - this.timeout(300000); - await runSystemTest('nodejs-pubsub'); - }); - }); - // Speech only has smoke tests, but still... - describe('speech', () => { - before(async () => { - await preparePackage('nodejs-speech'); - }); - it('should pass system tests', async () => { - await runSystemTest('nodejs-speech'); - }); - }); -}); diff --git a/test/apiCallable.ts b/test/apiCallable.ts index 4b879a93b..3b67b12ed 100644 --- a/test/apiCallable.ts +++ b/test/apiCallable.ts @@ -29,7 +29,7 @@ */ import {expect} from 'chai'; -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; import * as sinon from 'sinon'; import * as gax from '../src/gax'; diff --git a/test/bundling.ts b/test/bundling.ts index 406e28a30..9f3d6eb82 100644 --- a/test/bundling.ts +++ b/test/bundling.ts @@ -29,7 +29,7 @@ */ import {expect} from 'chai'; -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; import * as sinon from 'sinon'; import {BundleDescriptor} from '../src/bundlingCalls/bundleDescriptor'; diff --git a/test/grpc.ts b/test/grpc.ts index 53e1927f2..e39d8d1d2 100644 --- a/test/grpc.ts +++ b/test/grpc.ts @@ -37,12 +37,6 @@ import * as sinon from 'sinon'; import {GoogleProtoFilesRoot, GrpcClient} from '../src/grpc'; -// When this flag is set, tests that have to do with loadProto will be skipped. -// They are known to not work with grpc-js, as they use a now-deprecated API. -const USE_GRPC_JS = - semver.gte(process.version, '8.13.0') && - !!process.env.GOOGLE_CLOUD_USE_GRPC_JS; - function gaxGrpc(options?) { return new GrpcClient(options); } @@ -50,7 +44,7 @@ function gaxGrpc(options?) { describe('grpc', () => { describe('grpcVersion', () => { it('holds the proper grpc version', () => { - const grpcModule = USE_GRPC_JS ? '@grpc/grpc-js' : 'grpc'; + const grpcModule = '@grpc/grpc-js'; const grpcVersion = require(`${grpcModule}/package.json`).version; expect(gaxGrpc().grpcVersion).to.eq(grpcVersion); }); @@ -188,8 +182,8 @@ describe('grpc', () => { expect(stub.creds).to.deep.eq(dummyChannelCreds); // tslint:disable-next-line no-any (expect(stub.options).has as any).key([ - 'grpc.max_send_message_length', - 'grpc.initial_reconnect_backoff_ms', + 'max_send_message_length', + 'initial_reconnect_backoff_ms', ]); // tslint:disable-next-line no-any (expect(stub.options).to.not.have as any).key([ @@ -200,40 +194,6 @@ describe('grpc', () => { }); }); - it('supports grpc-gcp options', () => { - const apiConfigDefinition = require(path.resolve( - TEST_PATH, - 'test_grpc_config.json' - )); - const opts = { - servicePath: 'foo.example.com', - port: 443, - 'grpc_gcp.apiConfig': apiConfigDefinition, - }; - return grpcClient.createStub(DummyStub, opts).then(stub => { - expect(stub).to.be.an.instanceOf(DummyStub); - expect(stub.address).to.eq('foo.example.com:443'); - expect(stub.creds).to.deep.eq(dummyChannelCreds); - // tslint:disable-next-line no-any - (expect(stub.options).has as any).key([ - 'channelFactoryOverride', - 'callInvocationTransformer', - 'gcpApiConfig', - ]); - // tslint:disable-next-line no-any - (expect(stub.options).to.not.have as any).key([ - 'servicePath', - 'port', - 'grpc_gcp.apiConfig', - ]); - // tslint:disable-next-line no-any - (expect(stub.options['gcpApiConfig']).has as any).key([ - 'channelPool', - 'method', - ]); - }); - }); - it('uses the passed grpc channel', () => { const customCreds = {channelCreds: 'custom'}; const opts = { diff --git a/test/longrunning.ts b/test/longrunning.ts index ea7b1be8d..7c42429f5 100644 --- a/test/longrunning.ts +++ b/test/longrunning.ts @@ -29,7 +29,7 @@ */ import {expect} from 'chai'; -import {status} from 'grpc'; +import {status} from '@grpc/grpc-js'; import * as sinon from 'sinon'; import {LongrunningDescriptor} from '../src';