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

Refactor client API packages #16847

Merged
merged 34 commits into from
Oct 24, 2023

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Oct 13, 2023

Closes #16841

  • Move the current src/schema package into src/api/schema
  • Move some API-related queries to their own modules in src/api/{module} for easier discoverability and reusability.
    • Some APIs don't need to be moved as they are unlikely to be generally used across the client codebase like:
      • api/storage: lives under client/src/components/User/DiskUsage/Management/services.ts
  • Try to consolidate naming conventions:
    • Use the word fetch to explicitly convey that it will trigger a server request as opposed to simply get that may or may not trigger a request (for example when getting the cached values from a store). Examples:
      • When we define a fetcher for an endpoint like GET api/whatever we can name the fetcher as whateverFetcher:
        export const whateverFetcher = fetcher.path("/api/whatever").method("get").create();
      • When we require a custom fetcher with some extra parameters or response handling we can use as function name fetchWhatever:
        interface MagicalItem { ... }
        
        const whateverFetcher = fetcher.path("/api/whatever").method("get").create();
        
        export async function fetchMagicalWhatever(extraThing = "something"): Promise<MagicalItem[]> {
            const { data } = await whateverFetcher({ use_magic: true, extra_thing: extraThing });
            return data as MagicalItem[];
        }
  • Include a small best practices guide under client/docs/querying-the-api.md

How to test the changes?

  • This is a refactoring of components with existing test coverage.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@davelopez davelopez added kind/refactoring cleanup or refactoring of existing code, no functional changes area/client and removed area/documentation area/admin labels Oct 13, 2023
@davelopez davelopez force-pushed the refactor_client_api_modules branch from cd17116 to c11907a Compare October 13, 2023 08:40
@jmchilton
Copy link
Member

I definitely agree a best practice and greater consistency in the code around these packages would be good. Thank you for working on this.

reusability

I would argue that building reusable components in part means targeting specific, clean parts of the API that can likewise be decomposed. Centralizing the modules feels like a step backwards in terms of isolation which I think is a key aspect of reusabilty. A component that targets our license API or the TRS APIs would be great and could be used outside the context of Galaxy readily I would think given that these APIs are isolated well and highly structured. Shipping the component together with its API interaction layer would seem to be the right way to handle that - but maybe there are frontend idiosyncrasies that make it substantially different than the backend.

I'm not opposed to these changes at all - this comment doesn't need a response and shouldn't block the PR anyway. Just wanted to say my peace on isolation and convey my lessons learned on working on the backend components with a similar eye toward reusability.

@davelopez
Copy link
Contributor Author

@jmchilton I appreciate the comment! That is a good point, I sometimes struggle with how to organize big code bases, I really want to come up with a solution that can be considered best practice and feels consistent across the entire project and I'm totally willing to adapt to alternatives.

My line of thought was... I would like to query the API for some particular object in a component, what do I do?

  • Do I look for existing queries that return what I need? For that, we need a somewhat centralized place like this src/api.
  • Create my own queries (even if they exist elsewhere)? It is probably not a lot of duplication, but some parameters and responses for queries might require a bit of massaging that needs to be duplicated.
  • Use a composable (or a Pinia store) and relay to it the "querying"? I guess this would be similar to the centralized case and we might not always be required to store query results.

The whole idea of this was to actually come up with something that we are all happy with and we can all agree on, so I'm glad you commented and I'm more than happy to rewrite it :)
Maybe just focusing on naming things consistently would be enough. We use sometimes the term "service" and in other cases "queries".

@davelopez davelopez force-pushed the refactor_client_api_modules branch from c56ce84 to cbf20e9 Compare October 13, 2023 15:11
@mvdbeek
Copy link
Member

mvdbeek commented Oct 13, 2023

I understood

Move API-related queries to their own modules in src/api/{module} for easier discoverability and reusability.

