search

elixir-cloud-aai / cloud-components

Reusable components for the ELIXIR Cloud
https://elixir-cloud-components.vercel.app
Apache License 2.0
9 stars 14 forks source link

Multiple API Calls to toolClasses Endpoint on Initial Load #279

Closed Abyl10 closed 5 months ago

Abyl10 commented 6 months ago

Description

Upon initial load of the ecc-client-elixir-trs-filer component, the same API request to the https://trs-filer-test.rahtiapp.fi/ga4gh/trs/v2/toolClasses endpoint is being sent multiple times. This issue is observed in both the trs-classes and trs-list components, leading to unnecessary network traffic and potential performance degradation.

Steps to Reproduce

  1. Navigate to the component's page.
  2. Monitor the network traffic using the browser's developer tools.
  3. Observe that the GET request to https://trs-filer-test.rahtiapp.fi/ga4gh/trs/v2/toolClasses is made multiple times (exactly 3 times) on the initial load.

Expected Behavior

Possible solution(s)

A potential solution to this issue could be to centralize the API call within the ecc-trs-filer.ts file and then pass the fetched data as props to both the trs-classes and trs-list components. This approach would ensure that the API call is made once, and the necessary data is distributed efficiently to the components that require it.

Additional context

This issue may lead to increased load times and unnecessary strain on the server due to the duplicated requests. Addressing it could improve the overall performance and user experience of the application.

My contribution

I plan to address this issue and will be working on a fix. Once I have a solution, I will submit a pull request for review. This approach will ensure that the API call is optimized, reducing unnecessary network traffic and improving the application's performance.

upd: here is pull request https://github.com/elixir-cloud-aai/cloud-components/pull/280 I have no permission to link it.

JaeAeich commented 6 months ago

@Abyl10 thanks for the issue but we are depricating trs and filer package in the next PR. I don't think it makes sense fixing these issues are the code will be migrated and majorly rewritten in lit.

@uniqueg @git-anurag-hub @SalihuDickson ?

uniqueg commented 6 months ago

I suppose that is correct. However, given that @Abyl10 already provided a PR for the issue, maybe it still makes sense to merge it - if you think it will make the migration a little easier/straightforward, or at least doesn't harm the migration?

SalihuDickson commented 6 months ago

@uniqueg Yh, if we merge it without verifying that it works as it's supposed to. But since we're planning a full rewrite, it doesn't seem like a proper use of limited time, reviewing the functionality of the PR.

uniqueg commented 5 months ago

I guess that makes sense.

Apologies, @Abyl10: we still very much appreciate your initiative and hope this doesn't discourage you from further contributions :)