-
Notifications
You must be signed in to change notification settings - Fork 5
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
Allow specifying a topic when subscribing to a document #105
Conversation
* Subscribes to events on the document with the specific [targetPath]. | ||
*/ | ||
public fun events(targetPath: String): SharedFlow<Event> { | ||
return targetEventStreams.getOrElse(targetPath) { |
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.
I’m not sure if this is the common way of dealing shared flow, such as returning shared flow from a function or storing them in a map.
I’ll refactor it if it seems awkward or if there could be potential issues.
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.
We can use getOrPut
to skip manually setting the new value at the end, but I don't think we need targetEventStreams
, as sharing a SharedFlow instance for each path seems unnecessary.
Just returning a filtered Flow without eagerly sharing should be enough.
*/ | ||
public sealed class OperationInfo { | ||
|
||
public abstract var path: String |
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.
I defined path
as a mutable property because making it read-only seemed to result in clumsy boilerplate code for updating its value.
but I’m open to refactoring it if a better solution exists.
|
||
public abstract var path: String | ||
|
||
internal var executedAt: TimeTicket = TimeTicket.InitialTimeTicket |
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.
js implementation link
I changed the property name from element
to executedAt
becauaseelement
sounds more appropriate for something like CrdtElement
or JsonElement
, while executedAt
has been used as the property name for TimeTicket
until now.
(cc. @chacha912 )
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.
JS-SDK has been updated to remove InternalOpInfo
. Could you please review the changes and make the necessary updates accordingly?
yorkie-team/yorkie-js-sdk#566
* fix realTimeAttachments() * adjust timeout limit
Codecov Report
@@ Coverage Diff @@
## main #105 +/- ##
===========================================
+ Coverage 0 78.57% +78.57%
- Complexity 0 545 +545
===========================================
Files 0 53 +53
Lines 0 2619 +2619
Branches 0 357 +357
===========================================
+ Hits 0 2058 +2058
- Misses 0 367 +367
- Partials 0 194 +194
|
What this PR does / why we need it?
you can read the full description for these tasks from the following link.
Any background context you want to provide?
some previously written instrumentation tests are failing occasionally. I'll fix them in a separate PR.
What are the relevant tickets?
related to yorkie-team/yorkie-js-sdk#487, yorkie-team/yorkie-js-sdk#445
Checklist