Skip to content

Conversation

2color
Copy link
Member

@2color 2color commented Nov 7, 2024

Title

Fix #148.

By default, caching is enabled with TTL as specified in https://specs.ipfs.tech/routing/http-routing-v1/#response-headers

Description

  • feat: add request deduplication
  • test: add test for request deduplication
  • chore: remove console .log
  • feat: use cache api to cache responses
  • feat: disable cache if cacheTtl is 0

Notes & open questions

  • How to make sure we run tests in browser with an echo server in node.js

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@2color 2color requested a review from a team as a code owner November 7, 2024 12:12
@2color 2color changed the title feat: deduplicate identical concurrent requests and cache feat: add request deduplication for identical requests and response caching Nov 7, 2024
@2color 2color changed the title feat: add request deduplication for identical requests and response caching feat: add request deduplication and caching Nov 7, 2024
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor changes needed, and a question

*
* ### Caching
*
* By default, the client caches responses in browser environments for a duration of 5 minutes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change this to more accurately state that we set the x-cache-expires header on the response which only works in environments that respsect it (i.e. browsers)

Copy link
Member Author

@2color 2color Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-cache-expires is just an arbitrary name. The browser doesn't inspect it.

Instead, we inspect it when reading from the cache:

https://github.com/ipfs/helia-delegated-routing-v1-http-api/blob/1acaab8f90d7d40224f79294524a413bd104f6c1/packages/client/src/client.ts/#L388

I've refined the wording to reflect this more accurately.

@2color 2color requested a review from SgtPooki November 8, 2024 12:05
@2color 2color changed the title feat: add request deduplication and caching feat: add request deduplication and caching in client Nov 8, 2024
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@2color 2color requested a review from achingbrain November 11, 2024 07:36

// Clear the cache when stopping
if (this.cache != null) {
void this.cache.delete('delegated-routing-v1-cache')
Copy link
Member Author

@2color 2color Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realised that this can be a problem if you have multiple instances of the client in scope as we do in Verified Fetch.

But even then, it's most likely that they all get stopped at the same time rather than individually.

@2color 2color merged commit 98105ae into main Nov 11, 2024
20 checks passed
@2color 2color deleted the deduplicate branch November 11, 2024 09:33
github-actions bot pushed a commit that referenced this pull request Nov 11, 2024
## [@helia/delegated-routing-v1-http-api-client-v4.2.0](https://github.com/ipfs/helia-delegated-routing-v1-http-api/compare/@helia/delegated-routing-v1-http-api-client-4.1.2...@helia/delegated-routing-v1-http-api-client-4.2.0) (2024-11-11)

### Features

* request deduplication and caching in client ([#151](#151)) ([98105ae](98105ae)), closes [#148](#148)
Copy link

🎉 This PR is included in version @helia/delegated-routing-v1-http-api-client-v4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Helia makes duplicate (delegated) routing calls
2 participants