chore: switch to guts for typescript types generation #15801

Emyrk · 2024-12-09T18:50:49Z

What this does

Hopefully nothing. This moves to using guts for generating our typescript types. Our typescript generator has been made a package, with opinions removed.

Some changes

Explicit omitempty because undefined and null are different. Now it must be explicit. Added omitempty to some types to be consistent with how we were using them, used null for others.

Future work

Future PRs should fix when we use omitempty and pointers to distinguish null or ? in the typescript. The current code types seem arbitrary and do not distinguish between the two. This new type-gen is more accurate.

In golang, omitempty will marshal the json as {}, while nil will marshal as {"field":null}. We should be explicit when to do omitempty and when to leave it as <type> | null. Sometimes the field existing, but being null is preferred. We should migrate to null when we always want the field to exist.

I will not do this in this PR, but will make follow ups to fix certain areas over time.

Bug

Fixed if you remove a user from a group, it deletes the display name.

f0ssel

Go part is mostly what I reviewed, looks pretty simple since everything is in guts now, lgtm

f0ssel · 2024-12-11T18:08:29Z

scripts/apitypings/README.md

-# Future Ideas
-
- Use a yaml config for overriding certain types
+Uses it's own `go.mod` to exclude goja deps from the main go.mod.


Why does this matter in practice?

My understanding of go.mod isn't complete, but I did not want anyone importing coder/coder to see goja as an indirect dependency.

Doing it this way prevents goja from ever appearing in any go.mod/go.sum related to coder/coder. Given it's a JS runtime in Go, feels like a jank dep to include.

scripts/apitypings/main.go

f0ssel · 2024-12-11T18:12:54Z

site/src/api/api.ts

