Enforce member privacy using Private Elements (#1021)

Author: EzraBrooks

src/core/Action.ts

@@ -22,14 +22,8 @@ export default class Action<
   TResult = unknown,
 > extends EventEmitter {
   isAdvertised = false;
-  /**
-   * @private
-   */
-  _actionCallback: ((goal: TGoal, id: string) => void) | null = null;
-  /**
-   * @private
-   */
-  _cancelCallback: ((id: string) => void) | null = null;
+  #actionCallback: ((goal: TGoal, id: string) => void) | null = null;
+  #cancelCallback: ((id: string) => void) | null = null;
   ros: Ros;
   name: string;
   actionType: string;
@@ -141,9 +135,9 @@ export default class Action<
       return;
     }
 
-    this._actionCallback = actionCallback;
-    this._cancelCallback = cancelCallback;
-    this.ros.on(this.name, this._executeAction.bind(this));
+    this.#actionCallback = actionCallback;
+    this.#cancelCallback = cancelCallback;
+    this.ros.on(this.name, this.#executeAction.bind(this));
     this.ros.callOnConnection({
       op: "advertise_action",
       type: this.actionType,
@@ -175,24 +169,24 @@ export default class Action<
    * @param rosbridgeRequest.id - The ID of the action goal.
    * @param rosbridgeRequest.args - The arguments of the action goal.
    */
-  _executeAction(rosbridgeRequest: { id: string; args: TGoal }) {
+  #executeAction(rosbridgeRequest: { id: string; args: TGoal }) {
     const id = rosbridgeRequest.id;
 
     // If a cancellation callback exists, call it when a cancellation event is emitted.
     if (typeof id === "string") {
       this.ros.on(id, (message) => {
         if (
           isRosbridgeCancelActionGoalMessage(message) &&
-          typeof this._cancelCallback === "function"
+          typeof this.#cancelCallback === "function"
         ) {
-          this._cancelCallback(id);
+          this.#cancelCallback(id);
         }
       });
     }
 
     // Call the action goal execution function provided.
-    if (typeof this._actionCallback === "function") {
-      this._actionCallback(rosbridgeRequest.args, id);
+    if (typeof this.#actionCallback === "function") {
+      this.#actionCallback(rosbridgeRequest.args, id);
     }
   }
 

src/core/Ros.ts

@@ -76,9 +76,8 @@ export default class Ros extends EventEmitter {
    * Create the appropriate transport based on transport library configuration
    * @param url - WebSocket URL or RTCDataChannel label for rosbridge.
    * @returns The created transport
-   * @private
    */
-  async createTransport(
+  async #createTransport(
     url: string,
   ): Promise<WebSocket | RTCDataChannel | import("ws").WebSocket | null> {
     if (this.transportLibrary.constructor.name === "RTCPeerConnection") {
@@ -116,7 +115,7 @@ export default class Ros extends EventEmitter {
    * @param url - WebSocket URL or RTCDataChannel label for rosbridge.
    */
   async connect(url: string) {
-    const transport = await this.createTransport(url);
+    const transport = await this.#createTransport(url);
 
     if (!transport) {
       return; // Already connected
@@ -135,7 +134,7 @@ export default class Ros extends EventEmitter {
         this.emit("error", event);
       },
       onMessage: (message) => {
-        this.handleMessage(message);
+        this.#handleMessage(message);
       },
       decoder: this.transportOptions.decoder,
     });
@@ -144,9 +143,8 @@ export default class Ros extends EventEmitter {
   /**
    * Handle processed messages from SocketAdapter
    * @param message
-   * @private
    */
-  handleMessage(message: RosbridgeMessage) {
+  #handleMessage(message: RosbridgeMessage) {
     if (isRosbridgePublishMessage(message)) {
       this.emit(message.topic, message.msg);
     } else if (isRosbridgeServiceResponseMessage(message)) {

src/core/Service.ts

@@ -16,24 +16,21 @@ import Ros from "./Ros.js";
 export default class Service<TRequest, TResponse> extends EventEmitter {
   /**
    * Stores a reference to the most recent service callback advertised so it can be removed from the EventEmitter during un-advertisement
-   * @private
    */
-  _serviceCallback:
+  #serviceCallback:
     | ((
         rosbridgeRequest: RosbridgeCallServiceMessage<TRequest>,
       ) => void | Promise<void>)
     | null = null;
   isAdvertised = false;
   /**
    * Queue for serializing advertise/unadvertise operations to prevent race conditions
-   * @private
    */
-  _operationQueue = Promise.resolve();
+  #operationQueue = Promise.resolve();
   /**
    * Track if an unadvertise operation is pending to prevent double operations
-   * @private
    */
-  _pendingUnadvertise = false;
+  #pendingUnadvertise = false;
   ros: Ros;
   name: string;
   serviceType: string;
@@ -115,15 +112,15 @@ export default class Service<TRequest, TResponse> extends EventEmitter {
     callback: (request: TRequest, response: Partial<TResponse>) => boolean,
   ): Promise<void> {
     // Queue this operation to prevent race conditions
-    this._operationQueue = this._operationQueue
+    this.#operationQueue = this.#operationQueue
       .then(async () => {
         // If already advertised, unadvertise first
         if (this.isAdvertised) {
-          await this._doUnadvertise();
+          await this.#doUnadvertise();
         }
 
         // Store the new callback for removal during un-advertisement
-        this._serviceCallback = (rosbridgeRequest) => {
+        this.#serviceCallback = (rosbridgeRequest) => {
           const response = {};
           let success: boolean;
           try {
@@ -150,7 +147,7 @@ export default class Service<TRequest, TResponse> extends EventEmitter {
           }
         };
 
-        this.ros.on(this.name, this._serviceCallback);
+        this.ros.on(this.name, this.#serviceCallback);
         this.ros.callOnConnection({
           op: "advertise_service",
           type: this.serviceType,
@@ -163,19 +160,18 @@ export default class Service<TRequest, TResponse> extends EventEmitter {
         throw err;
       });
 
-    return this._operationQueue;
+    return this.#operationQueue;
   }
 
   /**
    * Internal method to perform unadvertisement without queueing
-   * @private
    */
-  async _doUnadvertise() {
-    if (!this.isAdvertised || this._pendingUnadvertise) {
+  async #doUnadvertise() {
+    if (!this.isAdvertised || this.#pendingUnadvertise) {
       return;
     }
 
-    this._pendingUnadvertise = true;
+    this.#pendingUnadvertise = true;
 
     try {
       /*
@@ -185,9 +181,9 @@ export default class Service<TRequest, TResponse> extends EventEmitter {
       this.isAdvertised = false;
 
       // Remove the registered callback to stop processing new requests
-      if (this._serviceCallback) {
-        this.ros.off(this.name, this._serviceCallback);
-        this._serviceCallback = null;
+      if (this.#serviceCallback) {
+        this.ros.off(this.name, this.#serviceCallback);
+        this.#serviceCallback = null;
       }
 
       /*
@@ -200,22 +196,22 @@ export default class Service<TRequest, TResponse> extends EventEmitter {
         service: this.name,
       });
     } finally {
-      this._pendingUnadvertise = false;
+      this.#pendingUnadvertise = false;
     }
   }
 
   async unadvertise(): Promise<void> {
     // Queue this operation to prevent race conditions
-    this._operationQueue = this._operationQueue
+    this.#operationQueue = this.#operationQueue
       .then(async () => {
-        await this._doUnadvertise();
+        await this.#doUnadvertise();
       })
       .catch((err) => {
         this.emit("error", err);
         throw err;
       });
 
-    return this._operationQueue;
+    return this.#operationQueue;
   }
 
   /**
@@ -226,14 +222,14 @@ export default class Service<TRequest, TResponse> extends EventEmitter {
     callback: (request: TRequest) => Promise<TResponse>,
   ): Promise<void> {
     // Queue this operation to prevent race conditions
-    this._operationQueue = this._operationQueue
+    this.#operationQueue = this.#operationQueue
       .then(async () => {
         // If already advertised, unadvertise first
         if (this.isAdvertised) {
-          await this._doUnadvertise();
+          await this.#doUnadvertise();
         }
 
-        this._serviceCallback = async (rosbridgeRequest) => {
+        this.#serviceCallback = async (rosbridgeRequest) => {
           try {
             this.ros.callOnConnection({
               op: "service_response",
@@ -252,7 +248,7 @@ export default class Service<TRequest, TResponse> extends EventEmitter {
             } satisfies RosbridgeServiceResponseMessage<TResponse>);
           }
         };
-        this.ros.on(this.name, this._serviceCallback);
+        this.ros.on(this.name, this.#serviceCallback);
         this.ros.callOnConnection({
           op: "advertise_service",
           type: this.serviceType,
@@ -265,6 +261,6 @@ export default class Service<TRequest, TResponse> extends EventEmitter {
         throw err;
       });
 
-    return this._operationQueue;
+    return this.#operationQueue;
   }
 }

src/core/Topic.ts

@@ -125,7 +125,7 @@ export default class Topic<T> extends EventEmitter {
     }
   }
 
-  _messageCallback = (data: T) => {
+  #messageCallback = (data: T) => {
     this.emit("message", data);
   };
   /**
@@ -142,7 +142,7 @@ export default class Topic<T> extends EventEmitter {
     if (this.subscribeId) {
       return;
     }
-    this.ros.on(this.name, this._messageCallback);
+    this.ros.on(this.name, this.#messageCallback);
     this.subscribeId =
       "subscribe:" + this.name + ":" + (++this.ros.idCounter).toString();
 
@@ -177,7 +177,7 @@ export default class Topic<T> extends EventEmitter {
       return;
     }
     // Note: Don't call this.removeAllListeners, allow client to handle that themselves
-    this.ros.off(this.name, this._messageCallback);
+    this.ros.off(this.name, this.#messageCallback);
     if (this.reconnect_on_close) {
       this.ros.off("close", this.reconnectFunc);
     }

src/tf/BaseTFClient.ts

@@ -12,7 +12,6 @@ export default class BaseTFClient extends EventEmitter {
     { transform?: Transform; cbs: ((tf: Transform) => void)[] }
   > = {};
   republisherUpdateRequested = false;
-  _isDisposed = false;
   ros: Ros;
   fixedFrame: string;
   angularThres: number;
@@ -159,12 +158,4 @@ export default class BaseTFClient extends EventEmitter {
       delete this.frameInfos[frameID];
     }
   }
-
-  /**
-   * Basic dispose functionality. Subclasses should override to add
-   * their specific cleanup logic.
-   */
-  dispose() {
-    this._isDisposed = true;
-  }
 }

src/tf/ROS2TFClient.ts

@@ -63,7 +63,6 @@ export default class ROS2TFClient extends BaseTFClient {
    * Unsubscribe and unadvertise all topics associated with this TFClient.
    */
   dispose() {
-    super.dispose();
     if (this.goal_id !== "") {
       this.actionClient.cancelGoal(this.goal_id);
     }

src/tf/TFClient.ts

@@ -26,7 +26,8 @@ export default class TFClient extends BaseTFClient {
     tf2_web_republisher.RepublishTFsRequest,
     tf2_web_republisher.RepublishTFsResponse
   >;
-  _subscribeCB: ((tf: tf2_msgs.TFMessage) => void) | undefined = undefined;
+  #subscribeCB: ((tf: tf2_msgs.TFMessage) => void) | undefined = undefined;
+  #isDisposed = false;
 
   /**
    * @param options
@@ -130,7 +131,7 @@ export default class TFClient extends BaseTFClient {
      * Do not setup a topic subscription if already disposed. Prevents a race condition where
      * The dispose() function is called before the service call receives a response.
      */
-    if (this._isDisposed) {
+    if (this.#isDisposed) {
       return;
     }
 
@@ -139,27 +140,27 @@ export default class TFClient extends BaseTFClient {
      * the republisher stops publishing it
      */
     if (this.currentTopic) {
-      this.currentTopic.unsubscribe(this._subscribeCB);
+      this.currentTopic.unsubscribe(this.#subscribeCB);
     }
 
     this.currentTopic = new Topic<tf2_msgs.TFMessage>({
       ros: this.ros,
       name: response.topic_name,
       messageType: "tf2_web_republisher/TFArray",
     });
-    this._subscribeCB = this.processTFArray.bind(this);
+    this.#subscribeCB = this.processTFArray.bind(this);
     // @ts-expect-error Function was bound above
-    this.currentTopic.subscribe(this._subscribeCB);
+    this.currentTopic.subscribe(this.#subscribeCB);
   }
 
   /**
    * Unsubscribe and unadvertise all topics associated with this TFClient.
    */
   dispose() {
-    super.dispose();
+    this.#isDisposed = true;
     this.actionClient.dispose();
     if (this.currentTopic) {
-      this.currentTopic.unsubscribe(this._subscribeCB);
+      this.currentTopic.unsubscribe(this.#subscribeCB);
     }
   }
 }

src/util/decompressPng.ts

@@ -10,7 +10,6 @@ import pngparse from "pngparse";
  * gzipping over WebSockets * is not supported yet), this function decodes
  * the "image" as a Base64 string.
  *
- * @private
  * @param data - An object containing the PNG data.
  * @param callback - Function with the following params:
  */

src/util/shim/decompressPng.ts

@@ -8,7 +8,6 @@
  * gzipping over WebSockets * is not supported yet), this function places the
  * "image" in a canvas element then decodes the * "image" as a Base64 string.
  *
- * @private
  * @param data - A string containing the PNG data.
  * @param callback - Function with the following params:
  */