openverse-frontend
openverse-frontend copied to clipboard
Replace `api-service` concept with Nuxt context-stored Axios instance
Problem
See https://github.com/WordPress/openverse-frontend/pull/1569/files#r933336938 for a description of the problem and a suggested solution. The discussion is copied here for posterity:
I think isVersioned here assumes that the api service being created will only be used for a particular set of resources or requests rather than all requests. Because we create multiple axios clients for different api service usages, we pass isVersioned when we want the URLs to automatically include the v1 string.
However, I also disagree with this change. @dhruvkb can you explain the benefit of this implementation over explicitly including the version string at the request site? If we introduce new versions of existing endpoints or have api services used for mixed version requests in the same cycle then this implementation will either have to be reverted back to the explicit version or be significantly increase the complexity of this code.
I prefer explicit request URLs personally, they are easier to search for in the app and make zero assumptions about the future or current versions of API endpoints that will or are being used.
- @sarayourfriend
Well, I think undoing this refactor is actually more trouble than it is worth and will complicate this PR. The existing "versioned API" approach we took was effectively the same and had the same bad assumptions (that v1 is the only version and that all requests performed by a given API service instance will all reference the same version of endpoint).
I personally find the current approach with the getResourceSlug function over-complicated, sneaky, and bakes assumptions into the base api-service.ts module that are not necessarily true.
Either the API service instances are only for making requests to the media endpoints (and we should use a plain axios instance for anything else) or we have to reconsider how the API service is used in the first place.
I think it's potentially an anti-pattern, the way it's currently implemented, and we might want to consider stuffing it into Nuxt context, either by hand-spinning our own module or using the existing nuxt/axios module.
This would have some significant benefits:
- We could insert the plugin after the API token request plugin so that it can automatically include the access token on the server. This allows downstream requesters to completely ignore the api token detail. Currently all api service instantiators have to consider the api token. This duplicates the work and complicates the downstream code.
- We can eliminate the complexity in the api-service module completely. The only issue that remains is the mapping of the media type to the endpoint names, but this can easily be put into a function or a static mapping for reference by requesters to construct API urls.
I'll open an issue for this. For now I think this should remain as is with @dhruvkb's refactors because it keeps the existing assumptions, but makes them ever so slightly more flexible to accommodate non-versioned API requests.
- @sarayourfriend
Prompted by @krysal's question which uncovered the assumption.
Basically it boils down to the fact that I think the api-service concept no longer serves us and in fact overcomplicates and bakes in incorrect assumptions about how API versioning works or can be used.
I am skeptical that we need an API service at all. If there are abstractions that can exist to simplify certain request areas, they can be baked into the areas that make those requests instead of into the loose service class model that doesn't suite a JavaScript request/response lifecycle very well (considering we don't have a DI container or anything else like that to make easy instantiation, dependency management, and further abstractions clean).
Description
Remove the api-service abstraction and replace it with the typical Nuxt plugin axios instance that has been appropriately instantiated with an API token when needed. Use request interceptors to do other transformations that might be helpful for downstream usages of the axios instance (like trailing slash enforcement).
Alternatives
Keep the current api service concept but untangle versioning from it entirely.
I don't think this is a good approach because it preserves the complexity of every single requester having to know about and manage the API token.
Implementation
- [ ] 🙋 I would be interested in implementing this feature.
An additional benefit to this change is that we would be able to actually use Sentry to catch Axios errors more easily. Right now we log and we throw some errors that are essentially meaningless and destroy all useful debugging context: https://github.com/WordPress/openverse-frontend/issues/1604
If we use a Nuxt context-contained Axios instance with appropriate helper functions attached, then we can make it more convenient to correctly handle errors in a way that preserves their context. While Nuxt context feels annoying and cumbersome, it will also unlock the ability for us to inject things like request trace IDs that we could even forward to the API and track a user session (without revealing any PII) from the initial SSR load, to client interactions and even up to API requests.