to be just that ? The schema portion is pure interfaces, my understanding is that once you push bundles (which are JS so don't contain the interfaces) those would be gone. And when you publish individual components those get pulled apart as well (so you don't get the things you're not using).

@davelopez
Copy link
Contributor Author

That is also a good point @mvdbeek, I'm no expert on that, but when the final bundle is done is supposed to have only what you actually imported as you mentioned, that is why we try to import individual functions instead of whole modules (I learned the lesson by doing the importing wrongly often but was catch in reviews 😆)

@mvdbeek
Copy link
Member

mvdbeek commented Oct 13, 2023

I should qualify this is my belief on how things should magically work (TM) with no foundation in the real world.

@davelopez davelopez force-pushed the refactor_client_api_modules branch from cbf20e9 to fded7e1 Compare October 13, 2023 15:43
@davelopez
Copy link
Contributor Author

Talking about magic... let's ask chatGPT for fun... 😆

Question:

In a big Vue project where I use Typescript and Webpack, what is the best practice when I create a new component that requires calling API endpoints?

Do I add a module directory at the top level where I put different modules for different API parts and then import the functions that I need?
Or is it better to create a module beside the component with the queries that I need?

Response:

When working on a large Vue project using TypeScript and Webpack and you need to create a new component that requires calling API endpoints, there are a few different approaches you can take, and the best practice may depend on the specific needs and structure of your project. Here are two common approaches:

Separate Modules for API Calls (Recommended):

Create a dedicated directory, such as api, at the top level of your project structure.
Inside the api directory, you can organize separate modules for different parts of your API, based on the resources or functionalities they represent. For example, you might have userApi.ts, productApi.ts, orderApi.ts, etc.
Each module should export functions or classes responsible for making API calls and handling the responses for a specific part of your API.
In your component, import and use the appropriate functions or classes from the relevant API module.
This approach keeps your API calls organized and modular, making it easier to maintain and test.
Example project structure:

/src
  /components
    /YourComponent
      YourComponent.vue
  /api
    userApi.ts
    productApi.ts
    orderApi.ts

Inline API Calls in Component:

Alternatively, you can create API calls directly within the component that needs them. In this case, you can define functions or methods within the component that handle API requests.
While this approach is more straightforward for small projects or simple components, it can make your components larger and harder to maintain as your project grows.
It can also lead to code duplication if multiple components require the same API calls.
Example project structure:

/src
  /components
    /YourComponent
      YourComponent.vue

In general, the first approach of separating API calls into dedicated modules is recommended for large projects. It promotes code organization, reusability, and maintainability. It also makes it easier to manage authentication, error handling, and other common concerns related to API interactions.

However, there may be situations where you need to make one-off API calls in a component, and it's acceptable to define them directly within the component. The key is to strike a balance between organization and efficiency based on the specific needs of your project and components.

@davelopez davelopez force-pushed the refactor_client_api_modules branch 4 times, most recently from a856ae9 to cebebca Compare October 20, 2023 11:12
@davelopez davelopez force-pushed the refactor_client_api_modules branch from cebebca to 9913b71 Compare October 23, 2023 16:13
@davelopez
Copy link
Contributor Author

davelopez commented Oct 23, 2023

I've included a draft for a best practices guide in client/docs/querying-the-api.md, please don't hesitate to comment with your concerns and ideas for improvement so we can create a set of best practices that we all are comfortable with :)

I'm happy to drop or rewrite as many changes as needed until we get something we want to maintain.

@davelopez
Copy link
Contributor Author

Look like test failures are unrelated. Taking it out of draft for the first wave of reviews :)

@davelopez davelopez marked this pull request as ready for review October 24, 2023 08:20
@github-actions github-actions bot added this to the 23.2 milestone Oct 24, 2023
@dannon dannon merged commit 42e244b into galaxyproject:dev Oct 24, 2023
47 of 50 checks passed
@davelopez davelopez deleted the refactor_client_api_modules branch October 24, 2023 16:50
@davelopez
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move API related logic in client to api package
4 participants