Constructors of `OAuth2Client` and `OAuth2Fetch` capture values causing race conditions sometimes

[debater-coder]

When I updated from 2.0.18 to 2.2.1 I noticed that some of my tests stopped passing. Some of these tests relied on msw. When used in a Node.js test environment, it alters the fetch function initially supplied by jsdom. However in update 2.2.0, the current value of fetch is captured here. This means that if the constructor is called before msw has finished mocking fetch, the initial jsdom implementation is captured and used instead.

Previously, I had a file which looked like this:

import { OAuth2Client } from "@badgateway/oauth2-client";
import config from "./config";

export const client = new OAuth2Client({
      server: config.server,
      clientId: config.client_id,
      tokenEndpoint: "/api/token",
      authorizationEndpoint: config.authorization_endpoint,
    });

This means that the constructor is called as soon as the module is imported, and msw may not have had a chance to mock yet. The solution for me, was to defer the creation of the client until it is needed like so:

import { OAuth2Client } from "@badgateway/oauth2-client";
import config from "./config";

let _client: OAuth2Client | null = null;

export const getClient = () => {
  if (!_client) {
    _client = new OAuth2Client({
      server: config.server,
      clientId: config.client_id,
      tokenEndpoint: "/api/token",
      authorizationEndpoint: config.authorization_endpoint,
    });
  }
  return _client;
};

However, this may not be obvious to new users of the library unfamiliar with the source code. A similar issue occurs with the getStoredToken option, which is invoked immediately by the constructor, which can be an issue if the persistent store needs initialisation.

Expected behaviour for the first case would be that instead of capturing the value at the constructor, the client could check the existence of the custom implementation when it is needed, like this:

this.settings.fetch?.(...) ?? fetch(...)

This could be made into a method on the OAuth2Client class:

customFetch(request: Request, init?: RequestInit | undefined) {
    return this.settings.fetch?.(...) ?? fetch(...)
}

Alternatively, if this is intended behaviour it could be documented in the README, but this may be a breaking change.

For the second case the expected behaviour would be to only call getStoredToken when actually making the request in a similar pattern.

[debater-coder]

Also, because getStoredToken is asynchronous, it causes a more critical race condition, where the fetch wrapper attempts to fetch before getStoredToken has even had a chance to return. I found workarounds for the first two issues, but this is more tricky as there is no way to access the promise. I will be back with a repro.

[debater-coder]

I'm closing this issue as it's entirely my fault and doing this in the top level in different modules is a bad idea anyway. I'll create a new issue for the other problem as it does seem to be a real bug.