Introduce method descriptor

[paul-sachs]

This new type is to be used as a self describing entity of a service + method. Useful for enclosing all required information to implement a standalone method.

[timostamm]

This type is desirable because there are many contexts where we - and users - only want to refer to a single RPC.

The most prominent case is a query in connect-query, but we have other use cases in connect-es:

1. ConnectRouter

ConnectRouter has an rpc method to register a single RPC:

function routes(router: ConnectRouter) {
  router.rpc(
    ElizaService,
    ElizaService.methods.say,
    async (req) => new SayResponse({
      sentence: `you said: ${req.sentence}`
    })
  );
}

If we generate this new method type in protoc-gen-connect-query, and accept it in ConnectRouter.rpc, users can use createRouterTransport to mock endpoints without needing additional code generation.

But we can't support both types forever. More choices also means more complexity, ultimately making the API more difficult to use. So ideally, it should also be possible to use ConnectRouter.rpc with code generated by protoc-gen-connect-es (a ServiceType).

2. Transport

The unary and stream methods of Transport always refer to a single RPC, but require both service and method:

interface Transport {
  unary<I extends Message<I> = AnyMessage, O extends Message<O> = AnyMessage>(
    service: ServiceType,
    method: MethodInfo<I, O>,
    // ...
  ): Promise<UnaryResponse<I, O>>;

  /**
   * Call a streaming RPC - a method that takes zero or more input messages,
   * and responds with zero or more output messages.
   */
  stream<I extends Message<I> = AnyMessage, O extends Message<O> = AnyMessage>(
    service: ServiceType,
    method: MethodInfo<I, O>,
    // ...
  ): Promise<StreamResponse<I, O>>;
}

Clearly, the new method type is the better fit in this case. The idea is to only generate the new method type for connect-query. Ultimately, Transport should also use the new method type, and code generated by protoc-gen-connect-es (a ServiceType) should be compatible.

3. Universal handlers

UniversalHandler is specific to a single RPC, but includes a full ServiceType.

4. Handler factory

ProtocolHandlerFactory accepts a MethodImplSpec, which includes a full ServiceType, but is specific to a single RPC.

---

In this project, we have createRegistryFromDescriptors(), which should also be able to provide the new method type for consistency - but also for practical use cases where you would want to use any of the cases above from dynamic types, for example from descriptors retrieved from a reflection endpoint.

Before we add new exports to this project, we need to fully consider these use cases (I am not sure the list above is exhaustive) to maintain good DX in the API.

To get the ball rolling, I suggest that we do not export any new types, and instead accept an override in ConnectRouter.rpc specifically. This change will have the least consequences, meaning that we can still pivot without breaking too many eggs.

[timostamm]

The diff between MethodInfo and the new MethodType is:

  readonly name: string;
  readonly I: MessageType<I>;
  readonly O: MessageType<O>;
  readonly idempotency?: MethodIdempotency;
+ readonly localName: string;
+ readonly service: Omit<ServiceType, "methods">;

1. Can we omit methods from service and still use a full ServiceType for the property?
2. Do we need localName? It makes it much more cumbersome to construct a MethodType from just a ServiceType.

Answering these two questions is IMO necessary to nail down what protoc-gen-connect-query should generate, and how ConnectRouter.rpc needs to be changed.

[paul-sachs]

@timostamm

To get the ball rolling, I suggest that we do not export any new types, and instead accept an override in ConnectRouter.rpc specifically. This change will have the least consequences, meaning that we can still pivot without breaking too many eggs.

That makes sense to me. I'll close this PR and open one in connect-es instead so we can have a unified location to discuss the specifics. I'll provide more feedback over there on the other questions.

[timostamm]

I'll close this PR and open one in connect-es instead so we can have a unified location to discuss the specifics. I'll provide more feedback over there on the other questions.

Can we continue the conversation in this thread? It works for me to link here from a PR from connect-es, but I'd prefer that we don't fragment the discussion.

[paul-sachs]

@timostamm

1. Can we omit methods from service and still use a full ServiceType for the property?

I'm not 100% sure what you mean. The omitted field does allow us to assign a full ServiceType value to this, it just doesn't allow us to rely on it downstream.

Do we need localName? It makes it much more cumbersome to construct a MethodType from just a ServiceType.

From what I can see, it's not required. @srikrsna-buf was there a specific use case you were thinking of?

[srikrsna-buf]

From what I can see, it's not required. @srikrsna-buf was there a specific use case you were thinking of?

I included it so that we can use the router, if we are changing the router then we don't need it.

[paul-sachs]

I included it so that we can use the router, if we are changing the router then we don't need it.

Alright, good to know. Since I think it does make sense to modify the router to allow the MethodType as a param, that will solve that problem. I'll remove localName from my PR to connect.

[timostamm]

I included it so that we can use the router, if we are changing the router then we don't need it.

@srikrsna-buf, can you elaborate? I'm not aware that localName is necessary anywhere with createConnectRouter() when RPCs are registered, but of course I might be missing something.

[timostamm]

1. Can we omit methods from service and still use a full ServiceType for the property?

I'm not 100% sure what you mean. The omitted field does allow us to assign a full ServiceType value to this, it just doesn't allow us to rely on it downstream.

I mean excess property checks.

When we accept self-contained method types across the APIs, we will still have to handle the regular MethodInfo. protoc-gen-connect-es will still generate them, it's still the best literal structure for a service.

So interop becomes important. It must be possible to create MethodTypes from a ServiceType, for example with an object spread:

const say = {...ElizaService.methods.say, service: ElizaService}

I don't want us to find out that we have to make the property optional instead of omitting it down the line, for any of the use cases above, after we have shipped the type.

[paul-sachs]

I don't want us to find out that we have to make the property optional instead of omitting it down the line, for any of the use cases above, after we have shipped the type.

That makes sense. I'll double check and write some tests around it if it's not the case.

[paul-sachs]

It seems like the inference used in rpc definitely ignores the excess properties. The only way I could get it to fail was like so:

const methodDefinition: MethodType<Int32Value, StringValue> = {
  ...testService.methods.unary,
  service: {
    //...testService works fine
    typeName: testservice.typeName,
    methods: testService.methods,
  }
};

And even this will work as long as you don't have the type declaration of MethodType<Int32Value, StringValue> (and use as const). Since these services are generated, I feel like this is an acceptable solution. We could of course just make it an optional property but that is less accurate.

[timostamm]

Thanks for verifying!

There is a long-standing feature request to expand excess property checks: microsoft/TypeScript#39998. But I don't think it would apply for const say = {...ElizaService.methods.say, service: ElizaService}.

Seems good to me to omit methods completely.

[srikrsna-buf]

@srikrsna-buf, can you elaborate? I'm not aware that localName is necessary anywhere with createConnectRouter() when RPCs are registered, but of course I might be missing something.

When we call router.service we need to provide an object with keys that match the local names of the rpcs.

[timostamm]

@srikrsna-buf, can you elaborate? I'm not aware that localName is necessary anywhere with createConnectRouter() when RPCs are registered, but of course I might be missing something.

When we call router.service we need to provide an object with keys that match the local names of the rpcs.

True. router.service() needs localName as a guaranteed safe property name for the service object. Similar with create*Client(). It will not be possible to go from MethodTypes to a ServiceType, only from a ServiceType to MethodTypes.

Seems to be an acceptable limitation, though. We might even want to consider replacing localName with template literal types in the future to remove the limitation.