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 way to get MethodInfo from RPC function #164

Open
bluskript opened this issue Sep 23, 2021 · 4 comments
Open

No way to get MethodInfo from RPC function #164

bluskript opened this issue Sep 23, 2021 · 4 comments
Labels

Comments

@bluskript
Copy link

Let me just say, I'm very impressed by the extensibility of protobuf-ts. The reflection API is very useful and seems to meet almost any problems I might be facing. However, there are some problems which I feel could have a cleaner solution.

I'm trying to implement a simple batching system. I started implementing this using the following code:

  async batchSame<I extends object, O extends object>(
    method: MethodInfo<I, O>,
    requests: I[]
  ): Promise<O[]> {
    const req = BatchSameRequest.create({
      endpoint: `/${method.service.typeName}/${method.name}`,
      requests: requests.map((r) => method.I.toBinary(r)),
    });
    const { responses } = await this.batch.batchSame(req).response;
    return responses.map((raw) => method.O.fromBinary(raw));
  }

However, I quickly realized that there didn't really seem to be any way to access a MethodInfo without invoking a request... Looking at the generated code, I saw that the generated methods access specific indices, and there's no way to access them outside of that context...

    key(input: KeyRequest, options?: RpcOptions): UnaryCall<KeyRequest, KeyResponse> {
        const method = this.methods[2], opt = this._transport.mergeOptions(options), i = method.I.create(input);
        return stackIntercept<KeyRequest, KeyResponse>("unary", this._transport, method, opt, i);
    }

My proposal: Introduce a { [method: string]: MethodInfo<I,O> } and use that instead since it's more ergonomic to work with... Although performance might hurt a little... I'm not entirely sure.

Also, if anyone has ideas for how to temporarily circumvent this limitation, please let me know.

@timostamm
Copy link
Owner

I didn't do that to preserve method order, same with message fields. Having a plain array in the same order as the proto source is the best thing to have if you have zero knowledge about the types.

But what you seem to want is access to MethodInfo by name, while preserving input and output types.

We'd have to generate literals like this:

export const MyService = {
    typeName : "x.MyService",
    options: {},
    methods: {
        unary: {
            name: "Unary",
            localName: "unary",
            I: StringValue,
            O: Int32Value,
            serverStreaming: false,
            clientStreaming: false,
            options: {},
            idempotency: undefined,
        }
    }
} as const;

It would let you state batchSame(MyService.methods.unary, ...).

There are some issues:

  1. We only generate partial method info and use normalizeMethodInfo() to set default values, saving code size
  2. MethodInfo also has a service property but we don't have access to the service in the literal
  3. const typing lets code generation errors slip in more easily

I think it's a very interesting idea. Object.keys(MyService.methods) would retain the source order.

@timostamm
Copy link
Owner

@bluskript, reading this again I noticed that you can get away with a cast:

const mi = Haberdasher.methods.find(m => m.name === "MakeHat") as MethodInfo<Size, Hat>;
assert(mi !== undefined);

const requests: Size[] = []; 
batchSame(mi, requests);

It's not type safe, but will get the job done if you need it.

@bluskript
Copy link
Author

Thanks for the tip, I came up with a slightly more scuffed and unsafe solution quite recently but it works too:

export async function batchGetUsers(conn: Connection, users: string[]) {
	const data = await conn.batchSame(
		"/protocol.profile.v1.ProfileService/GetProfile",
		users.map(userId => ({
			userId,
		})),
		GetProfileRequest,
		GetProfileResponse,
	);
	return data.map(r => r.profile);
}

while I changed my batchSame function to look like this:

	async batchSame<I extends object, O extends object>(
		path: string,
		requests: I[],
		input: IMessageType<I>,
		output: IMessageType<O>,
	): Promise<O[]> {
		const req = BatchSameRequest.create({
			endpoint: path,
			requests: requests.map(r => input.toBinary(r)),
		})
		const { responses } = await this.batch.batchSame(req).response
		return responses.map(raw => output.fromBinary(raw))
	}

@bluskript
Copy link
Author

It would still be nice if a generalized solution for all methods can be made, without requiring additional codegen though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants