feat: setBatch and getBatch #1153
Conversation
| setBatch( | ||
| items: | ||
| | Record<string, string | Uint8Array> | ||
| | Map<string | Uint8Array, string | Uint8Array>, |
There was a problem hiding this comment.
i like how you did the types here to support both Record and Map!
packages/core/src/internal/clients/cache/AbstractCacheClient.ts
Outdated
Show resolved
Hide resolved
| * May contain nulls corresponding to cache misses. | ||
| * @returns {(string | undefined)[]} | ||
| */ | ||
| public values(): (string | undefined)[] { |
There was a problem hiding this comment.
I wonder if we should consider making the return type for this be a Record<string, string | uint>, or a Map.
I can definitely imagine cases where a user would just want them in order, but they could maybe achieve that sufficiently through results. It feels like the more natural / common / simple use case (which is what we want to prioritize for this .values function) might be to get back a structure that they can index into by key.
would really love to get other folks thoughts on this though, especially since is the first major SDK we're releasing this in and we will be treating it as a canonical example.
There was a problem hiding this comment.
yeah I like that idea considering they provided a Record or Map, they're probably expecting a similar structure back.
I had missed this before but it seems like dictionaryGetFields returns items as Record or Map as well
There was a problem hiding this comment.
💯 on the record/map as the default. In the past when we have exposed an array (eg in earlier iterations of dictionaryGetFields), I believe we found the record/map easier to use. My intuition is users reach for the record/map first as well, and if you had an array you would zip it with the keys anyway. So this saves you that step.
| call.on('data', (getResponse: grpcCache._GetResponse) => { | ||
| const result = getResponse.result; | ||
| switch (result) { | ||
| case grpcCache.ECacheResult.Hit: | ||
| results.push(new CacheGet.Hit(getResponse.cache_body)); | ||
| break; | ||
| case grpcCache.ECacheResult.Miss: | ||
| results.push(new CacheGet.Miss()); | ||
| break; | ||
| default: | ||
| results.push( | ||
| new CacheGet.Error(new UnknownError(getResponse.message)) | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| call.on('end', () => { | ||
| resolve(new GetBatch.Success(results, keys)); | ||
| }); | ||
|
|
||
| call.on('error', (err: ServiceError | null) => { | ||
| this.cacheServiceErrorMapper.resolveOrRejectError({ | ||
| err: err, | ||
| errorResponseFactoryFn: e => new GetBatch.Error(e), | ||
| resolveFn: resolve, | ||
| rejectFn: reject, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
For the backlog: (1) the three callbacks -- data, end and error -- have boilerplate we can abstract away; and (2) we should require the developer to implement all three.
Otherwise 👍
There was a problem hiding this comment.
@malandis wdym? is your comment targeted at future streaming APIs?
There was a problem hiding this comment.
or just consolidating the get/set ones I guess?
Adds batch get and set APIs for #674
Note: the streaming APIs use a different set of interceptors, this issue is still open: #349
Another note: this PR doesn't remove the nodejs batchutils functions