feat: add BatchWriteRequests to WriteBatch.commit() #961
Conversation
dev/src/write-batch.ts
Outdated
| return this._firestore | ||
| .request<api.IBatchWriteRequest, api.BatchWriteResponse>( | ||
| 'batchWrite', | ||
| request as api.IBatchWriteRequest, |
There was a problem hiding this comment.
I am leaning towards recommending that you should have two different methods. The new method would be fairly short since we can't support implicit transactions directly.
We should also think about if we need a dummy request for batch writes to make sure the network is initialized.
There was a problem hiding this comment.
Separated out logic into batchCommit() method
dev/src/write-batch.ts
Outdated
| writeResult.updateTime | ||
| ? Timestamp.fromProto(writeResult.updateTime) | ||
| : null, | ||
| status.code as Status |
There was a problem hiding this comment.
This should be the status with its error message.
There was a problem hiding this comment.
Added an RpcStatus interface to hold the Status, not sure if that's what you were looking for
dev/src/write-batch.ts
Outdated
| requestTag?: string; | ||
| }, | ||
| useBatch = false | ||
| ): Promise<WriteResult[] | BatchWriteResult[]> { |
There was a problem hiding this comment.
I think this can always return BatchWriteResult. commit() can drop the unused Status.
There was a problem hiding this comment.
N/A since I separated out the logic
thebrianchen
left a comment
There was a problem hiding this comment.
I separated out the BatchWriteRequest logic from commit() into its own method, WDYT?
dev/src/write-batch.ts
Outdated
| return this._firestore | ||
| .request<api.IBatchWriteRequest, api.BatchWriteResponse>( | ||
| 'batchWrite', | ||
| request as api.IBatchWriteRequest, |
There was a problem hiding this comment.
Separated out logic into batchCommit() method
dev/src/write-batch.ts
Outdated
| requestTag?: string; | ||
| }, | ||
| useBatch = false | ||
| ): Promise<WriteResult[] | BatchWriteResult[]> { |
There was a problem hiding this comment.
N/A since I separated out the logic
dev/src/write-batch.ts
Outdated
| writeResult.updateTime | ||
| ? Timestamp.fromProto(writeResult.updateTime) | ||
| : null, | ||
| status.code as Status |
There was a problem hiding this comment.
Added an RpcStatus interface to hold the Status, not sure if that's what you were looking for
dev/src/write-batch.ts
Outdated
| } | ||
|
|
||
| /** Helper type to hold a google.rpc.Status object. */ | ||
| interface RpcStatus { |
There was a problem hiding this comment.
I think the type we actually want to return is GoogleError which exists in GoogleGax and matches your interface. Can you use that type instead? This allows us to use the same type in all Promise rejections.
dev/src/write-batch.ts
Outdated
| * | ||
| * @private | ||
| */ | ||
| async batchCommit(commitOptions?: { |
There was a problem hiding this comment.
What do you think of the name "bulkCommit"? I would like to reduce confusion, and deciding between "commit" and "batchCommit" is going to be tough.
There was a problem hiding this comment.
I like that more -- the hard part was figuring out where to draw the line since we refer to it externally as bulk writes, but the backend API refers to it as batch writes.
|
@schmidt-sebastian Would you mind taking one last look? made some other changes that I want another look over (using || check, GoogleError usage) |
No description provided.