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

No export for UnaryImpl provided #788

Open
sachaw opened this issue Aug 29, 2023 · 7 comments
Open

No export for UnaryImpl provided #788

sachaw opened this issue Aug 29, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@sachaw
Copy link

sachaw commented Aug 29, 2023

When using modern Node module resolution, there is no export provided that gives access to the UnaryImpl type from https://github.com/connectrpc/connect-es/blob/main/packages/connect/src/implementation.ts
It is likely easiest to just re-export it, instead of having a dedicated entry point,

@sachaw sachaw added the bug Something isn't working label Aug 29, 2023
@timostamm
Copy link
Member

Hey Sasha, we do not export all types to keep the API surface manageable. It would be very difficult to make any non-breaking changes otherwise.

Can you get by with ServiceImpl or MethodImpl? They are documented here.

@timostamm
Copy link
Member

For example, here is how you could extract a specific signature:

import type {MethodInfo, PartialMessage,} from "@bufbuild/protobuf";
import {Int32Value, MethodKind, StringValue} from "@bufbuild/protobuf";
import type {HandlerContext, MethodImpl} from "@connectrpc/connect";

type U = MethodImpl<MethodInfo<Int32Value, StringValue> & { kind: MethodKind.Unary }>;

export const u: U = (request: Int32Value, context: HandlerContext): PartialMessage<StringValue> => {
    context.signal.throwIfAborted();
    return {value: request.value.toString()};
}

How do you want to use the type? Feedback is much appreciated, so that we can consider it going forward.

@sachaw
Copy link
Author

sachaw commented Aug 29, 2023

This is how I'm currently using it:

  getUsers: UnaryImpl<GetUsersRequest, GetUsersResponse> = async (req, ctx) => {
    return new GetUsersResponse();
  };

as opposed to:

  async getUsers(
    req: GetUserRequest,
    ctx: HandlerContext,
  ) {
    return new GetUsersResponse();
  }

Doesn't really matter either way, it's also the way that most editors with intellisense will scaffold the property for you

@timostamm
Copy link
Member

Ah, so the IDE is scaffolding an implementation with UnaryImpl for you? I agree that's not ideal and it sounds like we should improve it if possible.

Can you provide some details? I can't reproduce with VS code or JetBrains, but I might not be hitting the right buttons.

@sachaw
Copy link
Author

sachaw commented Aug 30, 2023

It appears that it's just using whatever the inferred type definition is to scaffold it:
image
My class definition is simply:

export class Model
  extends BaseService<string>
  implements ServiceImpl<typeof ModelService>

If the intended usage is to not use UnaryImpl that's fine, just some guidance on what should be preferred would be appreciated.

@timostamm
Copy link
Member

Thanks for the context, @sachaw.

The recommended implementation using classes would be:

class Model implements ServiceImpl<typeof ModelService> {
  addServer(request: AddServerRequest) {
    // do something with the request...
    return new AddServerResponse();
  }
}

Note that the typing for the request argument is necessary because implements does not infer, but if you were to specify a type that is not assignable, you get a compile time error.

@timostamm
Copy link
Member

I think we should be able to get a IDE suggestion matching our recommendation by "unrolling" the types for service methods:

export type MethodImpl<M extends MI> =
M extends MI<infer I, infer O, MethodKind.Unary> ? UnaryImpl<I, O>

Similar to how we do it for client methods:

export type PromiseClient<T extends ServiceType> = {
[P in keyof T["methods"]]:
T["methods"][P] extends MethodInfoUnary<infer I, infer O> ? (request: PartialMessage<I>, options?: CallOptions) => Promise<O>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants