Very much a nit haha, but why a size check over return true in the if otherwise return false?

Another nit, since this is only used in one spot, is it worth a method? Could maybe just have eject there and not add/remove to the tracked IDs at all, since the finally will always remove it anyway.

Yeah, good call on the size check – your suggestion's a lot more straightforward

As for whether the method makes sense, I guess my gut feeling is that if I'm going to have a special add method for the interceptors, it makes sense to have an obvious way to remove them, too. Plus, since it's only a private method, it'd be easy to remove down the line, too

Will this cause calls not to error? Like if I was using this in my own project, and called client.getWorkspaces() and it got aborted, would it return with undefined?

If so, part of me wants a more explicit Go-like syntax to check if the error is an abort error (errors.Is(err, context.Canceled)) but at the same time, maybe this is React-y, and as long as the editor tells me "undefined means aborted" when I look at the function definition, that would not be too unreasonable.

So, as far as I can tell from the Axios docs, the way the error interceptor works is that, Axios will run all the interceptors, and then if the value it gets back is not undefined, Axios will treat that value as an error and throw it (even if the value isn't an Error instance). I don't think it's inherently React-ish – it's strictly an Axios thing

I could've just had return, but Backstage's linters complain about mixing return and return <value> in the same function

Ah yup, I get that the return undefined here is causing Axios to not throw and not being used itself as the return of client.getWorkspaces(), but I wonder what now is the return of client.getWorkspaces() with the error having been scrubbed? Since it did not throw, but there was also no data because it aborted, is it just naturally undefined maybe?

And if so, maybe returning undefined for canceled things is React-like so this is fine, was my thinking, but honestly I have no idea. 😛 So you would write code like:

const workspaces = client.getWorkspaces()
if (workspaces === undefined) {
  // This means the request was canceled
}

Ooh, yeah, good point – realistically, I only expect the cleanupClient function to be used for testing, but that is a hole in the logic that could pause pain points down the line

It's not the worst thing in the world – React Query exposes the error property as type unknown by default to be defensive, so technically, all RQ code already has to deal with an "error" potentially being undefined. But if I do need to keep this method for connecting to the Coder core files, I'll be sure to figure something out

Oh yeah good point, this really only comes into play when cleaning up the client.

Yikes that is confusing, good catch!

I like this approach!

Is this override temporary or here to stay? I wonder if we override it with the return below instead, like:

const getWorkspaces = () {}
// stuff
{
 ...baseSdk,
  getWorkspaces,
}

Yeah, good call. I think doing it that way is more clear

Mostly just musing, but if we did not have a proxy we could use URL to resolve the base:

To piggy back off my comment in the other PR, we should probably validate the response data.

Does AxiosError cover all non-20x status codes? I thought maybe it only covered failures to make the request.

Yeah, agreed – going to fix really quick

Actually, I looked this up, and the answer is "it depends". By default, AxiosError does cover all non-2xx status codes. But you can add a validateStatus function to the axios.get call to customize your logic for how you treat the status code

So right now, CoderClient isn't doing that, and neither is Coder Core. But once we bring in the core files, things could always change if we're not careful

Oooooooooooooohhh right now that you say that, I remember seeing validateStatus in Coder core. That makes sense.

getUserLoginType does seem reasonable to me, but in our other plugins we get the user; maybe it could be unclear why we do it differently in one spot? Could be cool to cache that user info at some point too in case we want to show the user's name or something.

Oh, for some reason, when I was looking at the functions in the api.ts file, I somehow missed getAuthenticatedUser

I wanted to mirror the function calls that we have there, but if we still have this function, I definitely think it makes more sense to use that

I like this how thorough this is with the cleanup logic, but the client is meant to last for the lifetime of the application, right? Is there a case where you would need to recreate the client? I wonder if we should write it with the assumption it is long-lived, at least until/unless there is a use case.

Yeah, it's sort of a carry over from the previous PR I closed out. There, it actually was necessary to have cleanup logic, because the file was operating under the assumption that we'd be importing a single Axios instance from the Coder core files, and you needed a way to ensure isolation for Client instances spun up for different test cases. Right now, CoderClient gets a brand new Axios instance each time it's instantiated, but that may or may not have to change, based on what I can change in core

So at least for right now, I kind of threw it in here just because. The actual CoderClient in production should basically be a singleton that never gets removed (especially since it's not tied to React rendering, so it can't get conditionally unmounted). Just to be on the cautious side, I'm going to hold onto the method for this PR, and then depending on how much luck I have with the Coder core changes to improve its test isolation, I'll either:

1. Remove this method because I'll be able to get guaranteed isolation from Coder core
2. Beef up the method because getting isolation set up in Core is harder than I thought

Perfection!

Might be a cool use case for Set.