Compiler flag oneof=unions generates type unions for oneof fields instead of individual properties #95

philikon · 2020-06-19T08:04:47Z

Addresses #74

philikon · 2020-06-19T08:37:33Z

I can't see what the failure is, circleci wants me to log in. :(

cliedeman · 2020-06-19T09:24:16Z

Summary of all failing tests
 
 FAIL  integration/simple/simple-oneofs-test.ts
 
  ● Test suite failed to run
 

 
    ENOMEM: not enough memory, read
 

 
      at ScriptTransformer._getTransformer (node_modules/@jest/transform/build/ScriptTransformer.js:355:19)
 
      at ScriptTransformer.transformSource (node_modules/@jest/transform/build/ScriptTransformer.js:444:28)
 

 
 FAIL  integration/bytes-as-base64/bytes-as-base64-test.ts
 
  ● Test suite failed to run
 

 
    ENOMEM: not enough memory, read
 

 
      at Object.<anonymous> (node_modules/bs-logger/dist/logger/index.js:36:15)

A transient failure

@philikon This looks great. Will give it a try on my project

cliedeman · 2020-06-19T10:37:03Z

@philikon one thing I noticed

    PleaseChoose.fromPartial({
      name: 'Debbie',
      age: 37,
      choice: { aBool: true, aString: 'a' }, // Valid
      choice: { $case: 'aBool', aBool: true, aString: 'a' }, // Invalid
      eitherOr: { or: 'perhaps not' },
    });

the From Partial allows the user to omit the case.

This seems to fix it. but here be dragons...

type DeepPartial<T> = T extends Builtin
  ? T
  : T extends Array<infer U>
  ? Array<DeepPartial<U>>
  : T extends ReadonlyArray<infer U>
  ? ReadonlyArray<DeepPartial<U>>
  : T extends { $case: string }
  // Try to make $case mandatory
  ? { [K in keyof Omit<T, '$case'>]?: DeepPartial<T[K]> } & { $case: T['$case'] }
  : T extends {}
  ? { [K in keyof T]?: DeepPartial<T[K]> }
  : Partial<T>;

This forces the user to allows provide a $case

cliedeman · 2020-06-19T11:01:21Z

A more conceptual issue:

Proto

  oneof choice {

    // Use this if you want a number. Numbers are great. Who doesn't
    // like them?
    double a_number = 2;

    // Use this if you want a string. Strings are also nice. Not as
    // nice as numbers, but what are you going to do...
    string a_string = 3;

    Submessage a_message = 4;

    // We also added a bool option! This was added after the 'age'
    // field, so it has a higher number.
    bool a_bool = 6;

    bytes buncha_bytes = 10;
  }

Generated Code

  choice?:
    | { $case: 'aNumber'; aNumber: number }
    | { $case: 'aString'; aString: string }
    | { $case: 'aMessage'; aMessage: PleaseChoose_Submessage | undefined }
    | { $case: 'aBool'; aBool: boolean }
    | { $case: 'bunchaBytes'; bunchaBytes: Uint8Array };

I think that the generated code should actually be

  choice?:
    | { $case: 'aNumber'; aNumber: number }
    | { $case: 'aString'; aString: string }
    | { $case: 'aMessage'; aMessage: PleaseChoose_Submessage /* undefined removed */ }
    | { $case: 'aBool'; aBool: boolean }
    | { $case: 'bunchaBytes'; bunchaBytes: Uint8Array };

my reasoning is that these 2 cases are equivalent from a protobuf point of view

{ choice: { '$case': 'aMessage', aMessage: undefined }}
{ choice: undefined }

This also allows us to remove some tedious if statements

philikon · 2020-06-19T15:50:59Z

the From Partial allows the user to omit the case.

Yes, I think that's the whole point of fromPartial yes? :)

I think that the generated code should actually be
| { $case: 'aMessage'; aMessage: PleaseChoose_Submessage /* undefined removed */ }

