Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace api-service concept with Nuxt context-stored Axios instance #530

Open
1 task
sarayourfriend opened this issue Jul 29, 2022 · 2 comments
Open
1 task
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: frontend Related to the Nuxt frontend

Comments

@sarayourfriend
Copy link
Collaborator

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.

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:

  1. 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.
  2. 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.

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.
@sarayourfriend sarayourfriend added 🟩 priority: low Low priority and doesn't need to be rushed 💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Jul 29, 2022
@sarayourfriend
Copy link
Collaborator Author

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: WordPress/openverse-frontend#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.

@obulat obulat transferred this issue from WordPress/openverse-frontend Feb 22, 2023
@obulat obulat added the 🧱 stack: frontend Related to the Nuxt frontend label Feb 22, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Openverse Backlog Feb 23, 2023
@sarayourfriend
Copy link
Collaborator Author

Conflicts with #592

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: frontend Related to the Nuxt frontend
Projects
Status: 📋 Backlog
Development

No branches or pull requests

2 participants