1718042 - [devtools-rfc] Add a way to distinguish Actors emitting an rdp event from an "internal" one

Status: NEW

(DevTools :: General, task, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

Actors are extending EventEmitter (via Pool), and have access to an emit method to, well, emit a given event.
One issue is that emit might have 2 different meanings:

• the event can be a "simple" one, like when emit is used in the client, where other objects can listen to it
• but it can also mean that the event will be sent through RDP and piped to Fronts, if the event was defined in the actor spec file.

So you could have:

myAwesomeActor.emit("add")
myAwesomeActor.emit("ready");

without being able to tell which behavior will happen, unless you look into the spec file.

In order to make that clearer, there are some solutions:

1. have a method for emitting event "locally" (e.g. emitLocally), that would simply call emit in the end. That would work but would be weird, unless we convert all the client codebase to use that.
2. or the other way around, had a specific method to send events through RDP (e.g. emitForRDP ?). It could also simply call emit behind the scene.

both solutions should have the advantage of not being mandatory, so we could migrate the codebase bit by bit instead of a giant patch.

What do you think? Are there other solutions I didn't think of?`

Comment 1

[Julian Descottes [:jdescottes]]

Great idea! It would make issues such as https://bugzilla.mozilla.org/show_bug.cgi?id=1702764 less confusing.

Since actor classes are generated from the specs, emitForRDP could check the spec and verify that the event is listed in it.

Maybe emitForRDP could also add a small property in the payload (eg isRDPEvent: true) and then RDP could (optionally?) log events which are emitted without the isRDPEvent packet so that we can easily spot missing events and migrate existing code.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Ah yes, that sounds great. So we could have something like:

diff --git a/devtools/shared/protocol/Actor.js b/devtools/shared/protocol/Actor.js
--- a/devtools/shared/protocol/Actor.js
+++ b/devtools/shared/protocol/Actor.js
@@ -40,9 +40,15 @@ class Actor extends Pool {
 
     this._actorSpec = actorSpecs.get(Object.getPrototypeOf(this));
     // Forward events to the connection.
+    // Bug XXX: This can be removed when all RDP events use `emitForRDP`
     if (this._actorSpec && this._actorSpec.events) {
       for (const [name, request] of this._actorSpec.events.entries()) {
         this.on(name, (...args) => {
+          console.warn(
+            "[DEPREACTED] use `emitForRDP` instead of `emit` for event '" +
+              name +
+              "'"
+          );
           this._sendEvent(name, request, ...args);
         });
       }
@@ -53,6 +59,16 @@ class Actor extends Pool {
     return "[Actor " + this.typeName + "/" + this.actorID + "]";
   }
 
+  emitForRDP(name, ...args) {
+    if (!this._actorSpec.events.has(name)) {
+      throw new Error(
+        `"${name}" event isn't declared in ${this.typeName} spec`
+      );
+    }
+
+    this._sendEvent(name, this._actorSpec.events.get(name), ...args);
+  }
+
   _sendEvent(name, request, ...args) {
     if (this.isDestroyed()) {
       console.error(

(where the console.warn would be guarded against a pref or something).

---

One thing that came to my mind is that at the moment, there is nothing preventing an event to be consumed both as RDP event and as "regular" one, so a "RDP" event could still be listened to by an object on the server.
I don't know if it does happen in our codebase, and I'd say that if it's the case, it could be a bit weird anyway?

Comment 3

[Julian Descottes [:jdescottes]]

The snippet looks good! If we manage to introduce a preference-based logger, we could even use that for the warn ;)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)

One thing that came to my mind is that at the moment, there is nothing preventing an event to be consumed both as RDP event and as "regular" one, so a "RDP" event could still be listened to by an object on the server.
I don't know if it does happen in our codebase, and I'd say that if it's the case, it could be a bit weird anyway?

Good point, we should review the existing usage. There is one recent use case that comes to mind, with target-available-form which is a RDP event from the Watcher but which is also used by parent-process-storage.js. I don't have a strong opinion on that. It's already hard enough to communicate between actors on the server, so I don't want to make it even more complicated :)

But we could have specific methods to listen to RDP events, to reduce the confusion without making it too complicated to communicate between actors?

Other topic:
Another implementation option is to have another event emitter, which would not be the actor itself. Each actor would have a rdpEventEmitter object, which would just be an EventEmitter instance, and to emit events for the client, it would have to do this.rdpEventEmitter.emit() instead of this.emit. This avoids adding new APIs. I considered something similar for the BiDi architecture. There are many options, I'm sure others will come up with more ideas!