Add doc.subscribe('status', callback) function
#828
Conversation
WalkthroughThe recent update introduces functionality to subscribe to document status changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Document
participant Client
User->>+Document: subscribe('status')
Document-->>User: Acknowledgement
Client->>+Document: attach()
Document-->>User: StatusChangedEvent (attached)
Client->>+Document: detach()
Document-->>User: StatusChangedEvent (detached)
Client->>+Document: deactivate()
Document-->>User: StatusChangedEvent (deactivated)
Client->>+Document: attach()
Document-->>User: StatusChangedEvent (attached)
Client->>+Document: remove()
Document-->>User: StatusChangedEvent (removed)
Assessment against linked issues
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #828 +/- ##
==========================================
+ Coverage 80.61% 80.64% +0.03%
==========================================
Files 60 60
Lines 4565 4573 +8
Branches 932 934 +2
==========================================
+ Hits 3680 3688 +8
Misses 616 616
Partials 269 269 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (2)
src/document/document.ts (2)
Line range hint
416-416: Avoid usinganytype for better type safety.- export type Indexable = Record<string, any>; + export type Indexable = Record<string, unknown>; // Consider using `unknown` for more strict type checking - public getCloneRoot(): CRDTObject | undefined { + public getCloneRoot(): CRDTObject | null { // Consider using `null` for explicit absence of value - public getUndoStackForTest(): Array<Array<HistoryOperation<P>>> { - public getRedoStackForTest(): Array<Array<HistoryOperation<P>>> { + public getUndoStackForTest(): Array<Array<HistoryOperation<P>> | null> { + public getRedoStackForTest(): Array<Array<HistoryOperation<P>> | null> {Also applies to: 418-418, 494-494
Line range hint
636-636: Remove non-null assertions and handle potential null values properly.- this.clone!.root + this.clone?.root ?? throw new Error("Clone root is undefined"); - this.clone!.presences.get(actorID)! + this.clone?.presences.get(actorID) ?? throw new Error("Presence for actorID is undefined"); - this.clone!.presences.get(this.changeID.getActorID())! + this.clone?.presences.get(this.changeID.getActorID()) ?? throw new Error("Presence for current actorID is undefined"); - this.getPresence(actorID)! + this.getPresence(actorID) ?? throw new Error("Presence for actorID is undefined"); - this.clone!.root.getObject() + this.clone?.root.getObject() ?? throw new Error("Object from clone root is undefined"); - this.clone!.presences.get(this.changeID.getActorID())! + this.clone?.presences.get(this.changeID.getActorID()) ?? throw new Error("Presence for current actorID is undefined"); - this.presences.get(clientID)! + this.presences.get(clientID) ?? throw new Error("Presence for clientID is undefined"); - this.getPresence(publisher)! + this.getPresence(publisher) ?? throw new Error("Presence for publisher is undefined"); - this.clone!.presences.get(this.changeID.getActorID())! + this.clone?.presences.get(this.changeID.getActorID()) ?? throw new Error("Presence for current actorID is undefined"); - this.clone!.presences.get(this.changeID.getActorID())! + this.clone?.presences.get(this.changeID.getActorID()) ?? throw new Error("Presence for current actorID is undefined"); - this.clone!.root + this.clone?.root ?? throw new Error("Clone root is undefined"); - this.clone!.root + this.clone?.root ?? throw new Error("Clone root is undefined"); - this.clone!.presences.get(this.changeID.getActorID())! + this.clone?.presences.get(this.changeID.getActorID()) ?? throw new Error("Presence for current actorID is undefined"); - this.clone!.presences.get(this.changeID.getActorID())! + this.clone?.presences.get(this.changeID.getActorID()) ?? throw new Error("Presence for current actorID is undefined"); - this.clone!.root + this.clone?.root ?? throw new Error("Clone root is undefined");Also applies to: 637-637, 644-644, 648-648, 656-656, 656-656, 718-718, 1075-1075, 1198-1198, 1199-1199, 1201-1201, 1242-1242, 1264-1264, 1335-1335, 1335-1335, 1340-1340
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/document/document.ts (3 hunks)
- test/integration/document_test.ts (2 hunks)
Additional Context Used
Biome (40)
src/document/document.ts (20)
399-399: Unexpected any. Specify a different type.
416-416: Unexpected any. Specify a different type.
418-418: Unexpected any. Specify a different type.
494-494: Unexpected any. Specify a different type.
636-636: Forbidden non-null assertion.
637-637: Forbidden non-null assertion.
644-644: Forbidden non-null assertion.
648-648: Forbidden non-null assertion.
656-656: Forbidden non-null assertion.
656-656: Forbidden non-null assertion.
718-718: Forbidden non-null assertion.
1075-1075: Forbidden non-null assertion.
1198-1198: Forbidden non-null assertion.
1199-1199: Forbidden non-null assertion.
1201-1201: Forbidden non-null assertion.
1242-1242: Forbidden non-null assertion.
1264-1264: Forbidden non-null assertion.
1335-1335: Forbidden non-null assertion.
1335-1335: Forbidden non-null assertion.
1340-1340: Forbidden non-null assertion.
test/integration/document_test.ts (20)
31-31: The computed expression can be simplified without the use of a string literal.
43-43: The computed expression can be simplified without the use of a string literal.
44-44: The computed expression can be simplified without the use of a string literal.
30-63: This function expression can be turned into an arrow function.
65-113: This function expression can be turned into an arrow function.
122-122: Forbidden non-null assertion.
123-123: Forbidden non-null assertion.
115-201: This function expression can be turned into an arrow function.
225-225: The computed expression can be simplified without the use of a string literal.
203-239: This function expression can be turned into an arrow function.
302-303: Avoid the delete operator which can impact performance.
334-334: Forbidden non-null assertion.
335-335: Forbidden non-null assertion.
336-336: Forbidden non-null assertion.
336-336: Forbidden non-null assertion.
336-336: Forbidden non-null assertion.
241-373: This function expression can be turned into an arrow function.
374-460: This function expression can be turned into an arrow function.
461-552: This function expression can be turned into an arrow function.
568-568: The computed expression can be simplified without the use of a string literal.
Additional comments not posted (1)
src/document/document.ts (1)
Line range hint
782-946: The implementation of thesubscribemethod for the 'status' event type is correctly aligned with the PR's objectives to handle document status changes.
chacha912
force-pushed
the
doc-subscribe-status
branch
from
fc699a0 to
f28885a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (3)
test/integration/document_test.ts (1)
20-20: Add documentation forStatusChangedEvent.It's good practice to include documentation comments for exported types and interfaces to improve code readability and maintainability.
src/document/document.ts (2)
Line range hint
636-636: Address the use of non-null assertions.The use of non-null assertions (!) should be avoided where possible as they can lead to runtime errors if the assumptions about non-nullability prove incorrect. Consider adding appropriate checks or refactor the code to ensure that the values are indeed non-null before using them.
Also applies to: 637-637, 644-644, 648-648, 656-656, 656-656, 718-718, 1075-1075, 1198-1198, 1199-1199, 1201-1201, 1242-1242, 1264-1264, 1335-1335, 1335-1335, 1340-1340
Line range hint
399-399: Specify more precise types instead ofany.Using
anytype can lead to potential type safety issues and bugs. It's recommended to replaceanywith more specific types to leverage TypeScript's type system fully. Here are some suggested changes:- type Indexable = Record<string, any>; + type Indexable = Record<string, unknown>;This change replaces
anywithunknown, which is safer because it forces you to perform some type of checking before performing operations on the values.Also applies to: 416-416, 418-418, 494-494
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/document/document.ts (3 hunks)
- test/integration/document_test.ts (2 hunks)
Additional Context Used
Biome (40)
src/document/document.ts (20)
399-399: Unexpected any. Specify a different type.
416-416: Unexpected any. Specify a different type.
418-418: Unexpected any. Specify a different type.
494-494: Unexpected any. Specify a different type.
636-636: Forbidden non-null assertion.
637-637: Forbidden non-null assertion.
644-644: Forbidden non-null assertion.
648-648: Forbidden non-null assertion.
656-656: Forbidden non-null assertion.
656-656: Forbidden non-null assertion.
718-718: Forbidden non-null assertion.
1075-1075: Forbidden non-null assertion.
1198-1198: Forbidden non-null assertion.
1199-1199: Forbidden non-null assertion.
1201-1201: Forbidden non-null assertion.
1242-1242: Forbidden non-null assertion.
1264-1264: Forbidden non-null assertion.
1335-1335: Forbidden non-null assertion.
1335-1335: Forbidden non-null assertion.
1340-1340: Forbidden non-null assertion.
test/integration/document_test.ts (20)
31-31: The computed expression can be simplified without the use of a string literal.
43-43: The computed expression can be simplified without the use of a string literal.
44-44: The computed expression can be simplified without the use of a string literal.
30-63: This function expression can be turned into an arrow function.
65-113: This function expression can be turned into an arrow function.
122-122: Forbidden non-null assertion.
123-123: Forbidden non-null assertion.
115-205: This function expression can be turned into an arrow function.
229-229: The computed expression can be simplified without the use of a string literal.
207-243: This function expression can be turned into an arrow function.
306-307: Avoid the delete operator which can impact performance.
338-338: Forbidden non-null assertion.
339-339: Forbidden non-null assertion.
340-340: Forbidden non-null assertion.
340-340: Forbidden non-null assertion.
340-340: Forbidden non-null assertion.
245-377: This function expression can be turned into an arrow function.
378-464: This function expression can be turned into an arrow function.
465-556: This function expression can be turned into an arrow function.
572-572: The computed expression can be simplified without the use of a string literal.
Additional comments not posted (2)
src/document/document.ts (2)
782-791: Ensure proper handling of the new 'status' event type in thesubscribemethod.The implementation of the new
subscribemethod for the 'status' event type appears to be correct and aligns with the existing patterns for other event types in theDocumentclass. This method allows users to register callbacks that are triggered when the document status changes, which is a key feature added by this PR.Also applies to: 932-945
387-387: Refine the type definition forDocEventCallbackMap.- type DocEventCallbackMap<P extends Indexable> = { + type DocEventCallbackMap<P extends Indexable = Indexable> = {This change ensures that the type parameter
Phas a default ofIndexable, which can help avoid type errors in cases where the generic type isn't explicitly provided.Likely invalid or redundant comment.
…nto doc-subscribe-status
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/document/document.ts (3 hunks)
Additional comments not posted (1)
src/document/document.ts (1)
Line range hint
322-330: TheStatusChangedEventinterface is well-defined and correctly includes all necessary properties for handling status changes. This will ensure that subscribers receive detailed and useful event data.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
test/integration/document_test.ts (2)
Line range hint
30-63: Consider refactoring all function expressions in this file to arrow functions for better consistency and to ensure the lexical binding ofthiswhere needed.- it('Can attach/detach documents', async function ({ task }) { + it('Can attach/detach documents', async ({Apply this pattern to all other test cases in this file.
Also applies to: 65-113, 201-237, 239-371, 372-458, 459-550, 551-587, 588-640, 641-673, 674-712, 713-748, 749-782, 796-861, 863-895, 897-931, 934-955, 956-988, 990-1036
Line range hint
300-301: Avoid using thedeleteoperator as it can lead to performance issues. Consider setting the property toundefinedinstead if removal of the property is not strictly necessary.- delete root.obj.food; + root.obj.food = undefined;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/client/client.ts (2 hunks)
- test/helper/helper.ts (1 hunks)
- test/integration/document_test.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- test/helper/helper.ts
Additional context used
Biome
test/integration/document_test.ts
[error] 30-63: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 65-113: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 115-199: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 201-237: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 300-301: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 239-371: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 372-458: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 459-550: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 551-587: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 588-640: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 641-673: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 674-712: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 713-748: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 749-782: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 796-861: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 863-895: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 897-931: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 934-955: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 956-988: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 990-1036: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/integration/document_test.ts (2 hunks)
Files not summarized due to errors (1)
- test/integration/document_test.ts: Error: Server error. Please try again later.
Additional context used
Biome
test/integration/document_test.ts
[error] 30-63: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 65-113: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 115-151: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 214-215: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 153-285: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 286-372: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 373-464: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 465-501: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 502-554: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 555-587: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 588-626: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 627-662: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 663-696: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 710-777: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 779-863: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 865-919: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 921-953: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 955-989: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 992-1013: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 1014-1046: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
Additional comments not posted (2)
test/integration/document_test.ts (2)
772-855: The test case logic is thorough and correctly tests document status changes.This test case effectively simulates various document status changes and uses assertions to verify that the correct events are collected, aligning well with the new functionality introduced in the PR.
853-854: Proper cleanup with unsubscribe calls.The unsubscribe calls are appropriately placed to ensure that event listeners are cleaned up after the test, preventing potential memory leaks or unintended behavior.
…nto doc-subscribe-status
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/integration/document_test.ts (2 hunks)
Additional context used
Biome
test/integration/document_test.ts
[error] 208-209: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (4)
test/integration/document_test.ts (4)
765-765: Convert function expression to arrow function for consistency.Arrow functions are more concise and lexically bind the
thisvalue, which is generally preferred in TypeScript, especially in test suites where callbacks are common.- it('Can subscribe to events related to document status changes', async function ({ + it('Can subscribe to events related to document status changes', async ({
765-765: Add documentation comment for the test case.It's recommended to add a documentation comment for the test case to explain what it tests and why it's important. This improves readability and maintainability.
/** * Test case for subscribing to document status changes. * Verifies the handling of various document states, including attachment, detachment, removal, deactivation, and syncing. */
864-864: Convert function expression to arrow function for consistency.Arrow functions are more concise and lexically bind the
thisvalue, which is generally preferred in TypeScript, especially in test suites where callbacks are common.- it('Should properly handle removed document when deactivating', async function ({ + it('Should properly handle removed document when deactivating', async ({
864-864: Add documentation comment for the test case.It's recommended to add a documentation comment for the test case to explain what it tests and why it's important. This improves readability and maintainability.
/** * Test case for handling removed documents when deactivating. * Verifies the handling of document states, including attachment, removal, and deactivation. */
| // 3. When c2 deactivates, it should also receive a removed event. | ||
| await c2.deactivate(); | ||
| // NOTE(chacha912): For now, document status changes to `Detached` when deactivating. | ||
| // This behavior may change in the future. |
There was a problem hiding this comment.
@chacha912
Would you explain your plan about this comment?
Until 0.4.25-rc of Yorkie Android sdk, it doesn't update status to detached when deactivating.
I found it with yorkie-js-sdk's test code, and I'll add a commit for fix it. (But PR is not yet. I'll make it after apply 0.4.27~28.)
I made a consensus about this with @7hong13
But, I found this comment now, I want to check this how it will be changed future.
There was a problem hiding this comment.
@maruldy At that time, there was a possibility of reattaching a document after detaching it, and it hadn't been decided whether deactivate should perform detach. So I left a NOTE.
If it's later decided that deactivate should include the same process of detaching the document, I considered that when c2 deactivates, it would receive the change of c1 removing the document, resulting in the final document status changing to removed.
Currently, when deactivating, only deactivateInternal is performed internally, changing all document statuses to detached, so the final document status changes to detached.
There was a problem hiding this comment.
I think this thread is resolved via below conversation. Thanks.
| unsub4(); | ||
| }); | ||
|
|
||
| it('Should properly handle removed document when deactivating', async function ({ |
There was a problem hiding this comment.
- Would you please explain the meaning of “properly handling”?
- And it seems that part of validation is written in duplicate with the TC above. is it intended?
There was a problem hiding this comment.
- Would you please explain the meaning of “properly handling”?
It means that when another peer deactivates a deleted document, the status events occur in the order verified in the test.
- And it seems that part of validation is written in duplicate with the TC above. is it intended?
The event verification is duplicated when first attaching and removing to show which events occur in order. Since the important part is number 3, it might be sufficient to verify only that part.
There was a problem hiding this comment.
Thanks for your reply. I want some more conversation for clearly understanding.
-
is peer means client object(e.g. c1 or c2)?
-
According to your explanation, this tc doesn't need verification with two clients and documents. If this tc's purpose is verification with
c2.deactivate(), i looks like only needed for c2 and d2. So I think it's enough with just one client and document. Would you please explain what is the point to use c1 and d1?
There was a problem hiding this comment.
Thank you for your question and for seeking clarification. Let me provide more details:
- The term
peeris used to refer to a client other than oneself. - We need c1 and c2 because this test is about a scenario where I deactivate, but another person has removed the document.
When a client (c1) removes a document, the document's state becomes "removed," and the connection to the server is terminated.
Now, we need to verify how another client (c2) should handle receiving the Change that the document has been removed. This can be examined in two scenarios:
-
When c2 sends a sync request (note that detach behaves similarly as it also receives the change that the document was removed):
- This is addressed in the test above:
// 6. When c2 syncs, it should also receive a removed event.
- This is addressed in the test above:
-
When c2 deactivates without syncing (or detaching):
- This is covered in the current test:
// 3. When c2 deactivates, it should also receive a removed event. - For now, in the case of only deactivating, since c2 doesn't receive the Change that the document was removed, its state changes to "detached."
- This is covered in the current test:
There was a problem hiding this comment.
Thanks for your very helpful comment.
I can almost understand after this comment why this tc was written.
I think this is the point that confused me, I didn't understand why step 2 is needed. It seems like step 2 is created to verifying for removed, but with the current specification, step 3 is checking for detached, and the test succeeds without step 2. (I tested this on my android sdk's test code)
This tc seems to be that contains part of the future spec(number2) and part of the current spec(number3) at the same time, right?
There was a problem hiding this comment.
@maruldy I'm glad my comment was helpful.
Step 2(//2. When c1 removes a document, it receives a removed event.) is a separate verification from step 3. It's not a verification of the future spec, but rather a test to verify that d1 receives a "removed" event when c1 removes the document.
In the case of the future spec, as noted, after c2 deactivates in step 3, d2 should receive a "removed" status event instead of "detached".
From the previous answer,
When a client (c1) removes a document, the document's state becomes "removed,".
This is what step 2 verifies. (d1 event)
When c2 deactivates without syncing (or detaching):
This is what step 3 verifies. (d2 event)
Since our focus is on d2, it might be sufficient to verify events only for d2 without verifying events for d1.
There was a problem hiding this comment.
@chacha912
Thanks for your reply.
Now I understand why this TC is contains step 2 and 3.
It's to validate that after calling c1.remove(), the detached event is still being received, and if the spec changes in the future to allow remove to be received, you'll want to modify this TC again then, right?
There was a problem hiding this comment.
@maruldy Yes, this test (step 3) checks if a 'detached' event occurs when c2 deactivates. And a comment is added to indicate that we might need to modify this test later if we change how things work.
There was a problem hiding this comment.
Thanks for long conversation. :) I'll replicate this tc to android sdk. 👍
What this PR does / why we need it?
This PR introduces the
doc.subscribe('status')function, allowing users to subscribe to changes in the document status and receive the StatusChangedEvent event.Any background context you want to provide?
What are the relevant tickets?
Fixes #827
Addresses yorkie-team/yorkie#928
Checklist
Summary by CodeRabbit
New Features
'status'in documents, allowing users to subscribe to and handle status change events.Bug Fixes
Tests