-
Notifications
You must be signed in to change notification settings - Fork 885
refactor: improve test isolation for Axios API logic #13125
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
Conversation
Putting off prepping this PR for review until I have #13130 done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 spotClient
class – Top-level wrapper for client-ish functionality; contains anApi
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
site/src/api/api.ts
Outdated
interface ClientApi { | ||
api: Api; | ||
getCsrfToken: () => string; | ||
setSessionToken: (token: string) => void; | ||
setHost: (host: string | undefined) => void; | ||
getAxiosInstance: () => AxiosInstance; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just have these defined outside Client
so that I can ensure it explicitly implements the properties (and has type errors if it doesn't)
site/src/api/api.ts
Outdated
? document.head.querySelector('meta[property="csrf-token"]') | ||
: null; | ||
|
||
interface ClientApi { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if instead of having an api
property, this was interface ClientApi extends Api
? Then we don't need the .api
everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
site/src/api/api.ts
Outdated
}; | ||
} | ||
|
||
export const client = new Client(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@aslilac Updated the code to flatten things out. Nothing else should've changed, aside from me updating Also put some comments in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@@ -26,6 +26,8 @@ import type { MonacoEditorProps } from "./MonacoEditor"; | |||
import { Language } from "./PublishTemplateVersionDialog"; | |||
import TemplateVersionEditorPage from "./TemplateVersionEditorPage"; | |||
|
|||
const { API } = api; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 vianew Client()
. For convenience, the file also exports a global-ish client calledclient
(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
Api
andClient
classes inside ofsrc/api/api.ts
, and turned all Axios methods into methods forApi
Why these changes?
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 requestCoderClient
s would be going through a singletonlet
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 toolCliff notes of what changes
The
Client
class implements this API:For convenience, the
api
file exports a defaultclient
instance:If you were doing imports like this for the Axios functionality:
Not much changes – the import now looks like this:
This also means that if you were relying on
* as api
imports to handle Jest mocking, you now have a more straightforward option: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: