feat: Switch packages for typescript generation code #1196
Conversation
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.
praise: Wonderful documentation!
| readonly email: string | ||
| readonly username: string | ||
| readonly password: string | ||
| readonly organization_id: string |
There was a problem hiding this comment.
Pretty much any non-primitive (basic type) wasn't included before. uuid is a named array type of byte.
All now handled. The unhandled cases in the past omitted the field, now they use any and leave a comment.
coder/site/src/api/typesGenerated.ts
Lines 213 to 215 in 3865f36
I also comment where I use some "intelligence" about external types.
coder/site/src/api/typesGenerated.ts
Lines 227 to 228 in 3865f36
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.
| 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.
Idk if anyone actually uses this... but https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer exists.
There was a problem hiding this comment.
If we add []byte to a type, we can adjust as needed. For now, I just use a number for a byte, and string for []byte 🤷.
TIL about that
There was a problem hiding this comment.
I was guessing these types should accept the golang marshal json. So this will at least work 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.
We should focus on making this script work perfectly for what we already plan to do in codersdk. 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.
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 for all 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.
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.
I like my current approach of just leaving a comment that it should be fixed and accept any.
I was afraid to error, as it's kinda a pain when the error blocks your work. The any type is valid typescript, so it still will 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.
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": | ||
| F438 | // All byte arrays are strings on the typescript. |
There was a problem hiding this comment.
@vapurrmaid could you weigh in here?
There was a problem hiding this comment.
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") |
| // 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.
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. |
| 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.
Would be dope if this was a yaml config instead.
* feat: Switch packages for typescript generation code Supports a larger set of types - [x] Basics (string/int/etc) - [x] Maps - [x] Slices - [x] Enums - [x] Pointers
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?