coder
coder copied to clipboard
refactor: improve test isolation for Axios API logic
Closes #13013
Summary
This PR moves all the Axios-based API functionality behind a Client
class. This lets you can instantiate a brand new client with all Axios functionality scoped to that specific instance via new Client()
. For convenience, the file also exports a global-ish client called client
(lowercase)
I knew this PR was going to be huge ahead of time, so I deliberately restricted myself to moving things around only. There should be zero changes in overall functionality.
Changes made
- Created new
Api
andClient
classes inside ofsrc/api/api.ts
, and turned all Axios methods into methods forApi
Why these changes?
- We are currently trying to bring "SDK-ish" functionality to the Backstage plugin, letting users make any API calls they want with full type-safety.
- One of the limitations, though, is that even though the Coder codebase is not using the global Axios instance, it's still basically a global singleton value
- One of the core pieces of the Backstage plugin is a
CoderClient
class that is in charge of caroling the Backstage APIs, and will eventually need to expose the Coder "SDK". As part of this functionality, it also needs to patch Backstage-specific information via the Axios interceptors, with some of this info being dynamic per request - Some of the Backstage plugin test cases instantiate a new class per case. This works fine without bringing in the Coder functionality, but if we were to bring in the Coder code unchanged, every single test would need to go through a set of manual cleanup steps to ensure no interceptor collisions. Otherwise, you get a bunch of test flakes, because all the
CoderClient
s would be going through a singleton - These are not challenges that can simply be solved with the
let
keyword; the only real solution is to have something "factory-ish". While you could make a single function that sets things up via closure, there's so much functionality that a class felt like a better organization tool - Edit (2024-05-07): We decided to vendor things for Backstage (basically copy-paste the files as a stopgap), but all these changes are still very helpful for the Backstage tests
- While core isn't making use of the benefits of this new isolation just yet, this PR will make it significantly easier to spin up arbitrary clients per test case, and get more sophisticated with interceptor logic
Cliff notes of what changes
The Client
class implements this API:
interface ClientApi {
api: Api;
getCsrfToken: () => string;
setSessionToken: (token: string) => void;
setHost: (host: string | undefined) => void;
getAxiosInstance: () => AxiosInstance;
}
For convenience, the api
file exports a default client
instance:
export const client = new Client();
If you were doing imports like this for the Axios functionality:
import * as api from "api/api";
const workspaces = api.getWorkspaces(/* args */);
Not much changes – the import now looks like this:
import { client } from "api/api".;
const workspaces = client.api.getWorkspaces(/* args */);
This also means that if you were relying on * as api
imports to handle Jest mocking, you now have a more straightforward option:
jest
.spyOn(client.api, "postWorkspaceBuild")
.mockResolvedValueOnce(MockWorkspaceBuild);
And while this is all class-based, steps have been taken to make sure that the API functions can freely be passed around any number of React components without losing their this
context:
export function workspaces(config: WorkspacesRequest = {}) {
const { q, limit } = config;
return {
queryKey: workspacesKey(config),
queryFn: () => client.api.getWorkspaces({ q, limit }),
} as const satisfies QueryOptions<WorkspacesResponse>;
}
Putting off prepping this PR for review until I have #13130 done
@aslilac Updated the code to flatten things out. Nothing else should've changed, aside from me updating getAppearance
to accommodate your banner PR from the other day
Also put some comments in the api.ts
file about why arrow functions are needed