-
Notifications
You must be signed in to change notification settings - Fork 55
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.
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
[FEAT] add method to bind to existing KVs #276
Conversation
nats-base-client/jsclient.ts
Outdated
@@ -92,6 +93,9 @@ class ViewsImpl implements Views { | |||
kv(name: string, opts: Partial<KvOptions> = {}): Promise<KV> { | |||
return Bucket.create(this.js, name, opts); | |||
} | |||
bind(name: string, codec?: KvCodecs): Promise<KV> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be triggered by an option in KvOptions. - the views interface should simply be the raw functionality - kv, object store, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aricart so basically additional "bind" option in KvOptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
went with bindOnly
as thats more self-explenatory and clear IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - with the exception of that arg should be optional and we should prepare for other things without flipping the signature.
nats-base-client/kv.ts
Outdated
static async bind( | ||
js: JetStreamClient, | ||
name: string, | ||
codec: KvCodecs = NoopKvCodecs(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make the sig (js: JetStreamClient, name: string, opts: {codec: KvCodec} = {})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This method allows binding to existing KV without creating it.