Introduce broadcast API for event sharing #884
Conversation
WalkthroughThe recent changes introduce a broadcasting feature in the Changes
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/sdk/src/client/client.ts (2 hunks)
- packages/sdk/src/document/document.ts (9 hunks)
- packages/sdk/src/util/validator.ts (1 hunks)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Additional comments not posted (8)
packages/sdk/src/util/validator.ts (1)
20-30: LGTM! ThevalidateSerializablefunction is well-implemented.The function correctly checks if a value is serializable using
JSON.stringifyand handles exceptions appropriately.packages/sdk/src/client/client.ts (1)
591-639: LGTM! Thebroadcastmethod is well-implemented.The method includes necessary checks for client activation, document attachment, and payload serializability. Error handling and logging are appropriately managed.
Run the following script to verify the usage of the
broadcastmethod in the codebase:Verification successful
Verification Complete:
broadcastMethod UsageThe
broadcastmethod is implemented correctly and is tested for error handling with unserializable payloads. No other direct usages were found in the codebase, indicating its usage is limited or indirect. The review comment accurately reflects the method's implementation.
- The method is tested in
packages/sdk/test/integration/client_test.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `broadcast` match the expected usage. # Test: Search for the function usage. Expect: Correct usage of the `broadcast` method. rg --type javascript -A 5 $'broadcast'Length of output: 72
Script:
#!/bin/bash # Description: Verify all function calls to `broadcast` match the expected usage. # Corrected command to search for the function usage. rg 'broadcast' --glob '*.ts' -A 5Length of output: 7095
packages/sdk/test/integration/client_test.ts (1)
867-885: LGTM! The test case for broadcasting unserializable payloads is well-implemented.The test case effectively verifies that the
broadcastmethod throws an error when provided with an unserializable payload.packages/sdk/src/document/document.ts (5)
178-180: LGTM! The addition of theBroadcastevent type is well-integrated.This enhancement allows for broadcast messaging capabilities within the document's event system.
380-383: LGTM! TheBroadcastEventinterface is well-defined.The interface correctly extends
BaseDocEventand includes the necessary properties for broadcast events.
603-611: LGTM! ThebroadcastEventHandlersmap is well-initialized.This map effectively stores handlers for broadcast events, indexed by their topic.
1650-1656: LGTM! The handling ofBroadcastevents inapplyDocEventis well-implemented.This ensures that broadcast events are processed and the appropriate handlers are invoked.
1052-1079: LGTM! ThesubscribeBroadcastEventmethod is well-implemented.This method allows clients to register callbacks for broadcast events and manage subscriptions effectively.
Run the following script to verify the usage of the
Broadcastevent type in the codebase:Verification successful
Broadcast Event Handling Verified
The usage of
DocEventType.Broadcastin the codebase is consistent and correctly implemented for handling broadcast events. ThesubscribeBroadcastEventmethod and related code appropriately manage event subscriptions and callbacks. No issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the `Broadcast` event type. # Test: Search for the `Broadcast` event type usage. Expect: Correct implementation and handling. rg --type javascript -A 5 $'DocEventType.Broadcast'Length of output: 85
Script:
#!/bin/bash # Description: Verify all usages of the `Broadcast` event type. # Test: Search for the `Broadcast` event type usage. Expect: Correct implementation and handling. rg -A 5 'DocEventType.Broadcast'Length of output: 1652
|
There are a few things I'd like to share: 1. How Handlers for Each Topic Are Managed: Inside the Document class, I've added a // document.ts
export class Document<T, P extends Indexable = Indexable> {
// skipped
private broadcastEventHandlers: Map<
string,
(topic: string, payload: any) => void
>;
}When a user subscribes to a specific topic for broadcast events: // User subscribes to a broadcast event with the topic "TOPIC_NAME"
doc.subscribeBroadcastEvent('TOPIC_NAME', (topic, payload) => {
// Handle the broadcast event for the specified topic
});The handler is registered in the broadcastEventHandlers map: export class Document<T, P extends Indexable = Indexable> {
public subscribeBroadcastEvent(
topic: string,
handler: (topic: string, payload: any) => void,
error?: ErrorFn,
): Unsubscribe {
// Register the handler in broadcastEventHandlers
this.broadcastEventHandlers.set(topic, handler);2. How Within the public applyWatchStream(resp: WatchDocumentResponse) {
// skipped
} else if (type === PbDocEventType.DOCUMENT_BROADCAST) {
if (resp.body.value.body) {
const { topic, payload } = resp.body.value.body;
const decoder = new TextDecoder();
event.push({
type: DocEventType.Broadcast,
value: { topic, payload: JSON.parse(decoder.decode(payload)) },
});
}
}
if (event.length > 0) {
this.publish(event);
}
}
}3. Test Case for Sending Unserializable Payloads: As discussed earlier, I've added a test case in Lastly, I've verified that the broadcast API works as expected in several examples, but I believe further testing is necessary to ensure complete coverage. |
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)
- packages/sdk/src/document/document.ts (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/document/document.ts
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)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/sdk/test/integration/client_test.ts
|
@gwbaik9717 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/test/integration/client_test.ts (1 hunks)
- packages/sdk/test/integration/integration_helper.ts (2 hunks)
Additional comments not posted (6)
packages/sdk/test/integration/integration_helper.ts (1)
24-24: LGTM! Verify the usage ofwithTwoClientsAndDocuments.The introduction of the
syncModeparameter enhances flexibility. Ensure that all calls towithTwoClientsAndDocumentsare updated to use the new parameter if necessary.Run the following script to verify the function usage:
Also applies to: 35-36
packages/sdk/test/integration/client_test.ts (5)
867-884: LGTM! Test for serializable payload broadcasting.The test case for broadcasting a serializable payload is well-implemented and ensures that no errors are thrown.
886-904: LGTM! Test for error handling with unserializable payloads.The test case correctly verifies that broadcasting an unserializable payload throws an error.
907-932: LGTM! Test for triggering subscribed broadcast event handlers.The test case effectively verifies that a subscribed handler is triggered for broadcast events.
934-959: LGTM! Test for unsubscribed broadcast event handlers.The test case correctly ensures that handlers for unsubscribed events are not triggered.
961-991: LGTM! Test for unsubscribing from broadcast events.The test case accurately verifies that handlers are not triggered after unsubscribing.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/sdk/src/client/attachment.ts (1 hunks)
- packages/sdk/src/client/client.ts (4 hunks)
- packages/sdk/src/document/document.ts (16 hunks)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Additional context used
GitHub Check: build (18.x)
packages/sdk/src/document/document.ts
[failure] 629-629:
Don't usenullas a type. Use undefined instead of null
[failure] 1341-1341:
Missing JSDoc comment
[failure] 1341-1341:
Don't usenullas a type. Use undefined instead of null
[failure] 2090-2090:
Unnecessary try/catch wrapper
Biome
packages/sdk/src/document/document.ts
[error] 2093-2093: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
Additional comments not posted (17)
packages/sdk/src/client/attachment.ts (1)
111-116: LGTM!The function is correctly implemented.
The code changes are approved.
packages/sdk/src/client/client.ts (1)
589-640: LGTM!The function is well-implemented with proper validation and error handling.
The code changes are approved.
packages/sdk/test/integration/client_test.ts (5)
867-882: LGTM!The test case is correctly implemented and covers the expected behavior.
The code changes are approved.
884-902: LGTM!The test case is correctly implemented and covers the expected behavior.
The code changes are approved.
905-933: LGTM!The test case is correctly implemented and covers the expected behavior.
The code changes are approved.
935-963: LGTM!The test case is correctly implemented and covers the expected behavior.
The code changes are approved.
965-998: LGTM!The test case is correctly implemented and covers the expected behavior.
The code changes are approved.
packages/sdk/src/document/document.ts (10)
177-181: LGTM!The addition of the
Broadcastevent type is appropriate and aligns with the new broadcast functionality.The code changes are approved.
200-201: LGTM!The update to include
BroadcastEventin theDocEventtype is necessary and correctly implemented.The code changes are approved.
381-384: LGTM!The
BroadcastEventinterface is well-defined and aligns with the requirements for broadcasting events.The code changes are approved.
403-403: LGTM!The addition of the
broadcastcallback inDocEventCallbackMapis necessary and correctly implemented.The code changes are approved.
565-568: LGTM!The
BroadcastSubscribePairtype is well-defined and aligns with the requirements for subscribing to broadcast events.The code changes are approved.
620-627: LGTM!The addition of
broadcastEventHandlersis necessary and correctly implemented to manage handlers for broadcast events.The code changes are approved.
658-659: LGTM!The initialization of
broadcastEventHandlersin the constructor is necessary and correctly implemented.The code changes are approved.
862-873: LGTM!The new overload of the
subscribemethod is necessary and correctly implemented to support subscribing to broadcast events.The code changes are approved.
Line range hint
1563-1600: LGTM!The update to handle broadcast events in the
applyWatchStreammethod is necessary and correctly implemented.The code changes are approved.
1691-1697: LGTM!The update to handle broadcast events in the
applyDocEventmethod is necessary and correctly implemented.The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/src/client/attachment.ts (1 hunks)
- packages/sdk/src/document/document.ts (16 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/sdk/src/client/attachment.ts
- packages/sdk/src/document/document.ts
gwbaik9717
force-pushed
the
broadcast-api
branch
from
84e8762
to
4d516be
Compare
|
Here is an update. 1. Unified Broadcast Event Subscription Users can now use the existing type SubscribePair = {
type: string;
};Building on this, the type BroadcastSubscribePair = {
type: 'broadcast';
topic: string;
} & SubscribePair;Here’s an example of how to subscribe to a broadcast event: const unsubscribe = d2.subscribe(
{ type: 'broadcast', topic: "TOPIC_NAME" },
(topic, payload) => {},
);2. Call Broadcasting is now handled directly by the To support this, I’ve added a export class Document<T, P extends Indexable = Indexable> {
// skipped
private client?: Client;
/**
* `setClient` sets the client of this document.
*
* @internal
*/
public setClient(client?: Client): void {
this.client = client;
}
}Here’s an example of how to broadcast a topic with payload. // must be serializable
const payload = {a:1, b:"2"}
await doc.broadcast("TOPIC_NAME", payload); |
|
Sorry for the late review. I'll check it today. CC) @chacha912, @gwbaik9717 |
There was a problem hiding this comment.
@gwbaik9717 Thank you for the PR, and I apologize for the delayed review. I've left a few comments for your consideration.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/sdk/src/client/attachment.ts (1 hunks)
- packages/sdk/src/document/document.ts (16 hunks)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/sdk/src/client/attachment.ts
- packages/sdk/src/document/document.ts
Additional comments not posted (5)
packages/sdk/test/integration/client_test.ts (5)
867-882: LGTM!The test case correctly verifies that the
broadcastmethod can handle a valid payload.The code changes are approved.
884-902: LGTM!The test case correctly verifies the error handling of the
broadcastmethod when given an unserializable payload.The code changes are approved.
905-928: LGTM!The test case correctly verifies that a subscribed broadcast event handler is triggered correctly.
The code changes are approved.
930-958: LGTM!The test case correctly verifies that an unsubscribed event handler does not trigger.
The code changes are approved.
960-993: LGTM!The test case correctly verifies that a handler does not trigger after unsubscribing.
The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/sdk/src/document/document.ts (1)
2059-2070: Consider using a more specific type for the payload.The
broadcastmethod is well-defined and aligns with the new broadcast functionality. However, the payload type is currently set toany. It would be better to use a more specific type that represents JSON-serializable data.Consider using a type like
Record<string, any>or a custom type that represents JSON-serializable data.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/src/document/document.ts (14 hunks)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Additional comments not posted (12)
packages/sdk/test/integration/client_test.ts (6)
867-882: LGTM!The test correctly verifies that the
broadcastmethod can handle a valid payload.The code changes are approved.
884-902: LGTM!The test correctly verifies the error handling of the
broadcastmethod when given an unserializable payload.The code changes are approved.
905-928: LGTM!The test correctly verifies that a subscribed broadcast event handler is triggered correctly.
The code changes are approved.
930-958: LGTM!The test correctly verifies that an unsubscribed event handler does not trigger.
The code changes are approved.
960-993: LGTM!The test correctly verifies that a handler does not trigger after unsubscribing.
The code changes are approved.
905-993: LGTM!The test correctly verifies that each request is handled one by one.
The code changes are approved.
packages/sdk/src/document/document.ts (6)
177-181: LGTM!The addition of the
Broadcastevent type is consistent with the new broadcast functionality.The code changes are approved.
200-201: LGTM!The update to the
DocEventtype to includeBroadcastEventis consistent with the new broadcast functionality.The code changes are approved.
381-384: LGTM!The
BroadcastEventinterface is well-defined and aligns with the new broadcast functionality.The code changes are approved.
403-403: LGTM!The update to the
DocEventCallbackMaptype to include abroadcastcallback is consistent with the new broadcast functionality.The code changes are approved.
555-568: LGTM!The
BroadcastSubscribePairtype is well-defined and aligns with the new broadcast functionality.The code changes are approved.
851-859: LGTM!The enhancement to the
subscribemethod to allow clients to register callbacks for broadcast events is consistent with the new broadcast functionality.The code changes are approved.
|
@hackerwins, I've reviewed, but could you please check it once more in case I missed anything? (Please check the comments I've left with cc regarding the subscribe interface and client property) |
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)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Additional comments not posted (5)
packages/sdk/test/integration/client_test.ts (5)
867-882: LGTM!The test case correctly verifies that a serializable payload can be successfully broadcast.
The code changes are approved.
884-902: LGTM!The test case correctly verifies that an error is thrown when broadcasting an unserializable payload.
The code changes are approved.
904-927: LGTM!The test case correctly verifies that the handler is triggered for a subscribed broadcast event.
The code changes are approved.
929-957: LGTM!The test case correctly verifies that the handler is not triggered for an unsubscribed broadcast event.
The code changes are approved.
959-990: LGTM!The test case correctly verifies that the handler is not triggered for a broadcast event after unsubscribing.
The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/src/document/document.ts (13 hunks)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/document/document.ts
Additional comments not posted (5)
packages/sdk/test/integration/client_test.ts (5)
867-882: LGTM!The test case correctly verifies that broadcasting a serializable payload does not throw any errors.
The code changes are approved.
884-902: LGTM!The test case correctly verifies that broadcasting an unserializable payload throws the expected error.
The code changes are approved.
904-929: LGTM!The test case correctly verifies that the handler is triggered for a subscribed broadcast event.
The code changes are approved.
931-962: LGTM!The test case correctly verifies that the handler is not triggered for an unsubscribed broadcast event.
The code changes are approved.
964-997: LGTM!The test case correctly verifies that the handler is not triggered for a broadcast event after unsubscribing.
The code changes are approved.
Refactor to have Client subscribe to Document's broadcast events and handle them by calling Client's broadcast method
This commit handles the case when broadcast event fails such as when trying to broadcast unserializable payload
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
packages/sdk/src/document/document.ts (2)
380-384: Consider using a more specific type for thepayloadproperty.The
BroadcastEventinterface is well-defined and aligns with the new broadcast functionality. However, thepayloadtype is set toany, which could be improved for better type safety. Consider using a more specific type that represents JSON-serializable data.
2023-2034: Consider using a more specific type for thepayloadparameter.The
broadcastmethod is well-implemented and aligns with the new broadcast functionality. However, thepayloadtype is set toany, which could be improved for better type safety. Consider using a more specific type that represents JSON-serializable data.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/sdk/src/client/attachment.ts (2 hunks)
- packages/sdk/src/client/client.ts (5 hunks)
- packages/sdk/src/document/document.ts (10 hunks)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Additional comments not posted (15)
packages/sdk/src/client/attachment.ts (2)
3-3: LGTM!The import statement for
Unsubscribeis correct and necessary.
25-26: LGTM!The addition of the
unsubscribeBroadcastEventproperty and the updated constructor are correctly implemented and enhance the class's functionality.Also applies to: 32-39
packages/sdk/src/client/client.ts (4)
45-45: LGTM!The import statement for
validateSerializableis correct and necessary.
307-318: LGTM!The addition of the
unsubscribeBroacastEventparameter in theattachmethod is correctly implemented and necessary for managing broadcast event subscriptions.Also applies to: 345-345
601-652: LGTM!The addition of the
broadcastmethod is correctly implemented and enhances theClientclass's functionality by allowing the broadcasting of payloads to specified topics.
818-818: LGTM!The call to
unsubscribeBroadcastEventin thedetachInternalmethod is correctly implemented and necessary for managing broadcast event subscriptions.packages/sdk/test/integration/client_test.ts (5)
867-882: LGTM!The test case for successfully broadcasting a serializable payload is correctly implemented and necessary for validating the
broadcastfunctionality.
884-906: LGTM!The test case for broadcasting an unserializable payload is correctly implemented and necessary for validating the error handling of the
broadcastfunctionality.
908-933: LGTM!The test case for triggering the handler for a subscribed broadcast event is correctly implemented and necessary for validating the event subscription functionality.
935-966: LGTM!The test case for not triggering the handler for an unsubscribed broadcast event is correctly implemented and necessary for validating the event unsubscription functionality.
968-1002: LGTM!The test case for not triggering the handler for a broadcast event after unsubscribing is correctly implemented and necessary for validating the event unsubscription functionality.
packages/sdk/src/document/document.ts (4)
176-180: LGTM!The addition of the
Broadcastevent type in theDocEventTypeenum is straightforward and aligns with the new broadcast functionality.
199-200: LGTM!The addition of the
BroadcastEventtype to theDocEventunion type is necessary for the new broadcast functionality.
403-403: LGTM!The addition of the
broadcastcallback to theDocEventCallbackMaptype is necessary for the new broadcast functionality.
834-842: LGTM!The addition of the
subscribemethod overload for thebroadcastevent type is necessary for the new broadcast functionality.
|
@hackerwins @chacha912 @sejongk Here's an update. In order to resolve the circular reference issue, we’ve removed the However, this solution presents two challenges: 1. Unsubscribing Handlers We had to ensure that handlers registered in export class Attachment<T, P extends Indexable> {
// skipped
unsubscribeBroadcastEvent: Unsubscribe;
}With this approach, when attaching a document to a client using the public attach<T, P extends Indexable>(
doc: Document<T, P>,
options: {
initialPresence?: P;
syncMode?: SyncMode;
} = {},
): Promise<Document<T, P>> {
// skipped
const unsubscribeBroacastEvent = doc.subscribe(
'broadcast',
(topic, payload, onBroadcastError) => {
try {
this.broadcast(doc.getKey(), topic, payload);
} catch (e: unknown) {
if (e instanceof Error) {
onBroadcastError?.(e);
}
}
},
);
this.attachmentMap.set(
doc.getKey(),
new Attachment(
this.reconnectStreamDelay,
doc,
res.documentId,
syncMode,
unsubscribeBroacastEvent,
),
); private detachInternal(docKey: DocumentKey) {
// skipped
attachment.unsubscribeBroadcastEvent();2. Error Handling for Broadcast Events The second challenge involves handling errors during broadcast events, such as when broadcasting an unserializable payload. Initially, we could simply use a try-catch block to handle errors when directly accessing the client's broadcast method: try {
doc.broadcast(broadcastTopic, payload)
}catch(e){
// do something
}However, since we’re now dealing with an event stream, a different approach to error handling is required. To accommodate this, I've introduced a new interface for broadcasting events. This interface includes an error handling function that triggers when a broadcast event fails. // Usage
const errorFn = () => {
// Do something
}
doc.broadcast("topic", payload, errorFn) |
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)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Additional comments not posted (6)
packages/sdk/test/integration/client_test.ts (6)
867-882: LGTM!The test case correctly verifies that broadcasting a serializable payload does not throw an error.
884-908: LGTM!The test case correctly verifies that broadcasting an unserializable payload throws an error.
910-935: LGTM!The test case correctly verifies that the handler is triggered for a subscribed broadcast event.
937-968: LGTM!The test case correctly verifies that the handler is not triggered for an unsubscribed broadcast event.
970-1004: LGTM!The test case correctly verifies that the handler is not triggered for a broadcast event after unsubscribing.
866-866: LGTM!The test case correctly verifies that local changes are handled correctly when receiving a snapshot.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/sdk/src/document/document.ts (1)
Line range hint
1540-1581: LGTM! But consider adding error handling for JSON parsing.The code changes are approved. However, consider adding error handling for the
JSON.parsecall to handle potential parsing errors gracefully.You can wrap the
JSON.parsecall in a try-catch block to catch any parsing errors:event.push({ type: DocEventType.Broadcast, value: { clientID: publisher, topic, - payload: JSON.parse(decoder.decode(payload)), + payload: (() => { + try { + return JSON.parse(decoder.decode(payload)); + } catch (e) { + console.error('Failed to parse broadcast payload:', e); + return null; + } + })(), }, });
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/sdk/src/client/client.ts (5 hunks)
- packages/sdk/src/document/document.ts (10 hunks)
- packages/sdk/test/integration/client_test.ts (1 hunks)
Additional comments not posted (17)
packages/sdk/src/client/client.ts (4)
607-655: LGTM!The new
broadcastmethod in theClientclass looks well implemented. It has proper checks for active client and attached document, validates the payload serializability, enqueues the broadcast task, encodes payload as JSON, and includes robust error handling.
348-348: LGTM!The code change to pass
unsubscribeBroacastEventto theAttachmentconstructor is approved.
307-321: LGTM!The new code to subscribe to document's 'local-broadcast' events and broadcast them using the client's
broadcastmethod looks good. It correctly extracts the topic and payload, calls thebroadcastmethod, and passes any errors to the event's error function.
821-821: LGTM!Calling
attachment.unsubscribeBroadcastEvent()when a document is detached in thedetachInternalmethod is a good change. It will prevent issues from subscriptions to a detached document.packages/sdk/test/integration/client_test.ts (6)
867-882: LGTM!The "Should successfully broadcast serializeable payload" test looks good. It correctly tests the happy path of broadcasting a serializable payload without errors.
884-908: LGTM!The "Should throw error when broadcasting unserializeable payload" test looks good. It correctly tests the scenario of broadcasting an unserializable payload and expects the right error message.
910-939: LGTM!The "Should trigger the handler for a subscribed broadcast event" test looks good. It correctly tests that a subscribed broadcast handler is triggered with the right topic and payload when a broadcast event is received.
941-974: LGTM!The "Should not trigger the handler for an unsubscribed broadcast event" test looks good. It correctly tests that a broadcast handler subscribed to a specific topic is not triggered when a broadcast event with a different topic is received.
976-1012: LGTM!The "Should not trigger the handler for a broadcast event after unsubscribing" test looks good. It correctly tests that a broadcast handler is not triggered after it has been unsubscribed, even if a broadcast event is received.
1014-1058: LGTM!The "Should not trigger the handler for a broadcast event sent by the publisher to itself" test looks good. It correctly tests that a broadcast event sent by a document does not trigger its own handler, but triggers handlers on other subscribed documents.
packages/sdk/src/document/document.ts (7)
176-185: LGTM!The code changes are approved.
204-206: LGTM!The code changes are approved.
386-396: LGTM!The code changes are approved.
415-416: LGTM!The code changes are approved.
847-864: LGTM!The code changes are approved.
1013-1036: LGTM!The code changes are approved.
2057-2069: LGTM!The code changes are approved.
|
Here's an update. As mentioned earlier, we've decided to have the 
Without differentiation, this setup can lead to an unexpected infinite loop. This happens because the client, upon receiving its own broadcast event, will rebroadcast it, creating a never-ending cycle. 
This scenario can be reproduced with the following test code. The test simulates a situation where the publisher is broadcasting an event and, at the same time, subscribing to the broadcast event. Currently, since Yorkie does not support self-broadcast filtering, the subscribe handler gets called, leading to numerous recursive broadcasts, as evidenced by the test results: it('Should not trigger the handler for a broadcast event sent by the publisher to itself', async ({
task,
}) => {
await withTwoClientsAndDocuments<{ t: Text }>(
async (c1, d1, c2, d2) => {
const eventCollector1 = new EventCollector<[string, any]>();
const eventCollector2 = new EventCollector<[string, any]>();
const broadcastTopic = 'test';
const payload = { a: 1, b: '2' };
// Publisher subscribes to the broadcast event
const unsubscribe1 = d1.subscribe('broadcast', (topic, payload) => {
if (topic === broadcastTopic) {
eventCollector1.add([topic, payload]);
}
});
const unsubscribe2 = d2.subscribe('broadcast', (topic, payload) => {
if (topic === broadcastTopic) {
eventCollector2.add([topic, payload]);
}
});
d1.broadcast(broadcastTopic, payload);
// Assuming that subscribers can receive the broadcast event within 1000ms.
await new Promise((res) => setTimeout(res, 1000));
unsubscribe1();
unsubscribe2();
assert.equal(eventCollector1.getLength(), 0);
assert.equal(eventCollector2.getLength(), 1);
},
task.name,
SyncMode.Realtime,
);
});
To resolve this issue, I've introduced a export enum DocEventType {
/**
* `Broadcast` means that the broadcast event is received from the remote client.
*/
Broadcast = 'broadcast',
/**
* `LocalBroadcast` means that the broadcast event is sent from the local client.
*/
LocalBroadcast = 'local-broadcast',
} /**
* `broadcast` the payload to the given topic.
*/
public broadcast(topic: string, payload: any, error?: ErrorFn) {
const broadcastEvent: LocalBroadcastEvent = {
type: DocEventType.LocalBroadcast,
value: { topic, payload },
error,
};
this.publish([broadcastEvent]);
}However, the above method still has a drawback. We may want to hide the doc.subscribe('local-broadcast', (event) => {
// Users do not need to know about this event
},
); |
There was a problem hiding this comment.
@gwbaik9717 @sejongk @chacha912
Thanks for your contribution and review.
To prevent serialization issues, this commit narrows the type of the `presence` object `P`. Previously, `<P extends Indexable>` allowed any value type, including non-JSON serializable ones like byte arrays, Date, and Long. We now introduce a `Json` type, inspired by Liveblocks, to ensure only JSON serializable types are allowed in the presence object. This change affects functions like `broadcast`, ensuring safe serialization and addressing issues such as #884. The new `Json` type is defined as follows: ``` export type Json = JsonPrimitive | JsonArray | JsonObject; type JsonPrimitive = string | number | boolean | null; type JsonArray = Array<Json>; type JsonObject = { [key: string]: Json | undefined }; ``` This type restriction enhances type safety and prevents potential runtime errors during JSON serialization of presence data. --------- Co-authored-by: Youngteac Hong <susukang98@gmail.com>
To prevent serialization issues, this commit narrows the type of the `presence` object `P`. Previously, `<P extends Indexable>` allowed any value type, including non-JSON serializable ones like byte arrays, Date, and Long. We now introduce a `Json` type, inspired by Liveblocks, to ensure only JSON serializable types are allowed in the presence object. This change affects functions like `broadcast`, ensuring safe serialization and addressing issues such as #884. The new `Json` type is defined as follows: ``` export type Json = JsonPrimitive | JsonArray | JsonObject; type JsonPrimitive = string | number | boolean | null; type JsonArray = Array<Json>; type JsonObject = { [key: string]: Json | undefined }; ``` This type restriction enhances type safety and prevents potential runtime errors during JSON serialization of presence data. --------- Co-authored-by: Youngteac Hong <susukang98@gmail.com>
What this PR does / why we need it?
This PR implements a broadcast API, which enables the sharing of a broader range of general events beyond the current document and presence events in Yorkie's Publish-Subscribe model.
Any background context you want to provide?
1. Broadcast Events:
Users can now broadcast custom events with a specified topic and payload.
The payload can be of any type, as long as it is serializable.
2. Subscribe to Broadcast Events:
Users can subscribe to specific topics and handle the events via a callback function.
The callback is triggered whenever an event with the corresponding topic is broadcast.
What are the relevant tickets?
Related to yorkie-team/yorkie#628
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
broadcastmethod in the Client class to send messages to specified topics.BroadcastandLocalBroadcastto enable clients to subscribe to broadcast messages.subscribemethod for clients to register handlers for broadcast events.Bug Fixes
Tests