connectrpc / connect-es

The TypeScript implementation of Connect: Protobuf RPC that works.
https://connectrpc.com/
Apache License 2.0
1.31k stars 73 forks source link

No export for `UnaryImpl` provided #788

Open sachaw opened 1 year ago

sachaw commented 1 year ago

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,

timostamm commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

https://github.com/connectrpc/connect-es/blob/83dbda37f44b4b2c3dbc91437c21c39eafa6d332/packages/connect/src/implementation.ts#L44-L45

Similar to how we do it for client methods:

https://github.com/connectrpc/connect-es/blob/83dbda37f44b4b2c3dbc91437c21c39eafa6d332/packages/connect/src/promise-client.ts#L39-L41