Regression: @google-cloud/logging unhandled rejections during auth

[ofrobots]

I wanted to try a performance benchmark now that 1.0.0 of @google-cloud/logging is now. The very basic example I started with seems to not work at all. Fails with unhandled rejections.

(node:44617) TypeError: Cannot read property 'config' of undefined
    at Auth.getAuthClient (/Users/ofrobots/tmp/log-t/node_modules/google-gax/node_modules/google-auto-auth/index.js:38:20)
    at /Users/ofrobots/tmp/log-t/node_modules/google-gax/lib/grpc.js:90:5
    at GrpcClient._getCredentials (/Users/ofrobots/tmp/log-t/node_modules/google-gax/lib/grpc.js:89:10)
    at GrpcClient.createStub (/Users/ofrobots/tmp/log-t/node_modules/google-gax/lib/grpc.js:172:15)
    at new LoggingServiceV2Client (/Users/ofrobots/tmp/log-t/node_modules/@google-cloud/logging/src/v2/logging_service_v2_client.js:122:38)
    at Object.LoggingServiceV2ClientBuilder.loggingServiceV2Client (/Users/ofrobots/tmp/log-t/node_modules/@google-cloud/logging/src/v2/logging_service_v2_client.js:832:12)
    at /Users/ofrobots/tmp/log-t/node_modules/@google-cloud/logging/src/index.js:603:52
    at Immediate.<anonymous> (/Users/ofrobots/tmp/log-t/node_modules/google-auto-auth/index.js:146:7)
    at runCallback (timers.js:672:20)
    at tryOnImmediate (timers.js:645:5)

A PR with the fix is here: googleapis/gax-nodejs#132.

This is a pretty big regression. How can we avoid such regressions going forward?

[bcomnes]

Just wasted an hour on this to realize it was a deep transitive dependency regression. :( Thanks for the fix!

[jonsharratt]

I have just hit this issue, life saver thanks for getting a fix in on this one, was in the same ⛵️ as @bcomnes :D Might be worth covering that issue with a test so it ensures it is all good going forward re: help stopping regressions.

[stephenplusplus]

How can we avoid such regressions going forward?

In terms of GCN, we would have to shrinkwrap our dependencies when releasing. Also, nightly cron jobs would catch these things. In terms of GAX, more tests :)

[stephenplusplus]

Sorry to everyone who got burned by this. This is one of those things "that can happen sometimes" in npm, since we don't lock down our exact dependencies when publishing, and we instead rely on a semver-safe range. It's still within the realm of possibility that a package author will have an oversight and include a breaking change in a release that shouldn't have included one, or just a bug, like what happened in this case.

I don't think shrinkwrapping is a road we want to go down, but we have an issue for nightly builds on the central tracker for GC* projects: https://github.com/GoogleCloudPlatform/gcloud-common/issues/222. That would catch things like this, where we could review any failed builds when they happen and look into the solution right away.

For this bug, GCN doesn't have to do anything to include the fix once it's released in GAX. It will automatically be picked up in @google-cloud/logging installs. Considering that, I'm going to close this and let GAX address their testing procedure however they determine is right for their project, and the nightly build issue will hold the discussion around when & how to implement that.

[bcomnes]

FWIW yarn was able to keep my project working due to the lock file in this scenario. I ran into the issue with another dev using npm and was in a 'works in my env' type situation.

[ofrobots]

@stephenplusplus Yes, I was also thinking something along the lines of a nightly build as well. I don't think we should shrinkwrap – that actually makes it harder to roll out fixes in a deep chain of dependencies. Users can and should use yarn to avoid such breaks at unexpected points.

My concern was more around users finding bugs before we find them in a CI, and a nightly build should help here. I'll subscribe to that issue.

BTW, FYI, google-gax@0.13.2 was released earlier today with the fix, so this should be fixed now.