Skip to content

feat: generate typescript types from codersdk structs #1047

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 24 commits into from
Apr 19, 2022

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented Apr 15, 2022

This isn't quite there yet, but I wanted to see how close I could get if I timeboxed to 30 minutes. I couldn't find any decent looking libraries in go to do this that didn't complicate things, so I reached for the go AST.

If we like this idea we could see how much work it would take to make this usable.

Update:
I've worked on making this production ready for us to use on the frontend. This generates a new types-generated.ts file that the frontend team can migrate to.

@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #1047 (7b63673) into main (1df943e) will decrease coverage by 0.75%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1047      +/-   ##
==========================================
- Coverage   67.42%   66.66%   -0.76%     
==========================================
  Files         260      261       +1     
  Lines       15374    15502     +128     
  Branches      152      152              
==========================================
- Hits        10366    10335      -31     
- Misses       3966     4118     +152     
- Partials     1042     1049       +7     
Flag Coverage Δ
unittest-go-macos-latest 53.30% <0.00%> (-0.61%) ⬇️
unittest-go-postgres- 65.90% <0.00%> (-0.71%) ⬇️
unittest-go-ubuntu-latest 55.86% <0.00%> (-0.65%) ⬇️
unittest-go-windows-2022 52.75% <0.00%> (-0.61%) ⬇️
unittest-js 67.96% <ø> (ø)
Impacted Files Coverage Δ
scripts/apitypings/main.go 0.00% <0.00%> (ø)
coderd/database/db.go 55.17% <0.00%> (-13.80%) ⬇️
peerbroker/dial.go 77.04% <0.00%> (-6.56%) ⬇️
cli/cliui/agent.go 77.19% <0.00%> (-5.27%) ⬇️
coderd/workspaceagents.go 59.62% <0.00%> (-2.97%) ⬇️
provisionerd/provisionerd.go 79.55% <0.00%> (-1.77%) ⬇️
coderd/provisionerdaemons.go 60.89% <0.00%> (-0.52%) ⬇️
provisioner/terraform/provision.go 68.11% <0.00%> (-0.44%) ⬇️
peer/conn.go 80.71% <0.00%> (+0.25%) ⬆️
peer/channel.go 83.81% <0.00%> (+0.57%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1df943e...7b63673. Read the comment docs.

@f0ssel f0ssel requested a review from greyscaled April 15, 2022 21:05
Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this concept, I was just talking to @code-asher about a really sneaky bug we ran into from having a hand-written type mismatch. There were no compiler warnings or runtime errors or logs, just a field wasn't there, and none of the calling code caused an exception because it handled undefined gracefully.

The one thing (which I don't think blocks anything, more of a FYI type of comment), is that we want to also add mocks for all types we add. It's OK if we still need to do that by hand, just means that you can't auto-generate and fire/forget, you'll have some manual work to do after generating.

Love this ❤️

Copy link
Contributor

@presleyp presleyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very exciting!

@@ -0,0 +1,111 @@
package main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just put this in cmd/typings or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but think this fits much more as a "generate.sh" type of script. I connected it to our make gen command, which I think fits nicer than making it a top level command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It breaks the idiom of all commands being inside the cmd folder. We have the DataDog CI report helper in scripts/datadog-cireport, so it could be there too!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's not a command it's just a script that happens to be go. I'll move to scripts if we want. Is this really any different than the scripts in the coderd/database package? Or should those move into scripts as well ideally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dump should totally move into scripts too. It's just nice to know whether a package should be imported or not based on folder structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool I moved it

coderts/types.ts Outdated
@@ -0,0 +1,172 @@
export interface BuildInfoResponse {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make these link directly to the Go files too... could be neat for documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on generating docs, may happen in this PR or a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the doc codelines

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is cool af

@f0ssel f0ssel requested review from greyscaled and kylecarbs April 18, 2022 21:49
@f0ssel f0ssel marked this pull request as ready for review April 18, 2022 21:49
@f0ssel
Copy link
Contributor Author

f0ssel commented Apr 18, 2022

This feels mergeable at this point for me, I'm going to continue down this path to generate docs as well but don't want that blocking this.

coderts/types.ts Outdated
@@ -0,0 +1,260 @@
// From codersdk/buildinfo.go:10:6.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this into site/src/api/types.ts instead? Otherwise, the site has to import outside of itself, which has caused issues in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome awesome. Approvin'

Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to rename coderts to apitypings or something more specified. Right now it's kinda unclear what it's doing. apitypings isn't great either though so 🤷

Let's remove experiment from the title too, since this has really been upgraded to a full-on feature 🦘🦘🦘

@f0ssel
Copy link
Contributor Author

f0ssel commented Apr 18, 2022

Since I'm moving it under the api directory it doesn't really matter the name anymore outside the command name. I'll make the change.

@kylecarbs
Copy link
Member

Ahh my bad I should have been specific. I just mean for the make gen/x command. It's minor though since make gen does it anyways, so not a biggie either way.

Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type WorkspaceAgentStatus = "connected" | "connecting" | "disconnected"

If it's easier for formatting, you can write it like this:

type Thing =
 | "a"
 | "b"
 | "c"

and just let prettier take care of the formatting (the above is valid TS)

@f0ssel f0ssel requested a review from a team as a code owner April 18, 2022 22:51
@f0ssel f0ssel force-pushed the f0ssel/coderts-generate branch from 748365d to a1f5044 Compare April 18, 2022 23:37
@f0ssel
Copy link
Contributor Author

f0ssel commented Apr 19, 2022

@kylecarbs @vapurrmaid any idea why the CI is failing? I'm not sure why it's not seeing prettier in one of them, and the other I can't figure out why it fails

@kylecarbs
Copy link
Member

image
I got this diff when running make fmt in my workspace. Want me to commit it?

@greyscaled
Copy link
Contributor

image I got this diff when running make fmt in my workspace. Want me to commit it?

That should be committed, totally.

@f0ssel
Copy link
Contributor Author

f0ssel commented Apr 19, 2022

Ok I had broken my prettier command when I renamed the files

@f0ssel
Copy link
Contributor Author

f0ssel commented Apr 19, 2022

Ok @kylecarbs I can't figure out why the make gen is different on my machine than CI. :/

@f0ssel f0ssel changed the title feat: experiment: generate typescript types from codersdk structs feat: generate typescript types from codersdk structs Apr 19, 2022
@f0ssel
Copy link
Contributor Author

f0ssel commented Apr 19, 2022

My machine still has the following: (unsure if related)

-//     protoc        v3.19.4
+//     protoc        v3.6.1

This is in my dogfood workspace so I'm not sure why I'm always out of sync.

@f0ssel
Copy link
Contributor Author

f0ssel commented Apr 19, 2022

My workspace still is making incorrect output from make/gen, but I just changed it to what CI wanted to unblock myself.

@f0ssel f0ssel enabled auto-merge (squash) April 19, 2022 00:44
@f0ssel f0ssel merged commit f46b4cf into main Apr 19, 2022
@f0ssel f0ssel deleted the f0ssel/coderts-generate branch April 19, 2022 00:45
@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants