- Notifications
You must be signed in to change notification settings - Fork928
feat: Switch packages for typescript generation code#1196
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedApr 28, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1196 +/- ##==========================================- Coverage 66.00% 56.64% -9.37%========================================== Files 265 265 Lines 16751 16937 +186 Branches 157 157 ==========================================- Hits 11057 9594 -1463- Misses 4533 6280 +1747+ Partials 1161 1063 -98
Continue to review full report at Codecov.
|
| @@ -0,0 +1,39 @@ | |||
| # APITypings | |||
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.
praise: Wonderful documentation!
| readonly email: string | ||
| readonly username: string | ||
| readonly password: string | ||
| readonly organization_id: 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.
👍 nice, there it is
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.
cc@presleyp 🎉
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.
Pretty much any non-primitive (basic type) wasn't included before.uuid is anamedarray type ofbyte.
All now handled. The unhandled cases in the past omitted the field, now they useany and leave a comment.
coder/site/src/api/typesGenerated.ts
Lines 213 to 215 in3865f36
| // Named type "github.com/coder/coder/coderd/database.ParameterValue" unknown, using "any" | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| readonlyParameterValue:any |
I also comment where I use some "intelligence" about external types.
coder/site/src/api/typesGenerated.ts
Lines 227 to 228 in3865f36
| // This is likely an enum in an external package ("github.com/coder/coder/coderd/database.ParameterSourceScheme") | |
| readonlydefault_source_scheme:string |
One thing to note is nothing here is set in stone, and if the FE team thinks a type is wrong, we can fix it. The current implementation comments all guesses though, so you know where things are maybe a bit ambiguous, or could be improved.
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.
Overall great improvement. Glad we can even add doc comments for types and fields in the future with this route. Only real unknowns are the byte arrays but we can figure that out.
| case bs.Info()&types.IsBoolean > 0: | ||
| return TypescriptType{ValueType: "boolean"}, nil | ||
| case bs.Kind() == types.Byte: | ||
| // TODO: @emyrk What is a byte for typescript? A string? A uint8? |
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.
Idk if anyone actually uses this... buthttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer exists.
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.
If we add[]byte to a type, we can adjust as needed. For now, I just use anumber for a byte, andstring for[]byte 🤷.
TIL about that
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 was guessing these types should accept the golang marshal json. So this will at leastwork in the sense the 2 types can be communicated over json.
scripts/apitypings/main.go Outdated
| ) | ||
| // TODO: Handle httpapi.Response and other types |
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.
We should focus on making this script work perfectly for what we already plan to do incodersdk. If this is relevant with that in mind, you should probably make this more specific to the code that handles this.
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 don't know if we need it. This is really to match Go types, but the general error/response type is not bad to implement once in TS forall endpoints that use it.
I don't want to run this code on another package, and type inferencing across packages is only possible to an extent.
I removed the TODO, but the basic thing is that external types are not currently handled. The only way to handle them better, is to run this code on those packages as well. Obviously then we want to allowlist types, vs the current deny list approach.
Here is the example of external types:
http://github.com/coder/coder/blob/3865f36c4bf8c2ddfc1c9081cf7bbf1a881675f6/codersdk/templateversions.go#L27-L30
They use database types that include enums. I would like to have a conversation about how to handle these, as we can handle them. Just a matter of "how"
| return TypescriptType{ValueType: bs.Name()}, nil | ||
| } | ||
| case *types.Struct: | ||
| // This handles anonymous structs. This should never happen really. |
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.
Maybe we just error and tell the user not to do that? I could go either way, but idk if it's worth explicitly supporting.
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 like my current approach of just leaving a comment that it should be fixed and acceptany.
I was afraid to error, as it's kinda a pain when the error blocks your work. Theany type is valid typescript, so it stillwill work from a compile time perspective.
I do see the benefit of an error though as it does force the change up front. I just see this code as more a way for the FE and BE to agree on api types, rather than a strict document that has no flexibility.
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 can cave on this if you feel strongly otherwise. In a month or two, I'm happy to make this more strict as we know more what we want.
| // When type checking here, just use the string. You can cast it | ||
| // to a types.Basic and get the kind if you want too :shrug: | ||
| case arr.Elem().String() == "byte": | ||
| // All byte arrays are strings on the typescript. |
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.
@vapurrmaid could you weigh in here?
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.
Yea, really curious here. The json marshal is a string, which is why I chose it. It's base64 encoded. Not ideal still?
| readonly name: string | ||
| // This is likely an enum in an external package ("github.com/coder/coder/coderd/database.ParameterSourceScheme") |
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.
Nice stop gap 👍
| // From codersdk/provisionerdaemons.go:26:6. | ||
| export type ProvisionerJobStatus = "pending" | "running" | "succeeded" | "canceling" | "canceled" | "failed" | ||
| // From codersdk/provisionerdaemons.go:26:6 | ||
| export type ProvisionerJobStatus = "canceled" | "canceling" | "failed" | "pending" | "running" | "succeeded" |
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.
ty for sorting, will prevent flakes
If we don't see us returning byte arrays from codersdk in the near future we can leave what is here and kick that can down the road. My guess would be an array of ints but idk for 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.
Yay, thanks!
| switch n.String() { | ||
| case "net/url.URL": | ||
| return TypescriptType{ValueType: "string"}, nil | ||
| case "time.Time": | ||
| // We really should come up with a standard for time. | ||
| return TypescriptType{ValueType: "string"}, nil | ||
| case "database/sql.NullTime": | ||
| return TypescriptType{ValueType: "string", Optional: true}, nil | ||
| case "github.com/google/uuid.NullUUID": | ||
| return TypescriptType{ValueType: "string", Optional: true}, nil | ||
| case "github.com/google/uuid.UUID": | ||
| return TypescriptType{ValueType: "string"}, nil | ||
| } |
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.
Would be dope if this was a yaml config instead.
* feat: Switch packages for typescript generation codeSupports a larger set of types - [x] Basics (string/int/etc) - [x] Maps - [x] Slices - [x] Enums - [x] Pointers
Uh oh!
There was an error while loading.Please reload this page.
Redo typescript autogen
Ok, this got a lot more complicated, but it handles all the cases. I didn't add a unit test, because we will just use the generated types as our testing. If a type is wrong, hopefully the FE team will complain and we fix it.
anyatm)Features
Ignore types
http://github.com/coder/coder/blob/071026deca83f558f9c5be3c70f90cda0105aa82/codersdk/client.go#L29-L29
Choose typescript values
Issues
time.Time?