Next.js does not serialize values that are undefined

[m8vago]

First of all thanks for your hard work, ts-proto is really awesome!

Recently we'd updated to the latest version and noticed, the grpc response has undefined values as a default for fields which aren't sent in the wire.

I think the change was made in this commit: https://github.com/stephenh/ts-proto/commit/70832d33bae82ecb3c5f87845d14e992a13437e4#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3d

However this broke our Next.js frontend, because next forbids to send an object in getServerSideProps() as a response that has any undefined value.

As far as i see, in the current code-base there is no option to disable the default undefined value for optional fields.

I am aware of Type.toJson() calls are dealing with this issue, but those are returning any types, which would make our code less type-safe.

Would it be possible to have a flag that restores the old behavior?

[stephenh]

Huh, that's interesting. I guess the next js restriction makes sense, insofar as it's expected a valid JSON value (I assume) and undefined as a value is not valid JSON.

As you pointed out, this is b/c you're putting "not meant to be JSON" objects on the wire, and previously it had just happened to work out. I'd be a little worried that this is a slippery slope, i.e. that besides the undefined issue that eventually you'll run into other mismatches where instead of just "proto messages that are implicitly JSON.stringified", you might want other "better/actual JSON" output that comes from the canonical protobuf JS encoding, which again as you mention is what the .toJsons do...

For example, how do you handle Dates? By default, ts-proto will use js Dates in messages, so the message type will be { startDate: Date } but the "actual JSON type" is { startDate: string }. Does your getServerSideProps in this case have to return startDate: string i.e. the literal JSON-on-the-wire type?

Fwiw, the reason we introduced this change is that setting all keys directly on instantiation is supposed to help (handwaving) with v8 optimizations, so I'd at least want to keep it as the default behavior.

Seems like there are a few options:

1. As you said, we could add an option to not set keys as undefined by default
2. We could potentially produce "actually on the wire" JSON types for each message, i.e. FooMessage.toJson would return a new FooMessageJson type that was strongly typed and would "do the right thing" i.e. startDate: Date would come out as startDate: string or what not (this would probably be a lot of work, but might be helpful to others who are similarly want to use FooMessage.toJson but also want static typing on alignment between the toJson output type and whatever actual-API types they're attempting to match.
3. ...huh, I found https://github.com/vercel/next.js/discussions/11209#discussioncomment-1881525 which insinuates that { value: undefined } actually is allowed by JSON.stringify ... so, now I'm doubting whether next.js is really "doing the right thing" here, b/c they seem to be adding additional restrictions on top of the native JSON.stringify. Can you just do that patch/dev-only handler in the linked discussion? (In particular the fact that { value: undefined } works just fine in production, but it's only a dev mode warning seems like a "trying to be too helpful" sort of thing imo.)

[m8vago]

Fwiw, the reason we introduced this change is that setting all keys directly on instantiation is supposed to help (handwaving) with v8 optimizations, so I'd at least want to keep it as the default behavior.

It's refreshing to see that you are caring about performance, definitely keep it as a default!

For example, how do you handle Dates? By default, ts-proto will use js Dates in messages, so the message type will be { startDate: Date } but the "actual JSON type" is { startDate: string }. Does your getServerSideProps in this case have to return startDate: string i.e. the literal JSON-on-the-wire type?

Right now we have two separated models. One defined in the proto file, thus processed and defined in typescript by ts-proto, and another one that acts as a DTO in next's frontend-backend communication, which we define manually. In proto the dates' type is google.protobuf.Timestamp, in the DTO it's a UTC string. We're also using service classes to handle the rpc calls and map the proto model into a DTO.

Example: proto

message FooProto {
  string id = 1;
  string name = 2;
  google.protobuf.Timestamp date = 3;
}

DTO

type FooDTO = {
  string id;
  string name;
  string date;
}

service function

async bar(id: string): Promise<FooDTO> {
  const res = await protomisify<string, FooProto>(this.client, this.client.bar)(id)

   return {
    ...res,
      date: timestampToUTC(res.date),
  }
}

This usage of typescript will throw a compile-time error, when the proto file has a new date field which it can't map into a string. We could use --ts_proto_opt=useDate=string, but some DTOs have additional fields, string literal unions that are lower-case and mapped from an enum, or completely a different structure, so mapping is necessary.

Seems like there are a few options:

The ultimate solution would be the second option. Some cases we would still have to map it after the toJson() call, but that's not a big deal. Of course for our use-case option 1 works as well.

Option 3 could work, but smells really-really badly. I'd like to avoid it if we can.

Right now we're just using the 1.96.0 version of ts-proto and it works fine, but in the future it would be nice to update, of course.

[stephenh]

Option 3 could work, but smells really-really badly. I'd like to avoid it if we can.

Yeah. Just kind of thinking, JSON.stringify already "does extra stuff". E.g. it calls .toJson on instances if the key exists. And it does actually dropped undefined values... So I guess I'm missing why next.js thinks it's important to, at dev-time, warn against just this one case.

...or do they not actually use JSON.stringify at production time? If so, then it'd make more sense that they need extra constraints that JSON.stringify itself does not have.

So, dunno, currently I'd want to understand next.js's reasoning here a little bit more, before agreeing that option 3 is terrible. :-)

For option 2, I kinda wonder if we could just have a sufficiently fancy mapped type, i.e. given a FooMessage the return type of FooMessage.toJson could be JsonOf<FooMessage>. And it would map each field type appropriately. That seems like it'd be neat, instead of creating yet-another-interface that is ~90% the same as FooMessage.

I filed #494 as a follow up; would be great if you wanted to poke at that. :-)

Since you are already writing mapping code, you could also do:

  const res = await protomisify<string, FooProto>(this.client, this.client.bar)(id)
   return {
    ...deleteUndefined(res),
      date: timestampToUTC(res.date),
  }

Which loops through res's fields and deletes any key that is undefined.

Or maybe even better work that into your protomisify infra.

[m8vago]

...or do they not actually use JSON.stringify at production time?

My bet is on this one.

I filed #494 as a follow up; would be great if you wanted to poke at that. :-)

I'll look into it.

Or maybe even better work that into your protomisify infra.

That's sounds like a fair workaround. Thanks!