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

Inherit top-level type (e.g. PlainMessage) #230

Closed
faustbrian opened this issue Sep 24, 2022 · 7 comments
Closed

Inherit top-level type (e.g. PlainMessage) #230

faustbrian opened this issue Sep 24, 2022 · 7 comments

Comments

@faustbrian
Copy link

faustbrian commented Sep 24, 2022

I have some user-facing functions that take PlainMessage<T> as input. The problem is that PlainMessage doesn't seem to trickle down to properties that themselves are messages. Take the below snippets as an example:

Protobuf

syntax = "proto3";

option optimize_for = SPEED;

message Op {
  string denomination = 1;
}

message Msg {
  repeated Op ops = 1;
}
// Type
type x = PlainMessage<Msg>

// Expected
type x = {
    ops: PlainMessage<Op>[];
}

// Actual
type x = {
    ops: Op[];
}

When using PlainMessage<T> I would expect all properties of T that are also messages to be also referenced as PlainMessage rather than expecting the (full) Message. Is this an intentional design choice and can this be worked around without a custom type outside the library or is this an oversight? Thanks in advanced!

@timostamm
Copy link
Member

Hey Brian, thanks for this issue!

The behavior is a design choice - the type is an exact representation for a message cloned with the spread operator: let plain: PlainMessage<Example> = {...example};

I'm aware that this is not always helpful in practice. Making PlainMessage<T> recursive is definitely an option, but I'm not quite sure yet that it is the best option. So before we consider this, we will be looking into generating an interface IExample for every message Example alongside the class, which behaves exactly like a recursive PlainMessage<T>.

The rationale for interface IExample is that a simple interface can be a bit more approachable and can provide better TypeScript error messages than a mapped type. It also means we would generate a bit more code, so we obviously want to consider this carefully.

Expect to see some updates regarding this soonish. In the meantime, I don't see any complications from using your own recursive version of PlainMessage<T>. If you do, please do let us know about how it works out for you!

@timostamm
Copy link
Member

A recursive version of PlainMessage:

import type { Message } from "@bufbuild/protobuf";

export type Plain<T extends Message> = {
  [P in keyof T as T[P] extends Function ? never : P]: PlainField<T[P]>;
};

// prettier-ignore
type PlainField<F> =
  F extends (Date | Uint8Array | bigint | boolean | string | number) ? F
  : F extends Array<infer U> ? Array<PlainField<U>>
  : F extends ReadonlyArray<infer U> ? ReadonlyArray<PlainField<U>>
  : F extends Message ? Plain<F>
  : F extends OneofSelectedMessage<infer C, infer V> ? { case: C; value: Plain<V> }
  : F extends { case: string | undefined; value?: unknown } ? F
  : F extends { [key: string|number]: Message<infer U> } ? { [key: string|number]: Plain<U> }
  : F;

type OneofSelectedMessage<K extends string, M extends Message> = {
  case: K;
  value: M;
};

@faustbrian
Copy link
Author

@timostamm Thanks! The Plain type does exactly what I needed, a lot nicer to work with without having to plaster my code with @ts-ignore since the underlying code works as expected 😅 Separate interfaces would be nice, could probably also be used to generate mocks from those interfaces if someone had the need for that.

@sjbarag
Copy link

sjbarag commented Oct 11, 2022

@timostamm that Plain type looks wonderful! Is there any chance of including that next to PlainMessage<> and PartialMessage<> in @bufbuild/protobuf?

@timostamm
Copy link
Member

Absolutely, @sjbarag. We're just wrapping up some work on the plugin framework, but this issue is up next!

@smaye81
Copy link
Member

smaye81 commented Nov 28, 2022

@faustbrian @sjbarag We landed #308 today, which makes the current PlainMessage type recursive. Closing this as a result, so let us know if this change works for you. If you notice any problems, feel free to open another issue.

@smaye81 smaye81 closed this as completed Nov 28, 2022
@sjbarag
Copy link

sjbarag commented Nov 28, 2022

Excellent, thanks @smaye81 !

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

No branches or pull requests

4 participants