feat!: use @grpc/grpc-js instead of grpc #484

alexander-fenster · 2019-05-03T06:29:10Z

BREAKING CHANGE

Make all client libraries use @grpc/grpc-js by default. The libraries that still want to use legacy grpc should pass the instance of grpc to client library constructor.

Tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

alexander-fenster · 2019-05-03T06:37:38Z

Something wrong with authentication - investigating...

codecov · 2019-05-03T07:05:20Z

Codecov Report

Merging #484 into master will decrease coverage by 0.03%.
The diff coverage is 80.95%.

@@            Coverage Diff            @@
##           master    #484      +/-   ##
=========================================
- Coverage   82.43%   82.4%   -0.04%     
=========================================
  Files          50      50              
  Lines        3439    3416      -23     
  Branches      266     262       -4     
=========================================
- Hits         2835    2815      -20     
+ Misses        539     537       -2     
+ Partials       65      64       -1

Impacted Files	Coverage Δ
src/index.ts	`90.62% <ø> (ø)`	⬆️
src/gax.ts	`83.07% <ø> (ø)`	⬆️
src/googleError.ts	`100% <ø> (ø)`	⬆️
...ixtures/google-gax-packaging-test-app/src/index.js	`0% <0%> (ø)`	⬆️
test/bundling.ts	`96.35% <100%> (ø)`	⬆️
src/bundlingCalls/bundleExecutor.ts	`87.05% <100%> (ø)`	⬆️
src/bundlingCalls/task.ts	`97.75% <100%> (ø)`	⬆️
test/apiCallable.ts	`81.34% <100%> (ø)`	⬆️
src/normalCalls/retries.ts	`89.28% <100%> (ø)`	⬆️
src/call.ts	`86.66% <100%> (ø)`	⬆️
... and 5 more

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 b0dbafb...2031b06. Read the comment docs.

codecov · 2019-05-03T07:05:20Z

Codecov Report

Merging #484 into master will decrease coverage by 0.05%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
- Coverage   82.43%   82.38%   -0.06%     
==========================================
  Files          50       50              
  Lines        3439     3417      -22     
  Branches      266      262       -4     
==========================================
- Hits         2835     2815      -20     
+ Misses        539      538       -1     
+ Partials       65       64       -1

Impacted Files	Coverage Δ
src/index.ts	`90.62% <ø> (ø)`	⬆️
src/gax.ts	`83.07% <ø> (ø)`	⬆️
src/googleError.ts	`100% <ø> (ø)`	⬆️
...ixtures/google-gax-packaging-test-app/src/index.js	`0% <0%> (ø)`	⬆️
test/bundling.ts	`96.35% <100%> (ø)`	⬆️
src/bundlingCalls/bundleExecutor.ts	`87.05% <100%> (ø)`	⬆️
src/bundlingCalls/task.ts	`97.75% <100%> (ø)`	⬆️
test/apiCallable.ts	`81.34% <100%> (ø)`	⬆️
src/normalCalls/retries.ts	`89.28% <100%> (ø)`	⬆️
src/call.ts	`86.66% <100%> (ø)`	⬆️
... and 5 more

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 728678a...7948190. Read the comment docs.

alexander-fenster · 2019-05-03T07:09:13Z

So, the authentication problem is caused by incorrect service account key (which just stopped working for some reason). I think at this point it's OK to remove the client library test (pub/sub, video intelligence, and speech), since we have an end-to-end test with a local gRPC server and it's much less fragile. When we are done will all these upgrades and semver major releases, we can put the client library system test back, but right now it will just prevent us from doing releases.

JustinBeckwith · 2019-05-03T16:32:43Z

src/grpc.ts

  servicePath: string;
  port: number;
-  sslCreds?: grpcTypes.ChannelCredentials;
+  // @ts-ignore waiting for grpc-js types


I don't understand why this is necessary. Should sslCreds just be any until we can fix things?

Changed to any!

JustinBeckwith · 2019-05-03T16:33:23Z

src/grpc.ts

+        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';
+        console.error(errorMessage);


I don't think we need to write to the console if we're throwing, do we?

I just wanted to communicate the error in all the ways possible :)

I agree with Justin, throw is probably sufficient, as it will end up in stderr and output essentially the same message (but with stack trace).

Removed console.error.

JustinBeckwith · 2019-05-03T16:34:18Z

system-test/test.clientlibs.ts

- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-import * as execa from 'execa';


removing this as part of this PR makes me uncomfortable and nervous 😨

I feel the same, but this kind of testing works great for minor changes but not for breaking changes. We have a full feature coverage through the end-to-end test (the one that runs local gRPC server), and also we'll only release it as a semver major so we'll have enough time to make sure client libraries work.

I promise I'll put this back when we are done with these huge changes :)

bcoe

This looks great to me, I'm really excited to be dropping node-gyp dependencies.

bcoe · 2019-05-03T17:14:43Z

src/grpc.ts

+        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';
+        console.error(errorMessage);


I agree with Justin, throw is probably sufficient, as it will end up in stderr and output essentially the same message (but with stack trace).

bcoe · 2019-05-03T17:15:04Z

system-test/fixtures/google-gax-packaging-test-app/src/index.js


 const assert = require('assert');
-
-let grpc;


seems like this update eliminates a lot of cruft.

feat: use @grpc/grpc-js instead of grpc

6f722a1

alexander-fenster requested review from JustinBeckwith and bcoe May 3, 2019 06:29

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 3, 2019

no need to have client libs test since end-to-end exists

7948190

Merge branch 'master' into no-grpc

1b4f1fb

JustinBeckwith suggested changes May 3, 2019

View reviewed changes

alexander-fenster added 3 commits May 3, 2019 09:45

use any instead of ts-ignore

aaae76e

Merge branch 'no-grpc' of github.com:googleapis/gax-nodejs into no-grpc

2556a9e

Merge branch 'master' into no-grpc

cdcf19c

bcoe approved these changes May 3, 2019

View reviewed changes

no need to console.error

7af9e20

JustinBeckwith approved these changes May 3, 2019

View reviewed changes

alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 3, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 3, 2019

alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 3, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 3, 2019

fix windows builds

2031b06

alexander-fenster merged commit b872f2b into master May 4, 2019

alexander-fenster deleted the no-grpc branch May 4, 2019 02:29

release-please bot mentioned this pull request Mar 17, 2020

chore: release 2.0.0 #738

Merged

feat!: use @grpc/grpc-js instead of grpc #484

feat!: use @grpc/grpc-js instead of grpc #484

Uh oh!

Conversation

alexander-fenster commented May 3, 2019

Uh oh!

alexander-fenster commented May 3, 2019

Uh oh!

codecov bot commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented May 3, 2019

Codecov Report

Uh oh!

alexander-fenster commented May 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented May 3, 2019 •

edited

Loading