Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add new storage clients to nodejs/web sdks #1318

Merged
merged 15 commits into from
Jun 21, 2024
Merged

feat: add new storage clients to nodejs/web sdks #1318

merged 15 commits into from
Jun 21, 2024

Conversation

bruuuuuuuce
Copy link
Contributor

@bruuuuuuuce bruuuuuuuce commented Jun 7, 2024 •
edited by rishtigupta
Loading

relies on momentohq/client-protos#285 to get merged first in order to consume protos from npmjs rather than locally

@bruuuuuuuce bruuuuuuuce marked this pull request as ready for review June 7, 2024 16:05
@bruuuuuuuce bruuuuuuuce requested a review from a team June 7, 2024 16:05
@cprice404 cprice404 self-requested a review June 12, 2024 00:45
cprice404
cprice404 reviewed Jun 18, 2024
View reviewed changes
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just clearing this off of my review list for now, please add me again when it's ready for 👀 !

cprice404
cprice404 reviewed Jun 19, 2024
View reviewed changes
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just took a quick peek. I have to step afk for a bit but I probably want to experiment with pulling the values out into their own class or interface and see how that feels compared to this.

packages/client-sdk-nodejs/src/config/storage-configuration.ts Show resolved Hide resolved
packages/client-sdk-nodejs/src/config/transport/storage/grpc-configuration.ts Outdated Show resolved Hide resolved
packages/client-sdk-nodejs/src/config/transport/storage/grpc-configuration.ts Outdated Show resolved Hide resolved
packages/client-sdk-nodejs/src/config/transport/storage/grpc-configuration.ts Outdated Show resolved Hide resolved
packages/client-sdk-nodejs/src/config/transport/storage/transport-strategy.ts Outdated Show resolved Hide resolved
packages/client-sdk-nodejs/src/internal/storage-control-client.ts Outdated Show resolved Hide resolved
packages/client-sdk-nodejs/src/internal/storage-control-client.ts Show resolved Hide resolved
packages/client-sdk-nodejs/src/internal/storage-data-client.ts Show resolved Hide resolved
packages/client-sdk-nodejs/src/internal/storage-data-client.ts Outdated Show resolved Hide resolved
packages/core/src/messages/responses/storage/scalar/storage-get.ts Outdated Show resolved Hide resolved
malandis
malandis reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@malandis malandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very polished and thorough. I have a suggestion re: ItemNotFoundError and the enum. The rest are minor concerns.

packages/client-sdk-nodejs/src/errors/cache-service-error-mapper.ts Show resolved Hide resolved
packages/client-sdk-nodejs/src/errors/cache-service-error-mapper.ts Show resolved Hide resolved
packages/core/src/errors/errors.ts Outdated Show resolved Hide resolved
packages/core/src/errors/errors.ts Outdated Show resolved Hide resolved
packages/client-sdk-web/src/errors/cache-service-error-mapper.ts Show resolved Hide resolved
packages/client-sdk-web/src/storage-client-props.ts Show resolved Hide resolved
packages/common-integration-tests/src/storage.ts Outdated Show resolved Hide resolved
packages/core/src/messages/responses/storage/scalar/storage-get.ts Outdated Show resolved Hide resolved
packages/core/src/messages/responses/storage/scalar/storage-value.ts Outdated Show resolved Hide resolved
packages/core/src/messages/store-info.ts Outdated Show resolved Hide resolved
@rishtigupta
Copy link
Contributor

I fixed s/ItemNotFoundError/StoreItemNotFoundError in the pr

@rishtigupta rishtigupta merged commit 5549eee into main Jun 21, 2024
13 checks passed
@rishtigupta rishtigupta deleted the feat/storageClients branch June 21, 2024 00:03
@rishtigupta rishtigupta mentioned this pull request Jun 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anitarua anitarua anitarua left review comments

@rishtigupta rishtigupta rishtigupta left review comments

@malandis malandis malandis approved these changes

@pgautier404 pgautier404 Awaiting requested review from pgautier404

@cprice404 cprice404 Awaiting requested review from cprice404

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bruuuuuuuce @rishtigupta @cprice404 @malandis @pgautier404 @anitarua