fix: oneof=union breaks wrapper types #458 #462

boukeversteegh · 2022-01-04T21:15:09Z

Tests and fixes the problem that the unwrap functions need to consider oneof=union option. See #458 .

Added this case to the existing integration test that has oneof=unions enabled.
Fixed a few small issues here and there on the way
- my original unwrapping code accidentally used the fieldName to read properties from the passed object, instead of the jsonName. fixed.
- made the generated code somewhat more readable by moving screen-filling boilerplate into a method (isObject and isSet)

During implementation, I found out that protobuf.js doesn't unwrap Value objects, so it becomes incompatible.
Originally, I wanted to solve this with an extra option to ts-proto, that 'unwraps' Value objects within fromJSON, but it made things complicated and broke other things, so I decided to keep things simple for now and just work around the issue in the test itself.

stephenh

Few minor questions but overall looks great, thanks @boukeversteegh !

stephenh · 2022-01-05T14:56:00Z

integration/batching-with-context/batching.ts

@@ -78,7 +78,9 @@ export const BatchQueryRequest = {

  fromJSON(object: any): BatchQueryRequest {
    const message = createBaseBatchQueryRequest();
-    message.ids = (object.ids ?? []).map((e: any) => String(e));
+    if (Array.isArray(object?.ids)) {


I like the isSet helper method, but for the arrays, @webmaster128 had just done quite a bit of work to rework all of our assignments to be one-liners, with the idea of a) minimizing code size, and b) eventually turning these into object literals, like:

return { a: ..., b: ...., c: ..., }

Which in theory should (with some handwaving) help v8 optimiziations.

Can we keep the previous ?? [] approach?

I see, so the idea is to get rid of the createBase...() call?

It made me wonder already why the default values for each field were overwritten each time. Thanks for clarifying, now it makes more sense.

I'll change back to the ternary style.

I see, so the idea is to get rid of the createBase...() call

Yep! @aikoven has an in-flight PR that is either doing this soon, or at least moving us closer to that I think.

@stephenh Ah I didn't know @aikoven was already working on this. So I actually just made a PR #463 to do what you described. It was kind of simple as I was already deep into this part of the code, and had a solution in mind for the trickier 'oneof' case.

So, @aikoven, if you also had something in the works and want to finish that instead, please close my PR. Sorry if I got in the way.

Thanks for adapting. Yeah the overall direction is to move away from mutability where you never know for sure what is set when. So a lot of recent work sent into exactly one assignment per field.

This PR nicely adds to that journey

I see, so the idea is to get rid of the createBase...() call?

We can easily get rid of it in fromJSON and fromPartial. For decode it will probably still be required as we are reading the serialized data into the object in unknown order and not all fields are serialized.

Yeah, for decode I was thinking that calling createBase() to get an object literal with every field assigned (right now I don't think it does literally every field, just some primitives), and then have the decode for loop just assign over existing keys, would be most likely to trigger the supposedly / I'm-admittedly-cargo-culting-a-little-bit optimization of "create objects with all keys set and then don't add/remove keys". :-)

right now I don't think it does literally every field, just some primitives

That changed in #457, see e.g. #457 (comment). Now all fields are set to default and decode can override them.

stephenh · 2022-01-05T14:57:21Z

src/main.ts

+  );
+
+  const isSet = conditionalOutput(
+    'isSet',


stephenh · 2022-01-05T15:07:00Z

src/main.ts

-    chunks.push(code`unwrap(message: Value): string | number | boolean | Object | null | Array<any> | undefined {
+    if (ctx.options.oneof === OneofOption.UNIONS) {
+      chunks.push(code`unwrap(message: Value): string | number | boolean | Object | null | Array<any> | undefined {
+        if (message.kind?.$case === 'nullValue') {


Ah sure, this makes sense...

Fwiw I was looking for a struct.ts that had this code in it, but don't see it ... looks like you did change the oneof-union test case to use a struct, but maybe the new google/protobuf/struct.ts just didn't get checked in as part of the PR?

Great catch! Wow :-) Just pushed it.

stephenh

Looks great, thanks @boukeversteegh !

boukeversteegh · 2022-01-05T16:39:51Z

Looks great, thanks @boukeversteegh !

Thank you for your review and coordination!

## [1.97.1](v1.97.0...v1.97.1) (2022-01-05) ### Bug Fixes * oneof=union breaks wrapper types [#458](#458) ([#462](#462)) ([dd16992](dd16992))

stephenh · 2022-01-05T16:46:17Z

🎉 This PR is included in version 1.97.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

webmaster128 · 2022-01-05T21:24:05Z

integration/angular/simple-message.ts

@@ -81,3 +80,7 @@ if (util.Long !== Long) {
  util.Long = Long as any;
  configure();
 }
+
+function isSet(value: any): boolean {
+  return value !== null && value !== undefined;


Now that this is pulled out into a nicely named helper, you can even use value != null.

Do we need type narrowing? You can do something like

function isSet<X>(value: X | undefined | null): value is X { return value != null; }

webmaster128 · 2022-01-05T21:31:15Z

integration/struct/google/protobuf/struct.ts

+function isObject(value: any): boolean {
+  return typeof value === 'object' && value !== null;
+}


👍

Just in case type narrowing matters at some point, have a look at this implementation I wrote for a different project: https://github.com/cosmos/cosmjs/blob/v0.27.0-rc2/packages/utils/src/typechecks.ts#L1-L12

fix: oneof=union breaks wrapper types #458

607d5cd

boukeversteegh linked an issue Jan 4, 2022 that may be closed by this pull request

google.protobuf.NullValue in oneof #458

Closed

undo local changes

b95e27d

boukeversteegh mentioned this pull request Jan 4, 2022

google.protobuf.NullValue in oneof #458

Closed

boukeversteegh added 7 commits January 4, 2022 22:59

Update struct.ts

0e17a41

prettier

6f52e2c

unwrap json values when json decoding in values integration test

8fc7544

Remove unnecessary unwrapFromJSON

3d231f8

Remove unnecessary unwrapFromJSON

a518a05

Remove unnecessary unwrapFromJSON

f952b0f

Fix test

2649b9a

boukeversteegh marked this pull request as ready for review January 5, 2022 10:23

boukeversteegh requested a review from stephenh January 5, 2022 10:23

stephenh approved these changes Jan 5, 2022

View reviewed changes

boukeversteegh added 2 commits January 5, 2022 17:07

Add missing struct file

e50fbd8

Keep one line conditional assignment for array fields

92f4a3f

stephenh approved these changes Jan 5, 2022

View reviewed changes

boukeversteegh merged commit dd16992 into main Jan 5, 2022

boukeversteegh deleted the fix/458 branch January 5, 2022 16:39

stephenh pushed a commit that referenced this pull request Jan 5, 2022

chore(release): 1.97.1 [skip ci]

f2a3f02

## [1.97.1](v1.97.0...v1.97.1) (2022-01-05) ### Bug Fixes * oneof=union breaks wrapper types [#458](#458) ([#462](#462)) ([dd16992](dd16992))

stephenh added the released label Jan 5, 2022

webmaster128 reviewed Jan 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: oneof=union breaks wrapper types #458 #462

fix: oneof=union breaks wrapper types #458 #462

boukeversteegh commented Jan 4, 2022 •

edited

Loading

stephenh left a comment

stephenh Jan 5, 2022

boukeversteegh Jan 5, 2022 •

edited

Loading

stephenh Jan 5, 2022

boukeversteegh Jan 5, 2022

webmaster128 Jan 5, 2022

webmaster128 Jan 5, 2022

stephenh Jan 6, 2022

webmaster128 Jan 6, 2022

stephenh Jan 6, 2022

stephenh Jan 5, 2022

stephenh Jan 5, 2022

boukeversteegh Jan 5, 2022

stephenh left a comment

boukeversteegh commented Jan 5, 2022

stephenh commented Jan 5, 2022

webmaster128 Jan 5, 2022 •

edited

Loading

webmaster128 Jan 5, 2022

fix: oneof=union breaks wrapper types #458 #462

fix: oneof=union breaks wrapper types #458 #462

Conversation

boukeversteegh commented Jan 4, 2022 • edited Loading

stephenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boukeversteegh Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenh left a comment

Choose a reason for hiding this comment

boukeversteegh commented Jan 5, 2022

stephenh commented Jan 5, 2022

webmaster128 Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boukeversteegh commented Jan 4, 2022 •

edited

Loading

boukeversteegh Jan 5, 2022 •

edited

Loading

webmaster128 Jan 5, 2022 •

edited

Loading