error when trying to encode null optional properties

[yoavmil]

If I set one optional property to null, and later try to encode it, the code doesn't check for null, it only checks for undefined. Therefor deeper inside it craseshes. see snippet:

export const Vec3 = {
  encode(message: Vec3, writer: Writer = Writer.create()): Writer {
    writer.uint32(13).float(message.x);
    ...
    if (message.direction !== undefined && message.direction !== undefined) { <---

I'm guessing the second conidtion should be

&& message.direction !== null

or you can just

if (!!message.direction) { // this covers both options

Cheers!

[stephenh]

Hm, are you using the latest version of ts-proto? In our test output, I see only a single message.thing !== undeifned) check, and not too:

https://github.com/stephenh/ts-proto/blob/main/integration/simple/simple.ts#L328

Granted, per your core ask, no, we're not checking if message.thing !== null, but I think that's because in theory null is not allowed by the type, i.e. the type is defined as OtherMessage | undefined and not as null:

https://github.com/stephenh/ts-proto/blob/main/integration/simple/simple.ts#L69

If you're doing something like:

const message = JSON.parse(someString) as Message

Then, right, we can't trust the as to match the type. Doing Message.fromPartial(JSON.parse(someString)) would be a safer way of doing this, and it does actually check null:

    message.thing =
      object.thing !== undefined && object.thing !== null ? ImportedThing.fromPartial(object.thing) : undefined;

Granted, it seems pretty cheap/innocent to have encode go ahead and check null as well, but, dunno, I think it could be a slippery slope where encode "can't trust" / has to double check our own types/message.

Is going through fromPartial okay?

[yoavmil]

Thanks for the fast reply. I have updated now to the latest version. Actually, I see that at fromPartial and at fromJSON you do check also for null, where as at encode you check only for defined. I recomend to do

if (message.thing == null) {} // this covers both options.

see here

BTW, my use case was that I have set a oneof field to be null, rather than deleting it:

message.thing = null; // not good, 
// VS
delete message.thing; // good.