fix(app-server): fix TS annotations for optional fields on requests#10412
fix(app-server): fix TS annotations for optional fields on requests#10412
Conversation
owenlin0
force-pushed
the
owen/fix_ts_request_annotations
branch
from
34e3480 to
1acf0e3
Compare
owenlin0
force-pushed
the
owen/fix_ts_request_annotations
branch
from
1acf0e3 to
8182773
Compare
There was a problem hiding this comment.
Does this change ALL nullable fields to be nullable or optional or only actually optional ones?
I can't find the PR but I'm almost positive that, last year, used to do this and explicitly moved away from it because of the ambiguitiy between null and undefined in many cases. Let's make sure we square this with that decision before moving forward
Maybe it was this one?
@bolinfest @etraut-openai do you remember when/where we did that?
|
@gpeal Yes, I was the one who landed a change to explicitly move away from However there is a distinction between structs sent by the client vs. structs sent by the server, and this only reintroduces |
owenlin0
force-pushed
the
owen/fix_ts_request_annotations
branch
from
7b28dbe to
22f3837
Compare
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Thanks for the thorough AGENTS.md update!
3 participants
This updates our generated TypeScript types to be more correct with how the server actually behaves, specifically for JSON-RPC requests.
Before this PR, we'd generate
field: T | null. After this PR, we will havefield?: T | null. The latter matches how the server actually works, in that if an optional field is omitted, the server will treat it as null. This also makes it less annoying in theory for clients to upgrade to newer versions of Codex, since adding a new optional field to a JSON-RPC request should not require a client change.NOTE: This only applies to JSON-RPC requests. All other payloads (i.e. responses, notifications) will return
field: T | nullas usual.