-
Notifications
You must be signed in to change notification settings - Fork 57
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
@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
packages/agent/src/prototyping/clients/dwn-server-info-cache-level.ts
Outdated
Show resolved
Hide resolved
packages/agent/src/prototyping/clients/dwn-server-info-cache-level.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
DwnServerInfo
HTTP client to get info from thedwn-server
's/info
endpointThis is helpful for retrieving registration requirements and whether the server supports sockets.
It uses a
TTLCache
memory cache as the currently implemented and default, however additional caches can be easily added if necessary.