refactor: improve test isolation for Axios API logic

[Parkreiner]

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]

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

[Parkreiner]

To give a high-level overview of the file now, there's basically four categories of things in here now:

• Api class – Defines all the Axios-based API calls in one spot
• Client class – Top-level wrapper for client-ish functionality; contains an Api instance
• Ad-hoc helper functions – these weren't moved into either class because, while they help with the API calls, they don't actually have any reliance on Axios. These are all stateless functions
• Additional types – I wish I could move these closer to the methods where they're used, but that's one of the tradeoffs with classes – you can't put type definitions between methods

I was very deliberate about not changing any of the functionality for the methods/functions, aside from changing variable references. You can assume all the overall functionality should be unchanged (especially if the unit/E2E tests are still passing)

Honestly, though, with how many changes this file has, it might be worth looking at it without the diffs

[Parkreiner]

Just have these defined outside Client so that I can ensure it explicitly implements the properties (and has type errors if it doesn't)

[aslilac]

What if instead of having an api property, this was interface ClientApi extends Api? Then we don't need the .api everywhere

[Parkreiner]

I considered that – my main concern was organization/discovery, especially with all the functions the file has

At least with the current approach, you know that everything in api is an API function, while everything else is something directly relevant to manipulating the client itself

[Parkreiner]

This feels like a lot of stuff to have in a completely flat hierarchy

[aslilac]

yes, but the difference between the "flattened" list and the .api list as it is is only 4 functions, and they're 4 functions you hardly ever want to call. it feels odd to me to hide all of stuff you actually want the deepest in the hierarchy, and there are plenty of place's in the go bits of the codebase where we flatten things for exactly this reason.

[Parkreiner]

Yeah, I can see that. If we're trying to optimize for the most common use case, then (1) the switch reduces the amount of typing the user needs to do, and (2) it reduces the emphasis placed on methods that are effectively escape hatches, and should probably be a little more hidden/hard to find

You changed my mind – I'll get this updated tomorrow morning

[aslilac]

Just a thought: if we named this API we could reduce the diff in a bunch of places. Change the import, but the code stays the same.

[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

[aslilac]

I quite dislike the one file that has global constants named both api and API, but I don't really have a great suggestion for fixing that given the need to use jest.mock in there. Otherwise, I like it!

[aslilac]

oh no

[Parkreiner]

Yeah, let me see if I can't rename some thing really quick to make things less confusing. I didn't think that part through after I consolidated the classes