tatethurston/TwirpScript

Optional message fields are `type | undefined` instead of `type | null`

take0ut opened this issue · 3 comments

First off, really enjoying this project. Love how strong the documentation and guide are.

I'm building an RPC server that is used for quickly writing/retrieving database records using Prisma as an ORM. Both Prisma and Twirpscript provide pre-generated types, but there is an incompatibility between them that is difficult to overcome.

Assume a database table like:

id: uuid
firstName string non null
lastName: string non null
class: string

Where there are two non null columns and one nullable column. Prisma generates types for this schema like:

id: uuid
firstName: string
lastName: string
class: string | null

If we were to write a protobuf for this schema, we'd want something like

string id = 1;
string firstName = 2;
string lastName = 3;
optional string class = 4;

When twirpscript generates types from this protobuf, it assumes (correctly, according to how protobuf compiling works), that class: string | undefined.

This creates an issue with typescript that is quite laborious to overcome. Any nullable column needs to be set to undefined if null. If using Prisma's native include helper, which automatically retrieves related tables, one needs to recursively perform these same checks on every included record, which would be hugely wasteful computationally.

There are workarounds, the most obvious being a @ts-expect-error command before the handler implementation. This is not desirable for obvious reasons, but since the code author knows that the Prisma schema and Protobuf schemas are the same, there is likely not that many issues that cannot be solved by strong test coverage.

Is there a better way to handle nullable values in Twirpscript or Protobuf as a whole? If not, is there a stronger pattern for dealing with database retrieval in protobuf that I can adopt? I am sure there are other ORMs without generated types I can use, but I have been enjoying Prisma, outside of this nuisance.

That being said, once "ignored", there are no issues with the actual handlers. They create/retrieve rows correctly, and as expected replace null columns with field defaults (in my case, an empty string).

TLDR: Twirpscript rightly interprets optional message fields as type | undefined. Is there a convenient way to adjust the generated types to be type | null to more easily support returning database rows?

Thanks @oweingrod I'm glad you're enjoying TwirpScript so far.

Yes, that friction is annoying, and stems from having two bottom types in JavaScript. I personally subscribe to Douglas Crockford's stance on avoiding null usage within applications, and always use undefined as the bottom type. Even with that approach, null can still be unavoidable because of 3rd party libraries.

My inclination is to make type emission for optionals class?: string | null | undefined. Relay takes the same approach for TypeScript type emission. The only disadvantage I can think of with this approach is that in application code, field === undefined will no longer work as a check, users will need to if (!field) or use loose equality checks if (field == undefined).

@tatethurston I think that this would be a reasonable change. Perhaps there is a way to expose an option to handle this in twirp.json so that users who have already built handlers relying on field being undefined do not experience breaking changes?

Yeah I considered that, but I'm thinking about approaching it from the opposite direction. In general I'm trying to stay as zero config / works out of the box as possible. I think being unimpacted or wanting the widened type is going to be the more common case than needing the narrower type.

I'm thinking I'll expose a config option to generate the narrower type (current state) if / when someone requests it.