Skip to content
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

Merged
merged 8 commits into from
Jun 19, 2023

Conversation

7hong13
Copy link
Contributor

@7hong13 7hong13 commented Jun 15, 2023

What this PR does / why we need it?

  • provide a new API that allows users to subscribe to the document with a specific target path.
  • improve the event value of Local/RemoteChange.

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

  • Added relevant tests or not required
  • Didn't break anything

@7hong13 7hong13 added the enhancement 🌟 New feature or request label Jun 15, 2023
@7hong13 7hong13 self-assigned this Jun 15, 2023
* Subscribes to events on the document with the specific [targetPath].
*/
public fun events(targetPath: String): SharedFlow<Event> {
return targetEventStreams.getOrElse(targetPath) {
Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 TimeTicketuntil now.
(cc. @chacha912 )

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

@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #105 (8de3704) into main (12baa6e) will increase coverage by 78.57%.
The diff coverage is 55.75%.

@@             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     
Impacted Files Coverage Δ
.../main/kotlin/dev/yorkie/document/json/JsonArray.kt 53.42% <0.00%> (ø)
...ain/kotlin/dev/yorkie/document/json/JsonElement.kt 88.88% <ø> (ø)
...n/kotlin/dev/yorkie/document/json/JsonPrimitive.kt 46.66% <ø> (ø)
...lin/dev/yorkie/document/operation/MoveOperation.kt 31.57% <0.00%> (ø)
.../kotlin/dev/yorkie/document/operation/Operation.kt 33.33% <ø> (ø)
...n/dev/yorkie/document/operation/SelectOperation.kt 33.33% <0.00%> (ø)
...ie/src/main/kotlin/dev/yorkie/document/Document.kt 66.66% <30.30%> (ø)
yorkie/src/main/kotlin/dev/yorkie/core/Client.kt 77.62% <40.00%> (ø)
...n/dev/yorkie/document/operation/RemoveOperation.kt 73.68% <62.50%> (ø)
...tlin/dev/yorkie/document/operation/AddOperation.kt 78.94% <66.66%> (ø)
... and 9 more

... and 34 files with indirect coverage changes

@7hong13 7hong13 merged commit 707e3dc into main Jun 19, 2023
@7hong13 7hong13 deleted the document_subscribe branch June 19, 2023 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants