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

Data fetching on client-side directly from GitHub API #325

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

AlexxNica
Copy link
Collaborator

Working on getting better loading times for roadmaps with suggestions from #33.
This is still a draft and it needs some cleanup and syncing before moving to ready for review.

Changes

  • Decoupled functions from our API to a new module we can use from the client-side.
  • The new module doesn't require GitHub API tokens and instead relies on the client's rate limiting for public GitHub API calls.
    • In the future we can have a fail-safe mode to fetch roadmaps using other methods if clients hit their limit, although we should be cautious due to possible abuse.
  • Calling GitHub APIs directly from the client without having to jump through our endpoints is expected to improve loading times and operational overhead by a lot, I still need to do some testing but seems like a good assumption.

@AlexxNica AlexxNica self-assigned this Feb 17, 2023
@vercel
Copy link

vercel bot commented Feb 17, 2023

@AlexxNica is attempting to deploy a commit to the ipfs-ignite Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Feb 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
starmaps ❌ Failed (Inspect) Feb 17, 2023 at 4:26AM (UTC)

@whizzzkid
Copy link
Collaborator

Some thoughts around following your approach on how it works and improves performance:

  • the public access limits are not published by github. This article suggests the calls are rate limited to 60req/hour. The bacalhau roadmap sends 63 requests and it can grow larger, so one load is going to exhaust that limit and it would need to fallback to the implemented proxy api anyways, traversing through the roadmap would need not be able to call public apis and we’ll spend time waiting for the calls to fail before falling back.
  • We’ll also need to compute responses on the client side instead of server side, that’s ok but just another cost to think about.
  • Caching responses in service worker won’t be reliable either because responses from other origins would be marked as opaque responses which would would not allow us to compare the network responses and cached responses, which means every update is basically new update. We’ll also might approach the service worker storage limit, because each opaque response would be counted as 7.5mb response, so 63 calls, = 472.5mb of storage for one roadmap.

@AlexxNica
Copy link
Collaborator Author

Preliminary tests with cache disabled

Before

perf-test_before-annotated

After

perf-test_after-annotated

@AlexxNica AlexxNica changed the title Improve loading times (#33) Data fetching on client-side directly from GitHub API Feb 21, 2023
@AlexxNica
Copy link
Collaborator Author

Caching responses in service worker won’t be reliable either because responses from other origins would be marked as opaque responses which would would not allow us to compare the network responses and cached responses, which means every update is basically new update. We’ll also might approach the service worker storage limit, because each opaque response would be counted as 7.5mb response, so 63 calls, = 472.5mb of storage for one roadmap.

@whizzzkid are there other ways to cache responses from the client without the downside of having to store them as opaque responses?

What do you think of using standard HTTP caching mixed with the service worker in the background to check for changes/updates?

We could use conditional requests to check if the content has changed before calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants