feat(firestore): split GAPIC from the firestore handwritten package#8928
feat(firestore): split GAPIC from the firestore handwritten package#8928quirogas wants to merge 3 commits into
Conversation
product-auto-label
Bot
added
the
api: firestore
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes auto-generated configuration and proto files, transitioning the Firestore library to depend on the external @google-cloud/firestore-api package. Imports and lazy-loading getters for v1 and v1beta1 are updated accordingly. The review feedback highlights opportunities to optimize performance and code cleanliness: first, by directly requiring @google-cloud/firestore-api in the client factory to avoid repeatedly triggering lazy getters; and second, by caching the loaded modules in the lazy getters for v1 and v1beta1 to prevent redundant module loading and property copying on multiple accesses.
| // scope, we lazy-load the module. | ||
| get: () => require('./v1'), | ||
| get: () => { | ||
| const api = require('@google-cloud/firestore-api').v1; |
There was a problem hiding this comment.
Will @google-cloud/firestore-api be published to NPM? If so, it may be preferred to deprecate this v1 property.
There was a problem hiding this comment.
Or simply remove this and the related type in ./types/...
| get: () => require('./v1beta1'), | ||
| // scope, we lazy-load the module. | ||
| get: () => { | ||
| const api = require('@google-cloud/firestore-api').v1beta1; |
There was a problem hiding this comment.
Will @google-cloud/firestore-api be published to NPM? If so, it may be preferred to deprecate this v1beta1 property.
There was a problem hiding this comment.
Or simply remove this and the related type in ./types/...
| * @internal | ||
| * @name Firestore.GrpcStatus | ||
| * @type {function} | ||
| */ |
There was a problem hiding this comment.
Was this removal intentional? If so, can you clarify why?
| ) | ||
| s.replace( | ||
| "handwritten/firestore/types/protos/firestore_admin_v1_proto_api.d.ts", | ||
| 'import Long = require\("long"\);', |
There was a problem hiding this comment.
TODO we need to understand why this replacement was made and determine if we need a new workaround
|
|
||
| # Mark v1beta1 as deprecated | ||
| s.replace( | ||
| "handwritten/firestore/dev/src/v1beta1/firestore_client.ts", |
There was a problem hiding this comment.
We may need to migrate this deprecation to the new implmementation, or simply mark the v1beta1 property deprecated in index.ts?
| os.unlink('handwritten/firestore/dev/protos.js') | ||
| os.unlink('handwritten/firestore/dev/protos.d.ts') | ||
| os.unlink('handwritten/firestore/dev/protos.json') | ||
| subprocess.run('handwritten/firestore/dev/protos/update.sh', shell=True) |
There was a problem hiding this comment.
I'm assuming we should also remove this script update.sh which is compiling the protos?
There was a problem hiding this comment.
Perhaps even remove all of handwritten/firestore/dev/protos/... and handwritten/firestore/dev/protos.(js|d.ts|json)
| return new (api.FirestoreClient as any)(...args); | ||
| }; | ||
| return Object.assign(fn, api); | ||
| }, |
There was a problem hiding this comment.
Wondering if we also need to update the related manually generated type exports in .types/firestore.d.ts lines 2990 - 3005
| return new (api.FirestoreClient as any)(...args); | ||
| }; | ||
| return Object.assign(fn, api); | ||
| }, |
There was a problem hiding this comment.
Wondering if we also need to update the related manually generated type exports in .types/firestore.d.ts lines 2990 - 3005
| shell.run(["npm", "run", "compile"], cwd="handwritten/firestore", hide_output=True) | ||
| s.copy("handwritten/firestore/build/src/v1/firestore*.d.ts", "handwritten/firestore/types/v1") | ||
| s.copy("handwritten/firestore/build/src/v1beta1/firestore_client.d.ts", "handwritten/firestore/types/v1beta1") | ||
| s.copy("handwritten/firestore/build/protos/firestore*.d.ts", "handwritten/firestore/types/protos") |
There was a problem hiding this comment.
possibly also remove handwritten/firestore/types/v1, handwritten/firestore/types/v1beta1, handwritten/firestore/types/protos
| source_location="build/src", test_project="node-gcloud-ci" | ||
| ) | ||
|
|
||
| s.copy(templates, destination="handwritten/firestore", excludes=[".eslintrc.json", ".kokoro/**/*", ".github/CODEOWNERS", "README.md"]) |
There was a problem hiding this comment.
Are the CODEOWNERS and README.md still getting generated with Librarian? Or are they even still useful with the new repo structure?
2 participants
Decouple the handwritten Firestore wrapper from the auto-generated GAPIC client layer by importing a standalone version of the GAPIC library. Standalone
-apipackages are now generated and maintained independently.dev/src/v1/,dev/src/v1beta1/) and OwlBot files (owlbot.py,.OwlBot.yaml).@google-cloud/firestore-api(^0.2.0).v1,v1beta1, andprotosfrom@google-cloud/firestore-api.module.exports.v1andmodule.exports.v1beta1to preserve backwards-compatible constructor behavior (new firestorePkg.v1(...)).Internal: b/531788771