-
Notifications
You must be signed in to change notification settings - Fork 110
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
feat: add has
and readAll
methods to StorageAdapter
interface
#366
Conversation
These methods will be used mainly by the chat-members and the broadcast plugins
Codecov ReportBase: 36.70% // Head: 36.65% // Decreases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
- Coverage 36.70% 36.65% -0.05%
==========================================
Files 18 18
Lines 4588 4602 +14
Branches 166 166
==========================================
+ Hits 1684 1687 +3
- Misses 2902 2913 +11
Partials 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
src/convenience/session.ts
Outdated
/** | ||
* Lists all keys | ||
*/ | ||
readAll?: () => Iterable<T> | AsyncIterable<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.
It does not list all the keys. But perhaps we should consider doing this instead. I think only retrieving values without the corresponding keys is rarely useful, so I see three alternatives here:
- Provide
readAllKeys
only - Provide
readAllKeys
andreadAllEntries
which returns[key, value]
pairs - Provide
readAllKeys
andreadAllEntries
andreadAllValues
Also, we have to think about readAll
from the current memory sessions. Should we align the name in the adapter for 2.0 (deprecate until then) or rather align the name in the interface to readAll
?
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 does not list all the keys. But perhaps we should consider doing this instead. I think only retrieving values without the corresponding keys is rarely useful, so I see three alternatives here:
You're right.
I haven't thought about the implementation of the broadcast plugin enough to be able to tell if we'll need keys, entries, or values, so I'd say we go with 3. to be future proof.
Should we align the name in the adapter for 2.0 (deprecate until then)
I'm assuming by this you mean we add the three new methods and have readAll
call readValues
(current behavior), then remove it on 2.0, right? If so, I think this is the way to go
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.
Yes, sounds good then!
@Satont I'm very curious what your opinion is on this |
I'll wait for this before I move on |
I'm wondering about implementation of something like that. Already expect to "crutches". I'm interested about use case such methods, can you explain a little more about what this is for?? |
Initially, for the |
Ok. Do you plan implement all those methods in storages? |
I can try to, yes. But the idea of havin the optional properties is so we don't necessarily have to do that. |
Ok, then go on. |
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.
LGTM.
Please implement these changes for the built-in memory sessions. Then we can merge.
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.
One last question. When using the new storage adapter, do we ever need to differentiate between sync and async iterables? If yes, how could this be done?
Co-authored-by: KnorpelSenf <[email protected]>
We shouldn't need to, because sync iterables can be consumed with |
Awesome, that's what I was hoping for! |
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.
LGTM, thanks!
These methods will be used mainly by the chat-members and the broadcast
plugins