Hmm, I need to think about this some more, but I think you might be right. Either you chose the aMessage case and then you need to provide the value, or you didn't want to provide a value at all, but then there's no point to provide a $case. Yeah, I think that makes sense, but before I go back into the code, I might want @stephenh and @timostamm to weigh in.

philikon · 2020-06-19T15:59:19Z

(Btw, in your example you can do switch (data.choice?.$case) and avoid the outermost if that way.)

cliedeman · 2020-06-19T17:36:59Z

Yes, I think that's the whole point of fromPartial yes? :)

Definitely. I guess its just a bit of a footgun because erroneous code would compile. But this was not the goal of the PR I think, because the existing code has the same issue that you attempt to set multiple oneOf fields.

I think a runtime exception in this case might be good enough? I tried to spin up a comparative example in go but the generation style prevents settings 2 cases of a one of field.

but then there's no point to provide a $case

I almost came to the same conclusion but then there is no clean way to narrow the type

switch (data.choice?.$case)

Thanks :), I was being intentionally pedantic to make sure typescript was doing the right thing.

timostamm · 2020-06-19T17:58:24Z

Wow, looks good, @philikon :)

{ choice: { '$case': 'aMessage', aMessage: undefined }}

I think $case should only point to aMessage when it has a value. But hey, this is miles better than before. The user should be well aware now that this is a oneof construct. Before, you would have to look at the .proto to notice.

Regarding fromPartial(): I think @cliedeman is on the right track. The following seems to work out for me:

export type PartialMessage<T> =
      T extends (Date | Uint8Array | Long | boolean | string | number) ?
        T
    : T extends Array<infer U> ?
        Array<PartialMessage<U>>
    : T extends ReadonlyArray<infer U> ?
        ReadonlyArray<PartialMessage<U>>
    : T extends { oneofKind: string } ?
        T
    : T extends { oneofKind: undefined } ?
        T
    : T extends {} ?
        { [K in keyof T]?: PartialMessage<T[K]> }
    : Partial<T>;

You can read it like a if, else if, ... else statement.

Don't think that this is a drop-in replacement, though. Have been working on yet another typescript plugin... Using the compiler api to generate .ts from .proto. Seems to work alright so far.

philikon · 2020-06-21T03:36:18Z

I think I understand the fromPartial concern now. @cliedeman, your example illustrates it perfectly and I think your proposed solution

T extends { $case: string }
  ? { [K in keyof Omit<T, '$case'>]?: DeepPartial<T[K]> } & { $case: T['$case'] }

works great. I think it's fine to require that if choice is filled out, you have to provide $case, but you don't have to provide anything else. I'll update the PR accordingly.

I'll also remove the optionality of message payloads inside the oneof as discussed.

…tead of individual properties Addresses stephenh#74

philikon · 2020-06-25T20:15:47Z

@stephenh any chance you could look at this over the next few days?

philikon · 2020-07-06T16:09:01Z

Ping?

stephenh · 2020-07-06T16:28:42Z

@philikon yep, sorry, thanks for the patience; I spent my free cycles over the weekend hacking on something else. Will get to this this week.

philikon · 2020-07-06T19:30:39Z

Awesome, thanks

stephenh

Looks really great, will merge and release. Thanks!

stephenh · 2020-07-11T16:10:36Z

src/types.ts

+  //
+  // When oneof=unions, we generate a single property for the entire `oneof`
+  // clause, spelling each option out inside a large type union. No need for
+  // union with `undefined` here, either.


Great comment, thanks!

stephenh · 2020-07-11T16:30:02Z

src/main.ts

