Replaced axios with fetch (FF-1976)

[leoromanovsky]

Motivation and Context

1️⃣ the axios package cannot be supported in chrome extensions v3 because it is based on APIs that do not exist within a service worker. dogfooding eppo's js sdk within an v3 extension throws this error:

http-client.js:32 Uncaught (in promise) Error: There is no suitable adapter to dispatch the request since :
- adapter xhr is not supported by the environment
- adapter http is not available in the build
    at HttpClient.handleHttpError (http-client.js:32:1)
    at HttpClient.get (http-client.js:25:1)
    at async ExperimentConfigurationRequestor.fetchAndStoreConfigurations (experiment-configura…n-requestor.js:10:1)
    at async Object.start (poller.js:22:1)
    at async EppoJSClient.fetchFlagConfigurations (eppo-client.js:52:1)
    at async init (index.js:90:1)
    at async index.js:12:1
Show 2 more frames

2️⃣ axios is a heavy weight package and is something like 50% of our asset size. removing it will improve our customers' applications

Description

• replace axios with fetch - supporting a base url and timeouts
• adds IHttpClient interface and implementing FetchHttpClient

➜  js-client-sdk-common git:(lr/ff-1976/axios-fetch) yarn why axios
yarn why v1.22.22
[1/4] 🤔  Why do we have the module "axios"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
error We couldn't find a match!
✨  Done in 0.11s.

Decisions

Since the HttpClient is instantiated inside the EppoClient, instead of being injected in, I have no choice but to continue mocking the http API. Changing from xhr-mock to mocking fetch itself. In the future if we decide passing the http client is useful then we can mock that object directly.

👉 Could use a set of eyes on the beforeAlls in the unit tests - everything passes but something feels a little off to me.

How has this been tested?

[leoromanovsky]

I was finding that this variable seemed to be impacted by other tests - there is a bit of spaghetti here that I would love to unpack so things can run in a more isolated fashion.

[aarsilv]

Interesting. Yeah test interdependency is not good.

[leoromanovsky]

the default exports in nodejs are kind of funny to me; noticed this wasn't renamed.

[aarsilv]

Nice work! 👏 This is a great modernization.

I agree the mocking got a bit all over. I think basically we separate that test into separate describe blocks for when we want the unobfuscated UFC, obfuscated UFC, or per-test. The first two just need to make one mock for all tests returning the appropriate UFC. The latter can not mock anything in a beforeAll(), leaving it up to the tests, but resetting in an afterEach() or afterAll(). I think that would clean things up.

Also, this would be a good time to sneak in changing the default for throwing error on initialization.

[aarsilv]

Jest runs each file in it's own process so I don't think it's necessary to clean this up. But you may want to, just in case. Also not necessary, but consider putting it in beforeAll() just to cluster that one-time stuff together.

beforeAll(async () => {
  const original fetch = fetch;
  global.fetch = jest.fn(() => {
    ...
   });   
  await init(storage);
});

afterAll(()=> {
  global.fetch = originalFetch;
})

Update: I see you do it later on. I think you could just ax this one entirely and just save off and restore fetch() at the top level. Then have two nested describes for unobfuscated and obfuscated responses, where you mock fetch()

[schmit]

🚀