feat: add support for Struct in NestJS

[ZimGil]

This PR is related to #69

After investigation of how Timestamp was solved with wrappers I was able to solve the issue we're seeing with Struct.

Notice that I'm considering that values of Stract are of type Value which is what's actually stated in the Docs

Field name	Type	Description
fields	map<string, Value>	Map of dynamically typed values.

While researching the wrappers solution I noticed that when wrapping google.protobuf.Struct we can wrap and unwrap but google.protobuf.Value can not be wrapped as protobufjs is not reading this value of wrappers. But due to the fact that Struct maps to Value I could use it's wrap and unwrap methods in order to wrap and unwrap Struct itself.

Same goes for ListValue

Had to bump @grpc/proto-loader to support NestJS useage of fromJSON()

[stephenh]

Hey @ZimGil , thanks for the PR! Apologies I just haven't had a time to get to it yet. I think it looks good. Neither NestJS or structs are things I've personally used so I really appreciate contributions to make them work!

I made a few small comments, and it looks like there is a failing test in CI, but overall this looks great, and would be great to get this merged. Thanks!

[stephenh]

Question: Should this true be a false? Just thinking that we are in the nested = false branch of the it, so should we pass along false as well?

[ZimGil]

The only case we actually need for nested to be false is the base object, we should only get here initially and from here it should be always true

[stephenh]

Gotcha, thanks!

[stephenh]

Question: Just curious, but instead of : message.fields.get(key), should this be just : undefined? Like if Value.unwrap can't unwrap the value, is there really a potentially valid-but-not-unwrappable value we need to get?

[ZimGil]

Can't recall why I put it there but there was something - I'll look into what happens if we remove it

[ZimGil]

The reason we have that here is for the case where the result of message.fields.get(key) is not necessarily actually wrapped and this cause the result of Value.unwrap() to return undefined

Returning the actual value instead help us avoid these.
Removing this broke the tests

[stephenh]

Ah okay, happy to keep it then.

Out of curiosity, is it expected that we'll end up with not-actually-wrapped values in should-be-wrapped cases, or was an existing test just being particularly ocd?

[ZimGil]

The second part :)

[stephenh]

Hm, I think I realized what was happening ...

In the findOneHero test, the server was only invoking Struct.wrap(...) once, but never invoking ListValue.wrap or Value.wrap on any of the inner contents.

Even after adding wrapper entries for them:

wrappers[".google.protobuf.Struct"] = { fromObject: Struct.wrap, toObject: Struct.unwrap } as any;
wrappers[".google.protobuf.Value"] = { fromObject: Value.wrap, toObject: Value.unwrap } as any;
wrappers[".google.protobuf.ListValue"] = { fromObject: ListValue.wrap, toObject: ListValue.unwrap } as any;

Value.wrap / ListValue.wrap are never invoked.

So I think that's why, inside the client .unwraps, we saw things that were "hey, wait a sec, not wrapped".

I was able to fix this by making Struct.wrap deeply wrap, and Struct.unwrap deeply unwrap...which AFAICT fixed the issue.

But it broke ts-proto's non-NestJS Struct.encode / Struct.decode methods, which specifically assumed that wrap/unwrap were shallow.

So now I need to go back and teach the encode/decode methods about the deepness of wrap/unwrap...or maybe for now we just have two versions of wrap/unwrap; one that is deep for NestJS, and one that is shallow for not-NestJS...

I'm going to go ahead and push up the deep-wrap/unwrap changes up to this PR, which will break some non-NestJS tests, but AFAICT that's the right path forward for NestJS's Struct support.

Does that seem right?

[ZimGil]

I agree that deeply wrapping and unwrapping is the way to go

Don't want to break the tests.

[stephenh]

Well, I cheated and just copy/pasted both deep & shallow versions of the wrappers, but it seems to be working for now. :-)

I really haven't personally used the Struct/List/ListValue types, and helping on this PR was the closest I've been to the code... I feel like something is still a little off...like how in the regular/non-NestJs output, Struct.encode takes a Struct but it should take an any...

Anyway, I'm going to punt on that for now. Tests are passing so I'll merge and 🤞 seems like it should work.

Thanks for the PR!

[stephenh]

Suggestion: Ah okay, we add the || options.nestJs b/c we need it to generate any wrap/unwrap methods...

I wonder if we could make this a little more nuanced, by doing something like:

const wrap = this.generateWrap(...);
const unwrap = this.generateUnwrap(...);
if (options.outputEncodeMethods || ... || wrap.length > 0 || unwrap.length > 0)

Pretty minor, but I think it'd avoid outputting some of the empty export const Hero = {} lines that are unnecessary.

[ZimGil]

I'll look into that

[ZimGil]

We can't seem to do that as props of generateWrap() and generateUnwrap() are in the scope of the callback of visit inside that if

Any other suggestion to clean up the output?

[stephenh]

Ah shoot, you're right. I had to poke around a little bit, but I think pushed up a few more conditionals that cleaned up some of the unnecessary output.

[ZimGil]

@stephenh I've pushed a fix the the tests - replied to the comments after playing around.

[stephenh]

Are the hasOwnProperty checks necessary here? Just curious if they are actually fixing something or just being extra.

[ZimGil]

Without it, it's looking at the prototype which actually has all other props set to null for some reason

[ZimGil]

At least in some cases

[stephenh]

🎉 This PR is included in version 1.139.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