-
Notifications
You must be signed in to change notification settings - Fork 339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added useNullAsOptional option #1017
Conversation
- with this flag, undefined types will be replaced with null. - fields with optional label in proto files, will implicitly accept undefined too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @TheMichio , thanks for the PR! I left a comment asking about whether we should be more purposefully writing null
into objects in decode/fromJSON.
export interface User { | ||
id: number; | ||
username: string; | ||
profile?: ProfileInfo | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ProfileInfo | null
type actually accurate?
Afaict the .decode
and .fromJSON
methods are not ever setting profile
to null
... so if it's never null, why are we adding it to the type?
Fwiw I was expecting this to be like profile: ProfileInfo | null
and then we'd like specifically insert null
as the "empty" value at runtime, in the decode/fromJSON methods.
Or is the point that we can accept a User.profile: null
object that was created from an ORM, with | null
in the type, and just not write it on the wire / still treat it as undefined?
Will the ORM be fine with accepting undefined
if we decode something off the wire it and give it profile: undefined
instead of profile: null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same doubt and confusion too 🥲
my goal is to give the user a chance to have better alignment with tools like ORMs, let me give an example with drizzle
ORM:
let's say we have two tables user
and profile
, we can have two scenarios in which we might include profile relation in our select query or not, the resuling type for the query would be something like:
- with profile relation included:
let user = await this.db.query.users.findFirst({
where: eq(users.id, id),
with: {
profile: true,
},
});
the type for user would be:
let user: {
id: number;
email: string;
username: string;
password: string;
profile: {
id: number;
name: string | null;
bio: string | null;
lastName: string | null;
} | null;
} | undefined
- without profile relation included:
let user = await this.db.query.users.findFirst({
where: eq(users.id, id),
});
the type for user would be:
let user: {
id: number;
email: string;
username: string;
password: string;
} | undefined
so the explicit null
type on the profile
is needed for the situation where we don't have any profile relation for user. and the optional part on profile?
is needed for the situation where we are NOT querying the relation ( we don't care about it ) and it's not even there.
the ideal generated interface by ts-proto
IMHO would be:
export interface Profile {
id: number;
name?: string | null;
lastName?: string | null;
bio?: string | null;
}
export interface User {
id: number;
email: string;
username: string;
profile?: Profile | null;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the exactOptionalPropertyTypes
on tsc type checking we can make the type ProfileInfo | null | undefined
, to make the typechecker happy as well.
please let me know what you think, thanks thanks 🙏
this is the best I could come up with to handle the undefined
, null
, optional
situation a little bit 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @TheMichio ; I guess my confusion with the two findFirst
examples is that I'm not sure it's a good idea to try and have a single protobuf message type be used for both of those queries simulatenously.
I.e. it sounds like you're trying to make a User
"domain model type" / shared type that is going to be used at multiple points in your application, and probably returned from multiple wire calls? Like maybe 1 RPC call has GetJustUser
that does the first findMany
but another RPC call has GetUserWithProfile
.
As you pointed out, these have fundamentally different types coming from the ORM, but I guess shouldn't they also have fundamentally different types when being put on the wire? I.e. it seems weird for the caller of GetJustUser
to have a message type that says "you might have a profile" when they would never have a profile.
Dunno, honestly I'm going back/forth on accepting the PR; it's really great you got it to work, and all of the code technically looks great, but it adds some implementation complexity that I'm not 100% sure is best for the long-term.
...
Hm, I guess we have #869 open for awhile, and also it does look like things like profile: null` are the default, which I'd missed in the first review--okay, seems good enough for me then. Thank you!
src/utils.ts
Outdated
return options.useNullAsOptional ? ` ${prefix} ${typeName} === null` : ""; | ||
} | ||
|
||
export const withOrMaybeCheckIsNotNull = (options: Pick<Options, "useNullAsOptional">, typeName: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really minor, but can you make these all function
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh dang, forgot about that, sure sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed them, done! 🙏
🎉 This PR is included in version 1.171.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changes:
this feature is needed when we wanna have better type alignment with ORMs ( drizzle, typeorm, prisma ) or other services such as Firestore, since they mostly ignore
undefined
in their types.Note:
@stephenh as you wanted I made simple, small changes, this works well for my
nestjs
project, I'm not very familiar with other implementations and frameworks, please check it out and let me know what you think. if any changes are required please let me know, I will work them out.thanks for this amazing library 🙏
closes #869