[JustinBeckwith]

BREAKING CHANGE: The Datastore.projectId property has been removed, and replaced with an asynchronous getProjectid() 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).

[codecov]

Codecov Report

Merging #657 into master will decrease coverage by 0.03%.
The diff coverage is 92.59%.

@@            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              

Impacted Files	Coverage Δ
src/transaction.ts	99.58% <ø> (-0.01%)	⬇️
src/index.ts	99.70% <60.00%> (-0.20%)	⬇️
src/request.ts	100.00% <100.00%> (ø)

---

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 ea22d60...aa6fe45. Read the comment docs.

[stephenplusplus]

There are a couple places I think need to be handled:

• DatastoreRequest.projectId:

  nodejs-datastore/src/request.ts

  Line 1351 in a209914

  reqOpts.projectId = datastore.projectId;

• Transaction.projectId:

  nodejs-datastore/src/transaction.ts

  Line 70 in a209914

  this.projectId = datastore.projectId;

[JustinBeckwith]

Thanks @stephenplusplus!

For datastoreRequest.projectId: I looked pretty closely at this one. The projectId was set higher in the function like you called out, but then after auth.getProjectId it is set again here:
https://github.com/googleapis/nodejs-datastore/pull/657/files#diff-31b74031f3c83e9e883fd930aaf87f54R1389

As far as I can tell, the reqOpts weren't passed into anything before that. It's a little hard to explain without checkout the branch and looking at that whole function for sure.

For transaction.ts that one was also a little weird. There was no compiler error for the dangling datastore.projectId because the types appear to allow [index: string]: Entity, which is aliased to any 😬 I'll look into that another day. For now, I removed the errant property, though it looks like it wasn't being used anywhere.

[stephenplusplus]

LGTM!

[crwilcox]

How does this compare to the other node.js client interfaces?

[JustinBeckwith]

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 projectId property in its current state, so I don't know how bad of a breaking change this really is. My hunch is that a property that starts as {{projectId}} and then magically at some point has the right value is less valuable than an async method that always returns the right value.

[crwilcox]

I am okay with taking this. Will let @BenWhitehead lgtm though.

[BenWhitehead]

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?

[schmidt-sebastian]

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.

[bcoe]

👋 speaking from the perspective of other gapic libraries, you are able to get projectID via:

await client.getProjectId()

So this approach is consistent. As @JustinBeckwith points out, Datastore.projectId can't work in GCP environments, when fetching projectID via the metadata server.