Feature/api_client

[s-pic]

Type of change

• New feature (non-breaking change which adds functionality)

change summary

Prepares to solve #https://github.com/FixMyBerlin/fixmy.platform/issues/271 → This just adds the api module to do http requests against our backend and makes it subject to further review. We can than gradually replace de-duped code in pages/Map/apiservice.js, pages/User/apiservice.js and pages/Reports/apiservice.js

motivation and context

We finally want to solve https://github.com/FixMyBerlin/fixmy.platform/issues/57 → Once we have a central api client and a central place to throw api-related errors, we can work on the error handling in the view layer.

updates to dependencies and project setup

• mocks ky with ky-universal in unit tests so that required browser APIs get polyfilled using node-fetch 39fa7eb
• updates fetch-mock. Only after that update, response bodies could be inspeced with fetch-mocks lastOptions()
• load dotenv during jest setup
• updates ky

How Has This Been Tested?

Run npm run test to run new unit tests.

Aspects marked for discussion

Errors classes:

• What kind of errors we want to differentiate in the client. Do we want to handle 401s, 404s etc. specifically?
• api docs for the backend to get a grasp on how responses are structured throughout the api. I just looked at some endpoints in the reports feature

token handling

• I figured we can read the current token from the store before every request and attach it as header.
  This has been implemented using a beforeRequest hook offered by ky
• If we handle the current token like so, we don't need to connect our components to the store just in order to pass the token down to some component so that this component can pass it to an apiService
• A negative aspect is that the apiService knows about our store, a dependency injection would be the cleaner pattern, but then we would need either multiple instances of the apiService or a Singleton. I considered the dependency to me fine since we will propably share the service between different apps.
• If we keep the token handling as proposed, we could consider not storing it in the redux store at all.

What are the proposed next steps?

Iteratively integrate this, this should be another issue.
Here already some ideas what should happen:

• make all ajax calls against our backend run over the new module
• remove token from component state

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works

[cafca]

@s-pic I have made a number of changes in this PR. I will list only the major changes here:

• export all the error classes from index.ts to enable this pattern:

try {
  await api.request(url)
} catch(err) {
  if (err instanceof api.ApiError) {
    // handle api error
  } else {
    // handle other error
  }
}

• renamed some of the modules and symbols to clarify their responsibilties
• flatten the syntax of the options argument to request by including kyOptions in the main options parameter. I know you mentioned that you think there are good reasons for doing this so maybe we should talk about it. I think the more concise syntax is worth any future namespace clashes if that was your concern, but maybe there's something else I'm not seeing.
• add an onSlowResponse callback that is triggered if requests don't complete within 5 seconds
• add docstrings
• added debug logging in the fmc:api namespace. I think this will come in very handy for debugging in the future.

I have made a comment above about the NetworkError class, which I don't like as it is now. We should find a way to fix it.

[s-pic]

Thanks for your remarks, I will get back to it asap.

[s-pic]

• flatten the syntax of the options argument to request by including kyOptions in the main options parameter. I know you mentioned that you think there are good reasons for doing this so maybe we should talk about it. I think the more concise syntax is worth any future namespace clashes if that was your concern, but maybe there's something else I'm not seeing.

Jup, my concern was to to mix up our config props with those for ky.
I see your point and I can live with the change 😃.

I have made a comment above about the NetworkError class, which I don't like as it is now. We should find a way to fix it.

I will take another shot at it.