@web5/agent Adding DwnServerInfo to RPC Clients#489
Conversation
🦋 Changeset detectedLatest commit: 1fbb6b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
DwnServerInfo to RPC Clients@web5/agent Adding DwnServerInfo to RPC Clients
|
TBDocs Report ✅ No errors or warnings @web5/api
@web5/crypto
@web5/crypto-aws-kms
@web5/dids
@web5/credentials
TBDocs Report Updated at 2024-05-02T21:24:04Z |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
==========================================
+ Coverage 90.81% 91.03% +0.21%
==========================================
Files 116 116
Lines 29442 29526 +84
Branches 2156 2174 +18
==========================================
+ Hits 26739 26878 +139
+ Misses 2668 2613 -55
Partials 35 35
|
b2116ea to
9a402a0
Compare
9a402a0 to
4461f5b
Compare
shamilovtim
left a comment
There was a problem hiding this comment.
Could the two caches somehow reuse the MemoryStore or LevelStore interfaces? It seems like we've written that same API a handful of time already in web5 SDK
2536cb4 to
1fbb6b5
Compare
|
@shamilovtim good call outs on the unnecessary caching implementations. We can add ones if there is a need but I think a memory TTL cache is all that we currently need.
The Let me know what you think about the approach and I'll create an issue. |
thehenrytsai
left a comment
There was a problem hiding this comment.
Looks good! Couple of comments.
| it('returns undefined after ttl', async function () { | ||
| // skip this test in the browser, sinon fake timers don't seem to work here | ||
| // with a an await setTimeout in the test, it passes. | ||
| if (!isNode) { |
There was a problem hiding this comment.
Sanity: this would be the first precedence that it doesn't work in browser right? Have you tried passing number instead of string to tickAsync()?
There was a problem hiding this comment.
It's really strange that it doesn't work. I just tried a number instead of string and get the same behavior because I didn't think to try that before. But still fails in the browsers.
| ttl?: string; | ||
| } | ||
|
|
||
| export class DwnServerInfoCacheMemory implements DwnServerInfoCache { |
There was a problem hiding this comment.
Consider turning this into generic, seems low hanging fruit.
| export class DwnServerInfoCacheMemory implements DwnServerInfoCache { | |
| export class MemoryCache<T> implements GenericCache<T> { |
There was a problem hiding this comment.
The thing about this is we already have one called MemoryStore which is why his is more specific. Maybe MemoryStoreAsync? Unsure
There was a problem hiding this comment.
@thehenrytsai Yeah @shamilovtim called out something similar and I was considering this, but figured we could do a separate PR to add TTLCacheAsync or something of that sort to @web5/common because we are doing this same wrapper in a couple of places to conform with an async interface.
DwnServerInfoHTTP client to get info from thedwn-server's/infoendpointThis is helpful for retrieving registration requirements and whether the server supports sockets.
It uses a
TTLCachememory cache as the currently implemented and default, however additional caches can be easily added if necessary.