Try swagger-typescript-api for real #535

david-crespo · 2021-11-28T21:32:48Z

As noted in #458, swagger-typescript-api generates much better types, especially for enums. Another benefit is that generated code is much smaller, so the core bundle (just our code, not vendor code) goes from 137 KB to 82 KB.

Another advantage is that it is trivial to point the generator to a custom set of templates, so we could easily take the default ones, check them in here, and tweak as needed. Overriding the templates in the official openapi-generator is a nightmare by comparison. It's also much easier to fork, as discussed below. No longer relevant, as I had to fork anyway.

Summary of changes

Swap out openapi-generator for swagger-typescript-api in scripts and swap out corresponding generated clients
Use patch-package'd swagger-typescript-api instead of the real one
Convert useApiQuery and friends to use the new client

Update mutate calls to use body instead of named params key, e.g.,

- projectCreate: { name, description },
+ body: { name, description },

ProjectCreateForm and InstanceCreateForm take onSuccess callbacks so we can assert they get called in the tests
Wrap every use of a date field in new Date() because the format functions require a Date, and the new client doesn't parse them for us like the old one did

Bug made me fork

There appears to be a bug in the --extract-request-params flag, which pulls path and query params into a single params object. It doesn't seem to work for all request functions — there are a few that still have separate path param args. I'll indicate one with a comment. I figured out the cause (I think) and wrote up an issue on the STA repo here: acacode/swagger-typescript-api#322. It would be trivial to fix in the code. Unfortunately, it would be a lot of work to fix it in template overrides alone (which would be nice because we wouldn't have to fork the repo). I think it would be possible, but it would be a lot worse than forking because the logic would be all twisted.

Summary of changes in the fork

See patches/swagger-typescript-api+9.3.1.patch.

I forked STA at oxidecomputer/swagger-typescript-api with what seems like a fix for the bug, and f5aba99 shows the result of pointing to the fork: params are extracted into a single object like they're supposed to.

Fix the bug above — always extract query params (commit)
Hard-code module namespace for all routes to methods so they all go on a single api.methods object (relevant line of commit — commit has a bunch of formatting noise due to commit hook in repo)
Delete prepare script that runs all tests after cloning, which made yarn install take several minutes (commit)

Considerations

Cloning from GitHub is kind of annoying. Maybe we can try to get (acceptable versions of) our changes to the code merged so we can then only override the templates.

vercel · 2021-11-28T21:32:51Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/HfEKkHfQP39fjinzG7J2ysuMtdnh
✅ Preview: https://console-ui-storybook-git-swagger-typescript-api-oxidecomputer.vercel.app

github-actions · 2021-11-28T21:33:02Z

Preview will be deployed at https://console-git-swagger-typescript-api.internal.oxide.computer

david-crespo · 2021-11-28T21:35:46Z

app/pages/ProjectsPage.tsx

