fix: oneof=union breaks wrapper types #458

[boukeversteegh]

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]

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?

[boukeversteegh]

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.

[stephenh]

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.

[boukeversteegh]

@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.

[webmaster128]

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

[webmaster128]

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.

[stephenh]

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". :-)

[webmaster128]

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]

Ah great!

[stephenh]

Nice

[stephenh]

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?

[boukeversteegh]

Great catch! Wow :-) Just pushed it.

[stephenh]

Looks great, thanks @boukeversteegh !

[boukeversteegh]

Looks great, thanks @boukeversteegh !

Thank you for your review and coordination!

[stephenh]

🎉 This PR is included in version 1.97.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[webmaster128]

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]

👍

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