-
Notifications
You must be signed in to change notification settings - Fork 102
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
feat: add async getProjectId method #657
Conversation
Codecov Report
@@ Coverage Diff @@
## master #657 +/- ##
==========================================
- Coverage 99.38% 99.34% -0.04%
==========================================
Files 7 7
Lines 6131 6110 -21
Branches 424 382 -42
==========================================
- Hits 6093 6070 -23
- Misses 37 39 +2
Partials 1 1
Continue to review full report at Codecov.
|
There are a couple places I think need to be handled:
|
Thanks @stephenplusplus! For As far as I can tell, the For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
How does this compare to the other node.js client interfaces? |
Good question! This makes Datastore consistent with the GAPIC libraries, and moves away from what Bigtable, Storage, PubSub, and BigQuery are doing. The goal would be to make this change for those libraries as well. It's impossible for me to know how much users rely on the |
I am okay with taking this. Will let @BenWhitehead lgtm though. |
Conceptually I'm okay with this, but I also don't have any idea around how this might be in the path for folks. (And I don't know NodeJS well enough to be able to speculate). @schmidt-sebastian Do you have ideas here or know someone who might? |
Note that I haven’t looked at the changes on this PR (and am also not going to voice an opinion on Datastore). In Firestore, we were able to do this without a breaking change by kicking off Project ID detection just before we serialized requests, which always happened as part of an async call. |
👋 speaking from the perspective of other gapic libraries, you are able to get projectID via:
So this approach is consistent. As @JustinBeckwith points out, |
BREAKING CHANGE: The
Datastore.projectId
property has been removed, and replaced with an asynchronousgetProjectid()
method. The projectId cannot be determined synchronously, so the previous approach was to use a{{projectId}}
string placeholder if the projectId had not yet been acquired. This made it difficult to know exactly when the property would be defined.As an added bonus, the
@google-cloud/projectify
dependency has been removed. Adding @crwilcox because this would be a breaking change (we're about to ship one).