+  if (options.oneof === OneofOption.UNIONS) {
+    oneofCase = `
+  : T extends { $case: string }
+  ? { [K in keyof Omit<T, '$case'>]?: DeepPartial<T[K]> } & { $case: T['$case'] }`;


stephenh · 2020-07-11T16:31:42Z

src/main.ts

+
+  // Ideally we'd put the comments for each oneof field next to the anonymous
+  // type we've created in the type union above, but ts-poet currently lacks
+  // that ability. For now just concatenate all comments into one big one.


Thanks again for the in-source comment.

stephenh · 2020-07-11T16:32:29Z

src/main.ts

@@ -604,6 +679,9 @@ function generateDecode(
          .addStatement(`message.%L.push(%L)`, fieldName, readSnippet)
          .endControlFlow();
      }
+    } else if (isWithinOneOf(field) && options.oneof === OneofOption.UNIONS) {
+      let oneofName = maybeSnakeToCamel(messageDesc.oneofDecl[field.oneofIndex].name, options);
+      func = func.addStatement(`message.%L = {$case: '%L', %L: %L}`, oneofName, fieldName, fieldName, readSnippet);


Huh, that wasn't too bad. Nice.

philikon · 2020-07-14T01:24:32Z

Woot! Thank you!

philikon force-pushed the oneofUnions branch from 6fcfe24 to 154e2fb Compare June 19, 2020 08:27

philikon force-pushed the oneofUnions branch 2 times, most recently from 5798b3a to 2cf9ebe Compare June 21, 2020 03:55

Compiler flag oneof=unions generates type unions for oneof fields ins…

8938c51

…tead of individual properties Addresses stephenh#74

philikon force-pushed the oneofUnions branch from 2cf9ebe to 8938c51 Compare June 25, 2020 05:25

stephenh approved these changes Jul 11, 2020

View reviewed changes

stephenh merged commit ce3ca37 into stephenh:master Jul 11, 2020

philikon deleted the oneofUnions branch July 14, 2020 01:24

zfy0701 added a commit to sentioxyz/ts-proto that referenced this pull request Jan 5, 2023

chore(sdk): definte basic trace handling schema (stephenh#95)

9e95f6f

stephenh mentioned this pull request Oct 13, 2024

Support for comments in oneof fields #1122

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compiler flag oneof=unions generates type unions for oneof fields instead of individual properties #95

Compiler flag oneof=unions generates type unions for oneof fields instead of individual properties #95

philikon commented Jun 19, 2020

philikon commented Jun 19, 2020

cliedeman commented Jun 19, 2020

cliedeman commented Jun 19, 2020

cliedeman commented Jun 19, 2020

philikon commented Jun 19, 2020 •

edited

Loading

philikon commented Jun 19, 2020

cliedeman commented Jun 19, 2020

timostamm commented Jun 19, 2020

philikon commented Jun 21, 2020

philikon commented Jun 25, 2020

philikon commented Jul 6, 2020

stephenh commented Jul 6, 2020

philikon commented Jul 6, 2020

stephenh left a comment

stephenh Jul 11, 2020

stephenh Jul 11, 2020

stephenh Jul 11, 2020

stephenh Jul 11, 2020

philikon commented Jul 14, 2020

Compiler flag oneof=unions generates type unions for oneof fields instead of individual properties #95

Compiler flag oneof=unions generates type unions for oneof fields instead of individual properties #95

Conversation

philikon commented Jun 19, 2020

philikon commented Jun 19, 2020

cliedeman commented Jun 19, 2020

cliedeman commented Jun 19, 2020

cliedeman commented Jun 19, 2020

philikon commented Jun 19, 2020 • edited Loading

philikon commented Jun 19, 2020

cliedeman commented Jun 19, 2020

timostamm commented Jun 19, 2020

philikon commented Jun 21, 2020

philikon commented Jun 25, 2020

philikon commented Jul 6, 2020

stephenh commented Jul 6, 2020

philikon commented Jul 6, 2020

stephenh left a comment

Choose a reason for hiding this comment

stephenh Jul 11, 2020

Choose a reason for hiding this comment

stephenh Jul 11, 2020

Choose a reason for hiding this comment

stephenh Jul 11, 2020

Choose a reason for hiding this comment

stephenh Jul 11, 2020

Choose a reason for hiding this comment

philikon commented Jul 14, 2020

philikon commented Jun 19, 2020 •

edited

Loading