Implement API Interface

[ducku]

Creates APIInterface class, components that uses API calls now creates an instance of the class.

Future alternative solutions to API calls should be implemented in this class.

Closes #377

[ducku]

Not too sure why "renders with error when api call to server throws" is failing

I'm rethrowing caught errors in the class, and errors seem to be displayed correctly when I test manually.

[adamnovak]

This looks like it should work and is a big improvement, but I think it's not factored quite the way it needs to be.

It shouldn't be breaking the tests, though; I will see if I can figure out what is going wrong there.

[adamnovak]

I don't think this belongs in components, since it isn't a React component. It could just go under src next to fetchAndParse.js.

[adamnovak]

There should be some documentation for the class and what it is for.

[adamnovak]

Also, if this is an Official Object-Oriented Programming Interface™, then we should have an interface that just defines the empty methods and has all the documentation for what they take and what they return, and a separate class that implements the interface by inheriting from the first class and having the code to make the web requests, like export class ServerAPI extends APIInterface. All the React component code would only talk about the interface, and never mention the specific implementation.

Then I can write another implementation of the interface that uses local computation (sometimes) instead of remote web requests, and it will be drop-in compatible because both implementations implement the same interface. Barbara Liskov will smile upon us and everything will Just Work without me having to make any more changes to the React components.

[adamnovak]

The constructor arguments should be documented. What kind of thing exactly does cancelSignal need to be? Can it ever be null if I don't need to cancel requests?

Also, if we use one cancelSignal for the whole object, there's no way to cancel an individual request.

[rmccrear]

The purpose of cancelSignal was to allow the component to cancel a fetch request if the component unmounts. (This was done to prevent the tests from giving warnings in the logs.)

The cancelSignal was needed in the fetch call's argument, and the fetchCanceler was called by the component in componentWillUnmount See: https://julietonyekaoha.medium.com/react-cancel-all-axios-request-in-componentwillunmount-e5b2c978c071 or https://gist.github.com/aderbas/187c3ed23db7d6ece9748a5858a8f0d5

Since there will now be an object associated with the fetch calls, you could probably remove the cancelSignal from APIInterface's constructor argument and just create fetchCanceler and cancelSignal inside the constructor of APIInterface. Then have APIInterface expose an abort() method for componentWillUnmount() to call. This would simplify the function signature to one argument.

As for the need to cancel individual requests, I don't think that's necessary. If the whole component is unmounted, all requests are cancelled.

[rmccrear]

If it's decided to implement one global APIInterface as per
#381 (comment) , then you could keep the fetchCanceler and the cancelSignal in the component and pass the cancelSignal into each of the APIInterface methods as arguments. (It's OK to pass null or undefined if you don't need to cancel it.)

You'd only really need to cancel if you are fetching in componentDidMount and want to avoid warnings if the component unmounts before the fetch call returns.

[adamnovak]

Each of these should have dome documentation for what kind of thing they take and what kind of thing they return (which is usually a parsed object in some kind of format).

We would like to have nice full documentation for the format of each reply, but that might be a lot of work to write, so maybe we should just say something like how it has to match the format of whatever API endpoint's response and it is basically a graph or a list of files or whatever, depending on the method.

[adamnovak]

I don't think these try/catch setups need to be here, if there's not going to be any error handling. They seem to be no-ops, except depending on the browser they can lose the original stack trace for the error.

[adamnovak]

It looks like now we're making a fresh APIInterface in every React component that needs to talk to the API, and the components still take a string apiUrl as a prop.

This lays some of the groundwork we need, but when we come up with another implementation of APIInterface that isn't URL-based (APIInterface2 or whatever), how are we going to plug it into the components?

I think what we should do is make the components all take the APIInterface instance itself as a prop, instead of taking a URL and making an APIInterface in each component. Then we can have one APIInterface for the whole application, and we can control where to get the data based on what one we send in from the top.

[adamnovak]

Also this file might really be supposed to have a .mjs extension, since it is an ES6 module.

[adamnovak]

@ducku I think the test is failing because of the exciting way we try to mock fetchAndParse for all the React components:

import * as fetchAndParseModule from "./fetchAndParse";
...
jest.mock("./fetchAndParse");

beforeEach(() => {
  jest.resetAllMocks();
});
...
it("renders with error when api call to server throws", async () => {
  fetchAndParseModule.fetchAndParse = () => {
    throw new Error("Mock Server Error");
  };

It seems like we intend this to reach into the actual module and change the value of that function for everyone else who has also imported the same module, in e.g. the React components. This doesn't seem like it is supposed to work, and I don't think calling jest.mock() on the module is really going to solve that. It looks a lot like this deprecated approach that people say works on some transpiling setups and not others. I think it can't work if the import statements are truly implemented correctly.

I think we might be able to make replacing the function work if we do it as an argument to jest.mock(), because Jest will find that call and actually run it before doing the imports when it runs the test file, and it will make a fake mocked file to import instead of the real one. But then maybe we can only install one mock?

But once we get the APIInterface set up so that you can pass it down from the top, we won't need any mocks any more at all. We can just implement the interface with an implementation that throws, and pass that down to test the response of the application to exceptions on API calls.

[adamnovak]

But it looks like the real problem is that we're expecting the error to display synchronously, while really it is being detected and rendered in in the background and so we need to have code to wait for it to show up. I'm not sure why that worked before.

[adamnovak]

OK, I think I fixed the test. None of the mock-replacing stuff was actually required; I'm not entirely sure I want to keep it, since it's ugly. But maybe once the replaceable-API design is done we can drop the whole mocking exercise.

The real problem was hoping the error would render synchronously, which it doesn't seem to promise to do.

[adamnovak]

I think I lied about how the canceling needs to wrok. @rmccrear made a good point in #381 (comment) that if unmounting a component needs to cancel that component's requests, and if we use one API client object for the whole app, then we can't just use a cancel method on the client since it would cancel everybody's requests.

So we probably need per-call AbortController inputs, or else something like an API subscription object that we can make from the client to make and cancel requests, without canceling all requests.

But since the TubeMapContainer and the HeaderForm probably never unmount, I'm not sure the problem I am worrying about will arise in practice. Unless maybe you open the demos while a request is in flight?

[adamnovak]

This looks good to me!

[adamnovak]

@ducku It looks like I missed that actually, even with this, we're still sending the raw apiUrl into HeaderForm:

sequenceTubeMap/src/App.js

Line 189 in 854896f

apiUrl={this.props.apiUrl}

Which means it still talks to the API in ways other than by using the APIClient (for example, to do file uploads).

So I think #377 is really not done; we need to make all communication go through the APIClient.