+			display_name: null,
+			avatar_url: null,
+			quota_allowance: null,
 		});
 	};

 	removeMember = async (groupId: string, userId: string) => {
 		return this.patchGroup(groupId, {
 			name: "",
-			display_name: "",
 			add_users: [],
 			remove_users: [userId],
+			display_name: null,
+			avatar_url: null,
+			quota_allowance: null,


Should these use omitempty instead of setting null?

I talked to @Parkreiner about this, and we have to do some overhauling of our types when it comes to omitempty vs null.

For requests, maybe omitempty is easier to use, but null forces the user to acknowledge the fields exist. This actually raised a group member bug.

This is very similar to what we did in our Golang linter for structs: #8403

For most objects returned by Golang, omitempty is incorrect, as we usually have a *string or some pointer to something. Go json marshals this as null, and the previous typescript code would treat the type as string | undefined, but at runtime the type is null. So it was just incorrect, however the truthy value is consistent.

tl;dr: We've been using undefined and null incorrectly in our typescript types, and it's too much to fix in this PR. I just had to make some changes to make the typescript compiler happy, even though nothing has functionally changed.

Yeah, I think the ?: syntax has better ergonomics in React component props, but in all other cases, having the explicit null case is better for making sure we don't have invalid/malformed requests

I'd rather we overhaul things so that we have to deal with explicit null cases for anything involving an API call

Parkreiner

I think there's a little bit of cleanup to do, but I think that this is perfectly fine. A lot of the comments I'm leaving are more for me to make sure I don't miss anything when I try to go in and fix things up later on

Parkreiner · 2024-12-11T21:18:54Z

scripts/apitypings/testdata/generics/generics.ts

-	readonly dynamic: Fi
8000
elds<boolean, A, string, S>;
-	readonly comparable: boolean;
-}
+export type Custom = string | boolean | number | number | string[] | (number | null);


I don't think this is a huge deal, but the TypeScript output here looks a little bit jank. I'm guessing the (number | null) is from a nullable int in Go, but the type number | (number | null) collapses down to just number | null

👍 I can definitely unest these

Parkreiner · 2024-12-11T21:21:06Z

scripts/apitypings/testdata/genericslice/genericslice.ts

-	readonly Slice: Readonly<Array<R>>;
-	readonly TwoD: Readonly<Array<Readonly<Array<R>>>>;
+    readonly Slice: readonly R[];
+    readonly TwoD: readonly R[][];


Can't remember if we talked about this being out of scope for this PR, but just in case, this 2D array isn't deeply readonly. It's a readonly array of mutable R arrays. The fully safe type would have to be readonly (readonly R[])[] (as ugly/clunky as it is)

Yea, I need to recursively apply it 😢

Parkreiner · 2024-12-11T21:23:50Z

site/e2e/api.ts

@@ -150,7 +152,8 @@ export async function verifyConfigFlagArray(
 	);

 	// Verify array of options with simple dots
-	for (const item of opt.value) {
+	// biome-ignore lint/suspicious/noExplicitAny: opt.value is any
+	for (const item of opt.value as any) {


Kind of wish we could've at least written the expression as const item of opt.value as any[], to get some degree of LSP feedback. I don't think it's a huge deal, since this is a test file, but yeah, we'll definitely need to clean up our types later on

Parkreiner · 2024-12-11T21:25:31Z

site/e2e/api.ts

@@ -166,7 +169,8 @@ export async function verifyConfigFlagEntries(
 	);

 	// Verify array of options with green marks.
-	Object.entries(opt.value)
+	// biome-ignore lint/suspicious/noExplicitAny: opt.value is any
+	Object.entries(opt.value as any)


This whole expression is a bit janky. I'll see if I can't clean up some if it during the two work days we have during Christmas break

Parkreiner · 2024-12-11T21:28:42Z

site/src/api/api.ts

+			display_name: null,
+			avatar_url: null,
+			quota_allowance: null,
 		});
 	};

 	removeMember = async (groupId: string, userId: string) => {
 		return this.patchGroup(groupId, {
 			name: "",
-			display_name: "",
 			add_users: [],
 			remove_users: [userId],
+			display_name: null,
+			avatar_url: null,
+			quota_allowance: null,


Yeah, I think the ?: syntax has better ergonomics in React component props, but in all other cases, having the explicit null case is better for making sure we don't have invalid/malformed requests

I'd rather we overhaul things so that we have to deal with explicit null cases for anything involving an API call

Parkreiner · 2024-12-11T21:41:05Z

site/src/pages/HealthPage/DERPPage.tsx

@@ -85,7 +85,11 @@ export const DERPPage: FC = () => {
 				<section>
 					<SectionLabel>Regions</SectionLabel>
 					<div css={{ display: "flex", flexWrap: "wrap", gap: 12 }}>
-						{Object.values(regions)
+						{Object.values(regions!)


Not a huge problem, but this ! isn't doing anything. At least from the type level, regions itself can't be nullish

github-actions bot assigned Emyrk Dec 9, 2024

Emyrk added 6 commits December 9, 2024 12:52

chore: add typescript type overrides

d87ff4b

chore: add enum list generation

f641ab6

update to account for json tagged embeds

16ac059

chore: first generate of the codersdk package

f38bb71

chore: switch gots -> guts (package rename)

fe9c7d1

chore: switch to guts for typescript types generation

6305d44

Emyrk force-pushed the stevenmasley/types_gen branch from ae94a07 to 6305d44 Compare December 9, 2024 18:54

Emyrk added 6 commits December 9, 2024 12:56

go mod tidy

0de43fd

biome lint ignore needs a reason

e495600

update readonly values

e46ba70

pluralize words in enums

d5c9520

remove old gen

b663db8

chore: null vs omitempty

0b436bc

Emyrk marked this pull request as ready for review December 9, 2024 22:00

Emyrk added 6 commits December 9, 2024 16:14

linting

31161ff

linting

4077d50

fixup! linting

59a13ad

be explicit for which generated files to ignore

5d58e13

fix js test assert

7ae88e5

fix biome ignore list

e0a967a

Emyrk requested review from f0ssel and Parkreiner December 10, 2024 13:55

add comments

a632a84

f0ssel approved these changes Dec 11, 2024

View reviewed changes

remove unused imports

9f31aa5

Emyrk merged commit 077e594 into main Dec 11, 2024
32 of 33 checks passed

Emyrk deleted the stevenmasley/types_gen branch December 11, 2024 20:39

github-actions bot locked and limited conversation to collaborators Dec 11, 2024

Parkreiner reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: switch to guts for typescript types generation #15801

chore: switch to guts for typescript types generation #15801

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chore: switch to guts for typescript types generation #15801

chore: switch to guts for typescript types generation #15801

Uh oh!

Conversation

Uh oh!

What this does

Some changes

Future work

Bug

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!