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.

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

Add context values to clients #841

Merged
merged 4 commits into from
Oct 3, 2023
Merged

Add context values to clients #841

merged 4 commits into from
Oct 3, 2023

Conversation

srikrsna-buf
Copy link
Member

Add context values to clients. This adds support for ContextValues in clients via CallOptions and Interceptors:

import { ElizaService } from "gen/...";
import { createPromiseClient } from "@connectrpc/connect";
import transport from "./transport.js";
import kUser from "user-context.js";

const client = createPromiseClient(ElizeService, trasport);

await client.say({ sentence: "Hi" }, { values: createContextValues().set(kUser, { ... }) });

Which can be accessed in an interceptor:

const tokenInterceptor = (next) => {
    return (req) => {
           req.header.set("Authorization", `Bearer ${req.values.get(kUser).token}`);
           return next(req);
    };
};

@paul-sachs
Copy link
Contributor

Love the simplicity. My one tiny nitpick is that the key on the Request object is just values, which on first read feels like it's some dictionary representation of the content of the request. Perhaps calling it context instead? I guess values just feels a touch generic.

@srikrsna-buf
Copy link
Member Author

Perhaps calling it context instead? I guess values just feels a touch generic.

I also prefer that, but a potential problem I see is that when we implement server-side interceptors, one of the possible designs is to just reuse the interceptor types that we use on the client. context could be confusing to users as they might expect HandlerContext.

If an alternate design for interceptors is possible then I don't see a problem with naming this context. My efforts to get TS to work have been unsuccessful so far I tried a couple of designs and the ones we have for the client seem the most promising but nevertheless we can wait until at least we finalize the design

@timostamm timostamm mentioned this pull request Oct 2, 2023
@timostamm
Copy link
Member

timostamm commented Oct 2, 2023

It's certainly a bit more verbose, but what about contextValues for the option name? It mirrors the name in #835.

Base automatically changed from sk/safe-context to main October 2, 2023 16:46
@srikrsna-buf
Copy link
Member Author

I changed the name to contextValues I also updated UniversalServerRequest. The only place it is called values is on HandlerContext.

@srikrsna-buf srikrsna-buf merged commit b2b9d8f into main Oct 3, 2023
4 checks passed
@srikrsna-buf srikrsna-buf deleted the sk/context-client branch October 3, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants