coder icon indicating copy to clipboard operation
coder copied to clipboard

refactor: improve test isolation for Axios API logic

Open Parkreiner opened this issue 10 months ago • 2 comments

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 and Client classes inside of src/api/api.ts, and turned all Axios methods into methods for Api

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 CoderClients 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>;
}

Parkreiner avatar May 01 '24 21:05 Parkreiner

Putting off prepping this PR for review until I have #13130 done

Parkreiner avatar May 02 '24 13:05 Parkreiner

@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

Parkreiner avatar May 09 '24 18:05 Parkreiner