Skip to content
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

Stop using global Axios instance for API calls #13013

Closed
Kira-Pilot opened this issue Apr 19, 2024 · 1 comment · Fixed by #13125
Closed

Stop using global Axios instance for API calls #13013

Kira-Pilot opened this issue Apr 19, 2024 · 1 comment · Fixed by #13125
Assignees
Labels
chore Non-customer facing refactors, cleanup, or technical debt. site Area: frontend dashboard

Comments

@Kira-Pilot
Copy link
Member

Related to coder/backstage-plugins#114

We use a global Axios interceptor instance in /site. As discussed in the ticket linked above, we would like to import our API definitions into external applications, specifically Backstage. In order to avoid configuration collisions or intercepting requests unrelated to Coder, we should scope our instance appropriately and document our usage of Axios.

@coder-labeler coder-labeler bot added chore Non-customer facing refactors, cleanup, or technical debt. site Area: frontend dashboard labels Apr 19, 2024
@Parkreiner Parkreiner changed the title Scope Axios interceptor instance to our API Stop using global Axios instance for API calls Apr 22, 2024
@Parkreiner
Copy link
Contributor

Parkreiner commented Apr 22, 2024

Looking into this now

There's a quick-and-dirty fix that comes to mind:

  1. Rename the global Axios import to globalAxios
  2. Use globalAxios.create to make a new Axios instance that is scoped to the ES module file
  3. Update all our API functions to use the new instance instead

My main worry is testing setup. The above change would solve the issue of collisions with other Axios users, but we'd still have a "global singleton-ish" value, even though it'd be scoped more defensively to Coder-only files.

I've already run into a testing issue when making new tests for Backstage:

import globalAxios from "axios";
const axiosInstance = globalAxios.create();

class CoderClient {
  constructor() {
    // Do other stuff before Axios call
    axiosInstance.interceptors.request.use(this.interceptAxiosRequest);
  }

  interceptAxiosRequest = async () => {
    // Pass along class state to each outgoing request. Needs to be defined
    // as arrow function to avoid loss of `this` context
  };
}

test("Test case 1", () => {
  const client1 = new CoderClient();
});

// Even after test 1 finishes, client1 can't be garbage-collected,
// because the Axios instance has an interceptor function with
// closure over client1. In effect, this means that when test case 2 makes
// an API request, the interceptor from client2 will apply first, but then
// the interceptor from client1 will also run, likely overwriting the
// request with stale data
test("Test case 2", () => {
  const client2 = new CoderClient();
});

I have some logic in place to make sure the classes clean up after themselves following each test (coderClient.cleanupClient), but it feels flaky/fragile, and it feels like the better option is to increase testing isolation. My main worry, though, is that I don't know if you can do that without rewriting the entire file to use classes instead of functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Non-customer facing refactors, cleanup, or technical debt. site Area: frontend dashboard
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants