Restrict presence object type to JSON serializable values (#898)

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 <[email protected]>
yorkie-team · Sep 12, 2024 · 4d5416f · 4d5416f
1 parent 4837c7d
commit 4d5416f
Show file tree

Hide file tree

Showing 13 changed files with 53 additions and 37 deletions.
diff --git a/.eslintignore b/.eslintignore
@@ -8,3 +8,6 @@ packages/sdk/src/api/yorkie/v1/resources_grpc_web_pb.d.ts
 packages/sdk/src/api/yorkie/v1/resources_pb.d.ts
 packages/sdk/test/vitest.d.ts
 packages/sdk/lib
+
+# examples
+examples/react-tldraw/src/tldraw.d.ts
diff --git a/examples/react-tldraw/src/tldraw.d.ts b/examples/react-tldraw/src/tldraw.d.ts
@@ -0,0 +1,6 @@
+import { Indexable, Json } from '@yorkie-js-sdk/src/document/document';
+import { TDUser } from '@tldraw/tldraw';
+
+declare module '@tldraw/tldraw' {
+  interface TDUser extends Indexable {}
+}
diff --git a/examples/vanilla-quill/src/main.ts b/examples/vanilla-quill/src/main.ts
@@ -29,7 +29,7 @@ function toDeltaOperation<T extends TextValueType>(
 ): DeltaOperation {
   const { embed, ...restAttributes } = textValue.attributes ?? {};
   if (embed) {
-    return { insert: JSON.parse(embed), attributes: restAttributes };
+    return { insert: embed, attributes: restAttributes };
   }
 
   return {

diff --git a/examples/vanilla-quill/src/type.ts b/examples/vanilla-quill/src/type.ts
@@ -7,5 +7,5 @@ export type YorkieDoc = {
 export type YorkiePresence = {
   username: string;
   color: string;
-  selection: TextPosStructRange | undefined;
+  selection?: TextPosStructRange;
 };
diff --git a/packages/sdk/src/client/client.ts b/packages/sdk/src/client/client.ts
@@ -43,6 +43,7 @@ import { OpSource } from '@yorkie-js-sdk/src/document/operation/operation';
 import { createAuthInterceptor } from '@yorkie-js-sdk/src/client/auth_interceptor';
 import { createMetricInterceptor } from '@yorkie-js-sdk/src/client/metric_interceptor';
 import { validateSerializable } from '../util/validator';
+import { Json } from '@yorkie-js-sdk/src/document/document';
 
 /**
  * `SyncMode` defines synchronization modes for the PushPullChanges API.
@@ -607,7 +608,7 @@ export class Client {
   public broadcast(
     docKey: DocumentKey,
     topic: string,
-    payload: any,
+    payload: Json,
   ): Promise<void> {
     if (!this.isActive()) {
       throw new YorkieError(

diff --git a/packages/sdk/src/devtools/types.ts b/packages/sdk/src/devtools/types.ts
@@ -17,20 +17,7 @@
 import type { PrimitiveValue } from '@yorkie-js-sdk/src/document/crdt/primitive';
 import type { CRDTTreePosStruct } from '@yorkie-js-sdk/src/document/crdt/tree';
 import { CounterValue } from '@yorkie-js-sdk/src/document/crdt/counter';
-
-/**
- * `Json` represents a JSON value.
- *
- * TODO(hackerwins): We need to replace `Indexable` with `Json`.
- */
-export type Json =
-  | string
-  | number
-  | boolean
-  // eslint-disable-next-line @typescript-eslint/ban-types
-  | null
-  | { [key: string]: Json }
-  | Array<Json>;
+import { Json } from '@yorkie-js-sdk/src/document/document';
 
 /**
  * `Client` represents a client value in devtools.

diff --git a/packages/sdk/src/document/change/context.ts b/packages/sdk/src/document/change/context.ts
@@ -159,7 +159,7 @@ export class ChangeContext<P extends Indexable = Indexable> {
 
     const reversePresence: Partial<P> = {};
     for (const key of this.reversePresenceKeys) {
-      reversePresence[key as keyof P] = this.previousPresence[key];
+      reversePresence[key as keyof P] = this.previousPresence[key as keyof P];
     }
     return reversePresence;
   }

diff --git a/packages/sdk/src/document/document.ts b/packages/sdk/src/document/document.ts
@@ -385,7 +385,7 @@ export interface PresenceChangedEvent<P extends Indexable>
 
 export interface BroadcastEvent extends BaseDocEvent {
   type: DocEventType.Broadcast;
-  value: { clientID: ActorID; topic: string; payload: any };
+  value: { clientID: ActorID; topic: string; payload: Json };
   error?: ErrorFn;
 }
 
@@ -421,13 +421,25 @@ export type DocEventCallback<P extends Indexable> =
   DocEventCallbackMap<P>[DocEventTopic];
 
 /**
- * Indexable key, value
+ * `Json` represents the JSON data type. It is used to represent the data
+ * structure of the document.
+ */
+export type Json = JsonPrimitive | JsonArray | JsonObject;
+
+// eslint-disable-next-line @typescript-eslint/ban-types
+type JsonPrimitive = string | number | boolean | null;
+type JsonArray = Array<Json>;
+type JsonObject = { [key: string]: Json | undefined };
+
+/**
+ * `Indexable` represents the type of the indexable object. It is used to
+ * represent the presence information of the client.
  * @public
  */
-export type Indexable = Record<string, any>;
+export type Indexable = Record<string, Json>;
 
 /**
- * Document key type
+ * `DocumentKey` represents the key of the document.
  * @public
  */
 export type DocumentKey = string;
@@ -2058,7 +2070,7 @@ export class Document<T, P extends Indexable = Indexable> {
   /**
    * `broadcast` the payload to the given topic.
    */
-  public broadcast(topic: string, payload: any, error?: ErrorFn) {
+  public broadcast(topic: string, payload: Json, error?: ErrorFn) {
     const broadcastEvent: LocalBroadcastEvent = {
       type: DocEventType.LocalBroadcast,
       value: { topic, payload },

diff --git a/packages/sdk/src/document/json/tree.ts b/packages/sdk/src/document/json/tree.ts
@@ -82,14 +82,14 @@ function buildDescendants(
     parent.append(textNode);
   } else {
     const { children = [] } = treeNode as ElementNode;
-    let { attributes } = treeNode as ElementNode;
+    const { attributes } = treeNode as ElementNode;
     let attrs;
 
     if (typeof attributes === 'object' && !isEmpty(attributes)) {
-      attributes = stringifyObjectValues(attributes);
+      const stringifiedAttributes = stringifyObjectValues(attributes);
       attrs = new RHT();
 
-      for (const [key, value] of Object.entries(attributes)) {
+      for (const [key, value] of Object.entries(stringifiedAttributes)) {
         attrs.set(key, value, ticket);
       }
     }
@@ -121,14 +121,14 @@ function createCRDTTreeNode(context: ChangeContext, content: TreeNode) {
     root = CRDTTreeNode.create(CRDTTreeNodeID.of(ticket, 0), type, value);
   } else if (content) {
     const { children = [] } = content as ElementNode;
-    let { attributes } = content as ElementNode;
+    const { attributes } = content as ElementNode;
     let attrs;
 
     if (typeof attributes === 'object' && !isEmpty(attributes)) {
-      attributes = stringifyObjectValues(attributes);
+      const stringifiedAttributes = stringifyObjectValues(attributes);
       attrs = new RHT();
 
-      for (const [key, value] of Object.entries(attributes)) {
+      for (const [key, value] of Object.entries(stringifiedAttributes)) {
         attrs.set(key, value, ticket);
       }
     }

diff --git a/packages/sdk/src/util/object.ts b/packages/sdk/src/util/object.ts
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+import { Indexable } from '@yorkie-js-sdk/src/document/document';
+
 /**
  * `deepcopy` returns a deep copy of the given object.
  */
@@ -40,7 +42,7 @@ export const isEmpty = (object: object) => {
 /**
  * `stringifyObjectValues` makes values of attributes to JSON parsable string.
  */
-export const stringifyObjectValues = <A extends object>(
+export const stringifyObjectValues = <A extends Indexable>(
   attributes: A,
 ): Record<string, string> => {
   const attrs: Record<string, string> = {};
@@ -53,7 +55,7 @@ export const stringifyObjectValues = <A extends object>(
 /**
  `parseObjectValues` returns the JSON parsable string values to the origin states.
  */
-export const parseObjectValues = <A extends object>(
+export const parseObjectValues = <A extends Indexable>(
   attrs: Record<string, string>,
 ): A => {
   const attributes: Record<string, unknown> = {};

diff --git a/packages/sdk/src/util/validator.ts b/packages/sdk/src/util/validator.ts
@@ -17,7 +17,7 @@
 /**
  * `validateSerializable` returns whether the given value is serializable or not.
  */
-export const validateSerializable = (value: any): boolean => {
+export const validateSerializable = (value: unknown): boolean => {
   try {
     const serialized = JSON.stringify(value);
 

diff --git a/packages/sdk/test/integration/client_test.ts b/packages/sdk/test/integration/client_test.ts
@@ -900,6 +900,8 @@ describe.sequential('Client', function () {
       eventCollector.add(error.message);
     };
 
+    // @ts-ignore
+    // Disable type checking for testing purposes
     doc.broadcast(broadcastTopic, payload, errorHandler);
 
     await eventCollector.waitAndVerifyNthEvent(1, broadcastErrMessage);

diff --git a/packages/sdk/test/integration/integration_helper.ts b/packages/sdk/test/integration/integration_helper.ts
@@ -13,12 +13,15 @@ export function toDocKey(title: string): string {
     .replace(/[^a-z0-9-]/g, '-');
 }
 
-export async function withTwoClientsAndDocuments<T>(
+export async function withTwoClientsAndDocuments<
+  T,
+  P extends Indexable = Indexable,
+>(
   callback: (
     c1: Client,
-    d1: Document<T>,
+    d1: Document<T, P>,
     c2: Client,
-    d2: Document<T>,
+    d2: Document<T, P>,
   ) => Promise<void>,
   title: string,
   syncMode: SyncMode = SyncMode.Manual,
@@ -29,8 +32,8 @@ export async function withTwoClientsAndDocuments<T>(
   await client2.activate();
 
   const docKey = `${toDocKey(title)}-${new Date().getTime()}`;
-  const doc1 = new yorkie.Document<T>(docKey);
-  const doc2 = new yorkie.Document<T>(docKey);
+  const doc1 = new yorkie.Document<T, P>(docKey);
+  const doc2 = new yorkie.Document<T, P>(docKey);
 
   await client1.attach(doc1, { syncMode });
   await client2.attach(doc2, { syncMode });