-
Notifications
You must be signed in to change notification settings - Fork 358
feat: added useNullAsOptional option #1017
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
Merged
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
useNullAsOptional=true |
Binary file not shown.
23 changes: 23 additions & 0 deletions
23
integration/use-null-as-optional/use-null-as-optional.proto
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
syntax = "proto3"; | ||
|
||
package useNullAsOptional; | ||
|
||
message ProfileInfo { | ||
int32 id = 1; | ||
string bio = 2; | ||
string phone = 3; | ||
} | ||
|
||
message User { | ||
int32 id = 1; | ||
string username = 2; | ||
optional ProfileInfo profile = 3; | ||
} | ||
|
||
message UserById { | ||
int32 id = 1; | ||
} | ||
|
||
service HeroService { | ||
rpc FindOneHero (UserById) returns (User) {} | ||
} |
297 changes: 297 additions & 0 deletions
297
integration/use-null-as-optional/use-null-as-optional.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,297 @@ | ||
/* eslint-disable */ | ||
import * as _m0 from "protobufjs/minimal"; | ||
|
||
export const protobufPackage = "useNullAsOptional"; | ||
|
||
export interface ProfileInfo { | ||
id: number; | ||
bio: string; | ||
phone: string; | ||
} | ||
|
||
export interface User { | ||
id: number; | ||
username: string; | ||
profile?: ProfileInfo | null; | ||
} | ||
|
||
export interface UserById { | ||
id: number; | ||
} | ||
|
||
function createBaseProfileInfo(): ProfileInfo { | ||
return { id: 0, bio: "", phone: "" }; | ||
} | ||
|
||
export const ProfileInfo = { | ||
encode(message: ProfileInfo, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer { | ||
if (message.id !== 0) { | ||
writer.uint32(8).int32(message.id); | ||
} | ||
if (message.bio !== "") { | ||
writer.uint32(18).string(message.bio); | ||
} | ||
if (message.phone !== "") { | ||
writer.uint32(26).string(message.phone); | ||
} | ||
return writer; | ||
}, | ||
|
||
decode(input: _m0.Reader | Uint8Array, length?: number): ProfileInfo { | ||
const reader = input instanceof _m0.Reader ? input : _m0.Reader.create(input); | ||
let end = length === undefined ? reader.len : reader.pos + length; | ||
const message = createBaseProfileInfo(); | ||
while (reader.pos < end) { | ||
const tag = reader.uint32(); | ||
switch (tag >>> 3) { | ||
case 1: | ||
if (tag !== 8) { | ||
break; | ||
} | ||
|
||
message.id = reader.int32(); | ||
continue; | ||
case 2: | ||
if (tag !== 18) { | ||
break; | ||
} | ||
|
||
message.bio = reader.string(); | ||
continue; | ||
case 3: | ||
if (tag !== 26) { | ||
break; | ||
} | ||
|
||
message.phone = reader.string(); | ||
continue; | ||
} | ||
if ((tag & 7) === 4 || tag === 0) { | ||
break; | ||
} | ||
reader.skipType(tag & 7); | ||
} | ||
return message; | ||
}, | ||
|
||
fromJSON(object: any): ProfileInfo { | ||
return { | ||
id: isSet(object.id) ? globalThis.Number(object.id) : 0, | ||
bio: isSet(object.bio) ? globalThis.String(object.bio) : "", | ||
phone: isSet(object.phone) ? globalThis.String(object.phone) : "", | ||
}; | ||
}, | ||
|
||
toJSON(message: ProfileInfo): unknown { | ||
const obj: any = {}; | ||
if (message.id !== 0) { | ||
obj.id = Math.round(message.id); | ||
} | ||
if (message.bio !== "") { | ||
obj.bio = message.bio; | ||
} | ||
if (message.phone !== "") { | ||
obj.phone = message.phone; | ||
} | ||
return obj; | ||
}, | ||
|
||
create<I extends Exact<DeepPartial<ProfileInfo>, I>>(base?: I): ProfileInfo { | ||
return ProfileInfo.fromPartial(base ?? ({} as any)); | ||
}, | ||
fromPartial<I extends Exact<DeepPartial<ProfileInfo>, I>>(object: I): ProfileInfo { | ||
const message = createBaseProfileInfo(); | ||
message.id = object.id ?? 0; | ||
message.bio = object.bio ?? ""; | ||
message.phone = object.phone ?? ""; | ||
return message; | ||
}, | ||
}; | ||
|
||
function createBaseUser(): User { | ||
return { id: 0, username: "", profile: null }; | ||
} | ||
|
||
export const User = { | ||
encode(message: User, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer { | ||
if (message.id !== 0) { | ||
writer.uint32(8).int32(message.id); | ||
} | ||
if (message.username !== "") { | ||
writer.uint32(18).string(message.username); | ||
} | ||
if (message.profile !== undefined && message.profile !== null) { | ||
ProfileInfo.encode(message.profile, writer.uint32(26).fork()).ldelim(); | ||
} | ||
return writer; | ||
}, | ||
|
||
decode(input: _m0.Reader | Uint8Array, length?: number): User { | ||
const reader = input instanceof _m0.Reader ? input : _m0.Reader.create(input); | ||
let end = length === undefined ? reader.len : reader.pos + length; | ||
const message = createBaseUser(); | ||
while (reader.pos < end) { | ||
const tag = reader.uint32(); | ||
switch (tag >>> 3) { | ||
case 1: | ||
if (tag !== 8) { | ||
break; | ||
} | ||
|
||
message.id = reader.int32(); | ||
continue; | ||
case 2: | ||
if (tag !== 18) { | ||
break; | ||
} | ||
|
||
message.username = reader.string(); | ||
continue; | ||
case 3: | ||
if (tag !== 26) { | ||
break; | ||
} | ||
|
||
message.profile = ProfileInfo.decode(reader, reader.uint32()); | ||
continue; | ||
} | ||
if ((tag & 7) === 4 || tag === 0) { | ||
break; | ||
} | ||
reader.skipType(tag & 7); | ||
} | ||
return message; | ||
}, | ||
|
||
fromJSON(object: any): User { | ||
return { | ||
id: isSet(object.id) ? globalThis.Number(object.id) : 0, | ||
username: isSet(object.username) ? globalThis.String(object.username) : "", | ||
profile: isSet(object.profile) ? ProfileInfo.fromJSON(object.profile) : null, | ||
}; | ||
}, | ||
|
||
toJSON(message: User): unknown { | ||
const obj: any = {}; | ||
if (message.id !== 0) { | ||
obj.id = Math.round(message.id); | ||
} | ||
if (message.username !== "") { | ||
obj.username = message.username; | ||
} | ||
if (message.profile !== undefined && message.profile !== null) { | ||
obj.profile = ProfileInfo.toJSON(message.profile); | ||
} | ||
return obj; | ||
}, | ||
|
||
create<I extends Exact<DeepPartial<User>, I>>(base?: I): User { | ||
return User.fromPartial(base ?? ({} as any)); | ||
}, | ||
fromPartial<I extends Exact<DeepPartial<User>, I>>(object: I): User { | ||
const message = createBaseUser(); | ||
message.id = object.id ?? 0; | ||
message.username = object.username ?? ""; | ||
message.profile = (object.profile !== undefined && object.profile !== null) | ||
? ProfileInfo.fromPartial(object.profile) | ||
: undefined; | ||
return message; | ||
}, | ||
}; | ||
|
||
function createBaseUserById(): UserById { | ||
return { id: 0 }; | ||
} | ||
|
||
export const UserById = { | ||
encode(message: UserById, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer { | ||
if (message.id !== 0) { | ||
writer.uint32(8).int32(message.id); | ||
} | ||
return writer; | ||
}, | ||
|
||
decode(input: _m0.Reader | Uint8Array, length?: number): UserById { | ||
const reader = input instanceof _m0.Reader ? input : _m0.Reader.create(input); | ||
let end = length === undefined ? reader.len : reader.pos + length; | ||
const message = createBaseUserById(); | ||
while (reader.pos < end) { | ||
const tag = reader.uint32(); | ||
switch (tag >>> 3) { | ||
case 1: | ||
if (tag !== 8) { | ||
break; | ||
} | ||
|
||
message.id = reader.int32(); | ||
continue; | ||
} | ||
if ((tag & 7) === 4 || tag === 0) { | ||
break; | ||
} | ||
reader.skipType(tag & 7); | ||
} | ||
return message; | ||
}, | ||
|
||
fromJSON(object: any): UserById { | ||
return { id: isSet(object.id) ? globalThis.Number(object.id) : 0 }; | ||
}, | ||
|
||
toJSON(message: UserById): unknown { | ||
const obj: any = {}; | ||
if (message.id !== 0) { | ||
obj.id = Math.round(message.id); | ||
} | ||
return obj; | ||
}, | ||
|
||
create<I extends Exact<DeepPartial<UserById>, I>>(base?: I): UserById { | ||
return UserById.fromPartial(base ?? ({} as any)); | ||
}, | ||
fromPartial<I extends Exact<DeepPartial<UserById>, I>>(object: I): UserById { | ||
const message = createBaseUserById(); | ||
message.id = object.id ?? 0; | ||
return message; | ||
}, | ||
}; | ||
|
||
export interface HeroService { | ||
FindOneHero(request: UserById): Promise<User>; | ||
} | ||
|
||
export const HeroServiceServiceName = "useNullAsOptional.HeroService"; | ||
export class HeroServiceClientImpl implements HeroService { | ||
private readonly rpc: Rpc; | ||
private readonly service: string; | ||
constructor(rpc: Rpc, opts?: { service?: string }) { | ||
this.service = opts?.service || HeroServiceServiceName; | ||
this.rpc = rpc; | ||
this.FindOneHero = this.FindOneHero.bind(this); | ||
} | ||
FindOneHero(request: UserById): Promise<User> { | ||
const data = UserById.encode(request).finish(); | ||
const promise = this.rpc.request(this.service, "FindOneHero", data); | ||
return promise.then((data) => User.decode(_m0.Reader.create(data))); | ||
} | ||
} | ||
|
||
interface Rpc { | ||
request(service: string, method: string, data: Uint8Array): Promise<Uint8Array>; | ||
} | ||
|
||
type Builtin = Date | Function | Uint8Array | string | number | boolean | undefined; | ||
|
||
export type DeepPartial<T> = T extends Builtin ? T | ||
: T extends globalThis.Array<infer U> ? globalThis.Array<DeepPartial<U>> | ||
: T extends ReadonlyArray<infer U> ? ReadonlyArray<DeepPartial<U>> | ||
: T extends {} ? { [K in keyof T]?: DeepPartial<T[K]> } | ||
: Partial<T>; | ||
|
||
type KeysOfUnion<T> = T extends T ? keyof T : never; | ||
export type Exact<P, I extends P> = P extends Builtin ? P | ||
: P & { [K in keyof P]: Exact<P[K], I[K]> } & { [K in Exclude<keyof I, KeysOfUnion<P>>]: never }; | ||
|
||
function isSet(value: any): boolean { | ||
return value !== null && value !== undefined; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 settingprofile
tonull
... 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 insertnull
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 itprofile: undefined
instead ofprofile: 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
andprofile
, 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:the type for user would be:
the type for user would be:
so the explicit
null
type on theprofile
is needed for the situation where we don't have any profile relation for user. and the optional part onprofile?
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: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 typeProfileInfo | 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 hasGetJustUser
that does the firstfindMany
but another RPC call hasGetUserWithProfile
.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!