-
Notifications
You must be signed in to change notification settings - Fork 881
fix: update API code to use Axios instances #13029
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
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.
Most of the file changes are here, but you can probably ignore everything past the first couple of lines. Main changes to the overall file:
- Updated the comment to list what exactly is importing the API file
- Updated all references for the global
axios
instance tocoderAxiosInstance
- Exported
coderAxiosInstance
- Updated some of the calls to
axios.isAxiosError
to justisAxiosError
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 feel that coderAxiosInstance
should never be exported or exposed. If we want to modify it, we should export custom functions for that.
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.
@BrunoQuaresma it needs to be. The VS Code extension mutates it for request stuff.
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.
This is interesting, I would not expect to see other packages/projects accessing the api/api.ts
directly.
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 think that's ideal long-term. Replicate the codersdk in Go
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.
@code-asher Any preferences as for whether we have an optional throwOnError
property that determines how the client resolves errors? I feel like it'd be good to default to false
Most of the time, I would like to do things the Go way, and expose the errors as values, but React Query is built on the assumption that if something fails, you just throw the error directly, and RQ will be the one to catch/sanitize it
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.
Ohhh yeah good point, I forgot about how React Query does it. A throwOnError
property sounds like a good idea, but will that make the types difficult? Maybe we default to throwing for now, since that is the status quo anyway.
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.
So my thinking was that if you set up the client with throwOnError = false
, the usage looks like this:
const { err, data } = await client.getWorkspaces();
And if the client is set up to be true
, it looks like this:
// err no longer exists in the return type
const { data } = await client.getWorkspaces();
We'd have to get a bit in the weeds of TypeScript's type system, and might have to do some type assertions for our own sanity, but I want to say it wouldn't be that bad
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.
If the types seem chill then I am in support!
Also want to ping you @code-asher so that you're aware of the changes for the VS Code plugin |
Anything I need to change to get this through? The Axios instance variable has already been renamed? |
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.
Looks good to me!
Oh one comment, there is an example API call in frontend.md, idk if it really matters but we could update that to use |
Oh, good shout. Let me get that fixed |
Addresses part of #13013
Changes made
axiosInstance
valueNotes
api.ts
file