@@ -76,7 +76,7 @@ const ProjectsPage = () => {
            </section>
            <footer className="p-4 border-t border-gray-400 text-xs">
              <span className="uppercase">
-                {formatDistanceToNowStrict(item.timeCreated, {
+                {formatDistanceToNowStrict(new Date(item.timeCreated), {


It doesn't parse dates by default. we fix this on the generator side by automatically parsing timeCreated and timeModified fields (or maybe any field that starts with time) as dates.

david-crespo · 2021-11-28T21:38:52Z

libs/api/__generated__/Api.ts

+     * @request POST:/organizations/{organization_name}/projects
+     */
+    organizationProjectsPost: (
+      organizationName: Name,


This seems like a bug to me. Compare to organizationProjectsGet, which puts the organizationName path param in an object. This is a problem for the way I want to extract the params arg type in the hook. I need there to be a fixed number of arguments. There might be a workaround but it would be gnarly.

david-crespo · 2021-11-28T21:39:24Z

libs/api/__generated__/Api.ts

+  | { instance: string; state: 'attached' }
+  | { instance: string; state: 'detaching' }
+  | { state: 'destroyed' }
+  | { state: 'faulted' }


this is 👌 👌 👌

david-crespo · 2021-11-28T21:40:09Z

libs/api/__generated__/Api.ts

+
+/**
+ * An IPv4 subnet, including prefix and subnet mask
+ * @pattern ^(10\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9]\.){2}(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])/(1[0-9]|2[0-8]|[8-9]))$^(172\.16\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])/(1[2-9]|2[0-8]))$^(192\.168\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])/(1[6-9]|2[0-8]))$


seems like we could easily tweak the templates to get this pattern in the code so we could use it for validation client-side

david-crespo · 2021-12-01T20:08:56Z

libs/api/hooks.ts

+    options?: UseMutationOptions<
+      Result<A[M]>,
+      ErrorResponse,
+      { params: Params<A[M]>; body?: Body<A[M]> }


The old way was more like Params<A[M]> & { body?: Body<A[M]> }, i.e., the path params are at top level and only body is under a key. That could work fine for us here. I'll probably try it just to make the diff smaller.

confirmed a5a7816 takes a lot of noise out of the diff

david-crespo · 2021-12-06T22:13:02Z

the stats look really bad, but if you exclude yarn.lock and the generated client it's surprisingly small

~/oxide/console $ git diff --stat main -- . ":(exclude)yarn.lock" ":(exclude)libs/api/__generated__/"
 .github/workflows/packer.yaml                               |  2 +-
 Dockerfile                                                  |  1 +
 app/components/InstancesTable.tsx                           | 10 ++---
 app/pages/LoginPage.tsx                                     | 11 ++---
 app/pages/ProjectCreatePage.tsx                             | 27 ++++++++----
 app/pages/ProjectsPage.tsx                                  |  2 +-
 app/pages/__tests__/InstanceCreateForm.spec.tsx             | 21 +++++++---
 app/pages/__tests__/ProjectCreatePage.spec.tsx              | 19 ++++++---
 app/pages/project/instances/create/InstancesCreatePage.tsx  | 19 +++++----
 app/pages/project/networking/VpcPage/tabs/VpcSubnetsTab.tsx |  2 +-
 app/util/errors.spec.ts                                     | 29 +++++--------
 app/util/errors.ts                                          |  8 ++--
 libs/api-mocks/instance.ts                                  |  6 +--
 libs/api-mocks/org.ts                                       |  4 +-
 libs/api-mocks/project.ts                                   |  4 +-
 libs/api/__tests__/hooks.spec.tsx                           | 13 +++---
 libs/api/hooks.ts                                           | 77 +++++++++++++++++++---------------
 libs/api/index.ts                                           | 26 +++++++-----
 libs/api/instance-can.ts                                    |  2 +-
 libs/table/QueryTable.tsx                                   | 17 +++-----
 libs/table/cells/DateCell.tsx                               |  6 +--
 libs/table/cells/InstanceStatusCell.tsx                     |  2 +-
 package.json                                                |  1 +
 packer/oxapi_demo                                           | 27 ++++++++----
 packer/provision.sh                                         |  3 ++
 tools/create_gcp_instance.sh                                |  2 +-
 tools/generate_api_client.sh                                | 15 ++-----
 27 files changed, 196 insertions(+), 160 deletions(-)
 ``

david-crespo · 2021-12-06T22:17:54Z

app/pages/__tests__/InstanceCreateForm.spec.tsx

    const mock = fetchMock.post(instancesUrl, { status: 201, body: instance })

-    const projectPath = `/orgs/${org.name}/projects/${project.name}`
-    expect(window.location.pathname).not.toEqual(projectPath)


this was never a good way to do this anyway, so I don't miss it. the problem was the location was being set by a previously run test, so this not equal was failing

zephraph · 2021-12-06T22:43:48Z

.github/workflows/packer.yaml

-          API_VERSION: de84bb85d6ca264b1adddac9822e173aeb2b433a
+          API_VERSION: 038dbd04fbc292b5f7880c0d0c723ef182145cb2


Okay, I've seriously got to fix this. If the file isn't changed in the current PR, this shouldn't be ran. Why it happens anyway is a mystery to me.

I think it watches the packer directory, which did have changes

david-crespo · 2021-12-06T22:44:04Z

app/pages/ProjectCreatePage.tsx

-      <ProjectCreateForm orgName={orgName} />
+      <ProjectCreateForm
+        orgName={orgName}
+        onSuccess={(project) => {


a function that takes the created project is a surprisingly natural interface here

app/pages/project/networking/VpcPage/tabs/VpcSubnetsTab.tsx

david-crespo added 3 commits November 27, 2021 16:06

working get projects request

a8b0767

basically functioning typed hook

79d157c

simpler error handling, nicer comment

9f407ed

be sneaky with imports to show it's a drop-in replacement

db6236d

vercel bot deployed to Preview November 28, 2021 21:33 View deployment

david-crespo commented Nov 28, 2021

View reviewed changes

david-crespo temporarily deployed to Preview VM November 28, 2021 21:37 Inactive

david-crespo commented Nov 28, 2021

View reviewed changes

make STA a dev dep instead of using npx

d7deee0

vercel bot deployed to Preview November 29, 2021 18:27 View deployment

david-crespo temporarily deployed to Preview VM November 29, 2021 18:32 Inactive

vercel bot had a problem deploying to Preview November 29, 2021 19:46 Failure

show corrected params output with forked STA

f5aba99

david-crespo force-pushed the swagger-typescript-api branch from 60ca7f5 to f5aba99 Compare November 29, 2021 19:53

vercel bot deployed to Preview November 29, 2021 19:53 View deployment

vercel bot deployed to Preview November 29, 2021 22:00 View deployment

everything works except the login page

057a7db

david-crespo force-pushed the swagger-typescript-api branch from c4f62ec to 057a7db Compare November 29, 2021 22:02

delete openapi-generator client

d2b6ca6

vercel bot deployed to Preview November 29, 2021 22:03 View deployment

david-crespo commented Dec 1, 2021

View reviewed changes

make mutate() argument look more like before

a5a7816

vercel bot deployed to Preview December 1, 2021 20:14 View deployment

Merge main into swagger-typescript-api

72cec66

vercel bot deployed to Preview December 6, 2021 20:21 View deployment

fix type errors on everything but login

111d3aa

automatically update packer-id

9863895

vercel bot deployed to Preview December 6, 2021 21:49 View deployment

jessfraz temporarily deployed to Preview VM December 6, 2021 21:52 Inactive

tests pass

b564bbd

vercel bot deployed to Preview December 6, 2021 22:10 View deployment

david-crespo temporarily deployed to Preview VM December 6, 2021 22:12 Inactive

david-crespo commented Dec 6, 2021

View reviewed changes

don't be scared, extract the whole onSuccess

358bdc9

vercel bot deployed to Preview December 6, 2021 22:43 View deployment

zephraph reviewed Dec 6, 2021

View reviewed changes

david-crespo commented Dec 6, 2021

View reviewed changes

zephraph reviewed Dec 6, 2021

View reviewed changes

app/pages/project/networking/VpcPage/tabs/VpcSubnetsTab.tsx Show resolved Hide resolved

david-crespo temporarily deployed to Preview VM December 6, 2021 22:46 Inactive

david-crespo marked this pull request as ready for review December 6, 2021 22:51

use package-patch instead of swagger (including patched template)

4fe9652

vercel bot deployed to Preview December 7, 2021 00:12 View deployment

don't need git if we're not using a fork

8f4836f

vercel bot deployed to Preview December 7, 2021 00:15 View deployment

david-crespo temporarily deployed to Preview VM December 7, 2021 00:17 Inactive

automatically update packer-id

fbb5e82

vercel bot deployed to Preview December 7, 2021 00:21 View deployment

jessfraz temporarily deployed to Preview VM December 7, 2021 00:24 Inactive

remove codemods, update link in readme

a6fb60d

vercel bot deployed to Preview December 7, 2021 01:11 View deployment

zephraph deployed to Preview VM December 7, 2021 01:13 View deployment

zephraph merged commit 47a11de into main Dec 7, 2021

zephraph deleted the swagger-typescript-api branch December 7, 2021 01:18

This was referenced Dec 12, 2021

Render the whole app in page-level tests #565

Merged

Experiment with other OpenAPI client generators #458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try swagger-typescript-api for real #535

Try swagger-typescript-api for real #535

david-crespo commented Nov 28, 2021 •

edited

Loading

vercel bot commented Nov 28, 2021 •

edited

Loading

github-actions bot commented Nov 28, 2021

david-crespo Nov 28, 2021

david-crespo Nov 28, 2021

david-crespo Nov 28, 2021

david-crespo Nov 28, 2021

david-crespo Dec 1, 2021

david-crespo Dec 1, 2021

david-crespo commented Dec 6, 2021

david-crespo Dec 6, 2021

zephraph Dec 6, 2021

david-crespo Dec 6, 2021

david-crespo Dec 6, 2021

		API_VERSION: de84bb85d6ca264b1adddac9822e173aeb2b433a
		API_VERSION: 038dbd04fbc292b5f7880c0d0c723ef182145cb2

Try swagger-typescript-api for real #535

Try swagger-typescript-api for real #535

Conversation

david-crespo commented Nov 28, 2021 • edited Loading

Summary of changes

Bug made me fork

Summary of changes in the fork

Considerations

vercel bot commented Nov 28, 2021 • edited Loading

github-actions bot commented Nov 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo commented Nov 28, 2021 •

edited

Loading

vercel bot commented Nov 28, 2021 •

edited

Loading