-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add the open/closed behavior for enums to documentation #522
Comments
This is in line with the official specification for enums (at least for proto3/open enums). See the official and unofficial docs.
FWIW you could likely make your function less expensive by changing it to something like: function knownEnumOrDefault<T extends Enum>(value: T[keyof T], e: T): T[keyof T] {
return e[e[value]] ?? e[e[0]] as T[keyof T];
} |
Thanks for the explanation, that sounds reasonable. It would be great to document this behavior, e.g. under Using enumerations 👍 |
Fair point. We could make this a bit more evident through our docs. |
Unfortunately the proto3/open enums don't play very well with TS enums (or rather, the developer experience around them). The Most Correct™️ type for an field in a message interface would be something like: enum SomeEnum {
UNSET = 0,
FOO = 1,
BAR = 2
}
interface MyMessage {
// number is for potentially unknown enum values in a proto3/open enum
myEnum: SomeEnum | number;
} Unfortunately because declare var m: MyMessage;
if (SomeEnum[m.myEnum]) {
m.myEnum;
// ^? (property) MyMessage.myEnum: number
} So you either have to cast it (🤢) or make a function to cast it (😕): if (SomeEnum[m.myEnum]) {
m.myEnum as SomeEnum;
// ^? (property) MyMessage.myEnum: SomeEnum
}
function isKnownSomeEnumValue(val: number): val is SomeEnum {
return Boolean(SomeEnum[val]);
}
if (isKnownSomeEnumValue(m.myEnum)) {
m.myEnum;
// ^? (property) MyMessage.myEnum: SomeEnum
} I think the only way to make the DX less bad (at least for performing the refinement) would be to represent protobuf enums as string unions and enum fields as: This allows much easier refinements between known/unknown values for an enum field: type SomeEnum = 'UNSET' | 'FOO' | 'BAR';
interface MyMessage {
// number is for potentially unknown enum values in a proto3/open enum
myEnum: SomeEnum | number;
}
if (typeof m.myEnum === 'string') {
m.myEnum;
// ^? (property) MyMessage.myEnum: SomeEnum
} |
As @jcready commented, this is desired behavior for proto3, and - at least at the time the implementation was written - it matches the behavior of TypeScript enumerations, which also accept arbitrary numbers: enum Foo {
A = 1,
B = 2,
}
const f: Foo = 3; // compiles with TS 4.9.5 TypeScript v5 changes the behavior of enums, making them closed in the type system - see the note at the bottom of the PR description here: microsoft/TypeScript#50528. We will have to see how we can improve the mismatch. |
Hey there,
first of all, thanks a lot for maintaining this library, what a breath of fresh air to the grpc ecosystem 👍
We noticed that enum values are decoded "naively" into their integer values, without any bounds check. This may cause out-of-type values on the typescript side, whereas my expectations from protobuf would be to receive the default value for such cases.
I'm not sure what the best way to handle this is. Checking for value correctness comes with a significant performance overhead since it needs to compare against a specific set of known values. But on the other hand, out-of-type values in typescript are very unexpected and can lead to severe consequences. Perhaps a good middle ground is to simply document this behavior? Or make it a configurable performance vs correctness option?
For the record, we are now using this function on all incoming enums to ensure correctness:
The text was updated successfully, but these errors were encountered: