fix: enforce that plugins must define their target

collective-soundworks · Oct 9, 2024 · d443f42 · d443f42
1 parent c7918c0
commit d443f42
Show file tree

Hide file tree

Showing 18 changed files with 217 additions and 58 deletions.
diff --git a/src/client/ClientPluginManager.js b/src/client/ClientPluginManager.js
@@ -99,7 +99,11 @@ class ClientPluginManager extends BasePluginManager {
     const ctor = factory(ClientPlugin);
 
     if (!(ctor.prototype instanceof ClientPlugin)) {
-      throw new Error(`[soundworks.ClientPluginManager] Invalid argument, "pluginManager.register" second argument should be a factory function returning a class extending the "Plugin" base class`);
+      throw new Error(`[ClientPluginManager] Invalid argument, "pluginManager.register" second argument should be a factory function returning a class extending the "Plugin" base class`);
+    }
+
+    if (ctor.target === undefined || ctor.target !== 'client') {
+      throw new Error(`[ClientPluginManager] Invalid argument, The plugin class should implement a 'target' static field with value 'client'`);
     }
 
     super.register(id, ctor, options, deps);

diff --git a/src/server/ServerPluginManager.js b/src/server/ServerPluginManager.js
@@ -141,7 +141,11 @@ class ServerPluginManager extends BasePluginManager {
     const ctor = factory(ServerPlugin);
 
     if (!(ctor.prototype instanceof ServerPlugin)) {
-      throw new Error(`[soundworks.PluginManager] Invalid argument, "pluginManager.register" second argument should be a factory function returning a class extending the "ServerPlugin" base class`);
+      throw new Error(`[ServerPluginManager] Invalid argument, "pluginManager.register" second argument should be a factory function returning a class extending the "ServerPlugin" base class`);
+    }
+
+    if (ctor.target === undefined || ctor.target !== 'server') {
+      throw new Error(`[ServerPluginManager] Invalid argument, The plugin class should implement a 'target' static field with value 'server'`)
     }
 
     super.register(id, ctor, options, deps);

diff --git a/src/server/ServerStateManager.js b/src/server/ServerStateManager.js
@@ -154,7 +154,7 @@ class ServerStateManager extends BaseStateManager {
   /** @private */
   async [kServerStateManagerDeletePrivateState](state) {
     this.#sharedStatePrivateById.delete(state.id);
-    // execute hooks
+    // @todo - could use getValuesUnsafe instead
     let currentValues = state[kSharedStatePrivateGetValues]();
     const hooks = this.#deleteHooksByClassName.get(state.className);
 
@@ -186,8 +186,7 @@ class ServerStateManager extends BaseStateManager {
    *
    * This is automatically handled by the {@link Server} when a client connects.
    *
-   * @param {number} nodeId - Id of the client node, as given in
-   *  {@link client.StateManager}
+   * @param {number} nodeId - Unique id of the client node
    * @param {object} transport - Transport mecanism to communicate with the
    *  client. Must implement a basic EventEmitter API.
    *

diff --git a/tests/essentials/Client.spec.js b/tests/essentials/Client.spec.js
@@ -11,7 +11,7 @@ import {
 import pluginDelayClient from '../utils/PluginDelayClient.js';
 import config from '../utils/config.js';
 
-describe('# client::Client', () => {
+describe('# Client', () => {
   describe(`## new Client(config)`, () => {
     it(`should throw if no config is given`, () => {
       try {
@@ -276,7 +276,9 @@ describe('# client::Client', () => {
     it(`should stop the contexts first and then the plugins`, async () => {
       const server = new Server(config);
       server.pluginManager.register('test-plugin', Plugin => {
-        return class TestPlugin extends Plugin {}
+        return class TestPlugin extends Plugin {
+          static target = 'server';
+        }
       });
 
       await server.init();
@@ -297,6 +299,7 @@ describe('# client::Client', () => {
 
       client.pluginManager.register('test-plugin', Plugin => {
         return class TestPlugin extends Plugin {
+          static target = 'client';
           async start() {
             await super.start();
             // just check that we are ok with that, and that we are not stuck

diff --git a/tests/essentials/Server.spec.js b/tests/essentials/Server.spec.js
@@ -21,7 +21,7 @@ import config from '../utils/config.js';
 const __filename = url.fileURLToPath(import.meta.url);
 const __dirname = url.fileURLToPath(new URL('.', import.meta.url));
 
-describe('# server::Server', () => {
+describe('# Server', () => {
   describe(`## new Server(config)`, () => {
     it(`should throw if invalid config object`, () => {
 
@@ -445,6 +445,8 @@ describe('# server::Server', () => {
       const server = new Server(config);
       server.pluginManager.register('test-plugin', Plugin => {
         return class TestPlugin extends Plugin {
+          static target = 'server';
+
           async start() {
             await super.start();
             // just check that we are ok with that, and that we are not stuck

diff --git a/tests/plugins/ClientPluginManager.spec.js b/tests/plugins/ClientPluginManager.spec.js
@@ -136,12 +136,11 @@ describe(`# PluginManagerClient`, () => {
       assert.ok('should not throw');
     });
 
-    it.skip(`[FIXME #83] should throw when registering server side plugin on client side`, async () => {
-      client.pluginManager.register('delay-1', pluginDelayServer, {});
-
+    it(`should throw when registering server side plugin on client side`, async () => {
       let errored = false;
+
       try {
-        await client.init();
+        client.pluginManager.register('delay-1', pluginDelayServer, {});
       } catch (err) {
         console.log(err.message);
         errored = true;

diff --git a/tests/plugins/Plugin.spec.js b/tests/plugins/Plugin.spec.js
@@ -60,13 +60,13 @@ describe('# Plugin', () => {
   describe(`## [client] Plugin.state propagation`, () => {
     it(`should propagate its inner state`, async () => {
       const server = new Server(config);
-      server.pluginManager.register('stateful', (ServerPlugin) => class StatefulPlugin extends ServerPlugin {});
+      server.pluginManager.register('stateful', (ServerPlugin) => class StatefulPlugin extends ServerPlugin { static target = 'server' });
 
       await server.init();
       await server.start();
 
       const client = new Client({ role: 'test', ...config });
-      client.pluginManager.register('stateful', (ClientPlugin) => class StatefulPlugin extends ClientPlugin {});
+      client.pluginManager.register('stateful', (ClientPlugin) => class StatefulPlugin extends ClientPlugin { static target = 'client' });
       await client.init();
 
       const plugin = await client.pluginManager.get('stateful');
@@ -91,7 +91,9 @@ describe('# Plugin', () => {
     it(`should be forwarded by the stateManager`, async () => {
       const server = new Server(config);
       server.pluginManager.register('stateful', (Plugin) => {
-        return class StatefulPlugin extends Plugin {}
+        return class StatefulPlugin extends Plugin {
+          static target = 'server';
+        }
       });
 
       await server.init();
@@ -102,6 +104,8 @@ describe('# Plugin', () => {
 
       client.pluginManager.register('stateful', (Plugin) => {
         return class StatefulPlugin extends Plugin {
+          static target = 'client';
+
           constructor(client, id) {
             super(client, id);
             this.state = { rand: 0 };
@@ -141,6 +145,8 @@ describe('# Plugin', () => {
 
       server.pluginManager.register('dependent', (Plugin) => {
         return class Dependant extends Plugin {
+          static target = 'server';
+
           constructor(server, id) {
             super(server, id);
 
@@ -177,6 +183,8 @@ describe('# Plugin', () => {
 
       client.pluginManager.register('dependent', (Plugin) => {
         return class Dependant extends Plugin {
+          static target = 'client';
+
           constructor(client, id) {
             super(client, id);
 
@@ -215,8 +223,10 @@ describe('# Plugin', () => {
   describe(`## [server] Plugin.state propagation`, () => {
     it('PluginManager should properly propagate plugin state', async () => {
       const server = new Server(config);
-      server.pluginManager.register('stateful', (ClientPlugin) => {
-        return class StatefulPlugin extends ClientPlugin {
+      server.pluginManager.register('stateful', (Plugin) => {
+        return class StatefulPlugin extends Plugin {
+          static target = 'server';
+
           constructor(client, id) {
             super(client, id);
             this.state = { rand: 0 };

diff --git a/tests/plugins/ServerPluginManager.spec.js b/tests/plugins/ServerPluginManager.spec.js
@@ -5,6 +5,7 @@ import { Client } from '../../src/client/index.js';
 import ServerPluginManager from '../../src/server/ServerPluginManager.js';
 import ServerPlugin from '../../src/server/ServerPlugin.js';
 import pluginDelayServer from '../utils/PluginDelayServer.js';
+import pluginDelayClient from '../utils/PluginDelayClient.js';
 import config from '../utils/config.js';
 import {
   kStateManagerClientsByNodeId,
@@ -121,6 +122,21 @@ describe(`# ServerPluginManager`, () => {
 
       assert.ok('should not throw');
     });
+
+    it(`should throw when registering client side plugin on server side`, async () => {
+      let errored = false;
+
+      try {
+        server.pluginManager.register('delay-1', pluginDelayClient, {});
+      } catch (err) {
+        console.log(err.message);
+        errored = true;
+      }
+
+      if (!errored) {
+        assert.fail(`should have thrown`);
+      }
+    });
   });
 
   describe(`## [private] async init()`, () => {
@@ -297,6 +313,8 @@ describe(`# ServerPluginManager`, () => {
 
       function testPluginFactory(Plugin) {
         return class TestPlugin extends Plugin {
+          static target = 'server';
+
           async addClient(client) {
             await super.addClient(client);
             addClientCalled = true;
@@ -311,7 +329,7 @@ describe(`# ServerPluginManager`, () => {
       const plugin = await server.pluginManager.get('test-plugin');
 
       const client = new Client({ role: 'test', ...config });
-      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin {});
+      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin { static target = 'client' });
       await client.init();
 
       assert.equal(plugin.clients.size, 1);
@@ -325,6 +343,8 @@ describe(`# ServerPluginManager`, () => {
 
       function testPluginFactory(Plugin) {
         return class TestPlugin extends Plugin {
+          static target = 'server';
+
           async addClient(client) {
             await super.addClient(client);
             assert.equal(this.clients.has(client), true);
@@ -337,6 +357,8 @@ describe(`# ServerPluginManager`, () => {
 
       function testPluginFactory2(Plugin) {
         return class TestPlugin extends Plugin {
+          static target = 'server';
+
           async addClient(client) {
             await super.addClient(client);
             addClientCalled2 = true;
@@ -351,7 +373,7 @@ describe(`# ServerPluginManager`, () => {
       await server.start();
 
       const client = new Client({ role: 'test', ...config });
-      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin {});
+      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin { static target = 'client' });
       await client.init();
       await client.start();
 
@@ -368,6 +390,8 @@ describe(`# ServerPluginManager`, () => {
 
       function testPluginFactory(Plugin) {
         return class TestPlugin extends Plugin {
+          static target = 'server';
+
           async removeClient(client) {
             await super.removeClient(client);
             removeClientCalled = true;
@@ -381,7 +405,7 @@ describe(`# ServerPluginManager`, () => {
       await server.start();
 
       const client = new Client({ role: 'test', ...config });
-      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin {});
+      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin { static target = 'client' });
       await client.init();
       await client.start();
 
@@ -399,6 +423,8 @@ describe(`# ServerPluginManager`, () => {
 
       function testPluginFactory(Plugin) {
         return class TestPlugin extends Plugin {
+          static target = 'server';
+
           async removeClient(client) {
             await super.removeClient(client);
 
@@ -415,7 +441,7 @@ describe(`# ServerPluginManager`, () => {
       await server.start();
 
       const client = new Client({ role: 'test', ...config });
-      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin {});
+      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin { static target = 'client' });
       await client.init();
       await client.start();
 
@@ -432,6 +458,8 @@ describe(`# ServerPluginManager`, () => {
 
       function testPluginFactory(Plugin) {
         return class TestPlugin extends Plugin {
+          static target = 'server';
+
           async removeClient(client) {
             await super.removeClient(client);
             removeClientCalled = true;
@@ -443,6 +471,8 @@ describe(`# ServerPluginManager`, () => {
 
       function testPluginFactory2(Plugin) {
         return class TestPlugin extends Plugin {
+          static target = 'server';
+
           async removeClient(client) {
             await super.removeClient(client);
             removeClientCalled2 = true;
@@ -457,7 +487,7 @@ describe(`# ServerPluginManager`, () => {
       await server.start();
 
       const client = new Client({ role: 'test', ...config });
-      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin {});
+      client.pluginManager.register('test-plugin', (Plugin) => class TestPlugin extends Plugin { static target = 'client' });
       await client.init();
       await client.start();
       await client.stop();

diff --git a/tests/utils/PluginDelayClient.js b/tests/utils/PluginDelayClient.js
@@ -1,5 +1,7 @@
 export default function(Plugin) {
   return class PluginDelayClient extends Plugin {
+    static target = 'client';
+
     constructor(client, id, options) {
       super(client, id);
 

diff --git a/tests/utils/PluginDelayServer.js b/tests/utils/PluginDelayServer.js
@@ -1,5 +1,7 @@
 export default function(Plugin) {
   return class PluginDelayServer extends Plugin {
+    static target = 'server';
+
     constructor(server, id, options) {
       super(server, id);
 

diff --git a/types/common/BasePlugin.d.ts b/types/common/BasePlugin.d.ts
@@ -5,7 +5,7 @@ export default BasePlugin;
  */
 export type pluginOnStateChangeCallback = (#state: BasePlugin) => any;
 /**
- * Delete the registered {@link pluginOnStateChangeCallback }.
+ * Delete the registered {@link pluginOnStateChangeCallback}.
  */
 export type pluginDeleteOnStateChangeCallback = () => any;
 /**

diff --git a/types/common/BasePluginManager.d.ts b/types/common/BasePluginManager.d.ts
@@ -7,7 +7,7 @@ export default BasePluginManager;
  */
 export type pluginManagerOnStateChangeCallback = (: object, initiator: ClientPlugin | ServerPlugin | null) => any;
 /**
- * Delete the registered {@link pluginManagerOnStateChangeCallback }.
+ * Delete the registered {@link pluginManagerOnStateChangeCallback}.
  */
 export type pluginManagerDeleteOnStateChangeCallback = () => any;
 /**

diff --git a/types/common/BaseStateManager.d.ts b/types/common/BaseStateManager.d.ts
@@ -7,7 +7,7 @@ export default BaseStateManager;
  */
 export type stateManagerObserveCallback = () => any;
 /**
- * Callback to execute in order to remove a {@link stateManagerObserveCallback }
+ * Callback to execute in order to remove a {@link stateManagerObserveCallback}
  * from the list of observer.
  */
 export type stateManagerDeleteObserveCallback = () => any;

diff --git a/types/common/SharedState.d.ts b/types/common/SharedState.d.ts
@@ -1,11 +1,11 @@
 export const kSharedStatePromiseStore: unique symbol;
 export default SharedState;
 /**
- * Callback executed when updates are applied on a {@link SharedState }.
+ * Callback executed when updates are applied on a {@link SharedState}.
  */
 export type sharedStateOnUpdateCallback = (newValues: any, oldValues: any) => any;
 /**
- * Delete the registered {@link sharedStateOnUpdateCallback }.
+ * Delete the registered {@link sharedStateOnUpdateCallback}.
  */
 export type sharedStateDeleteOnUpdateCallback = () => any;
 /**
@@ -79,7 +79,16 @@ export type sharedStateDeleteOnUpdateCallback = () => any;
  * ```
  */
 declare class SharedState {
-    constructor(id: any, remoteId: any, className: any, classDescription: any, client: any, isOwner: any, manager: any, initValues: any, filter: any);
+    constructor({ stateId, instanceId, className, classDescription, isOwner, manager, initValues, filter, }: {
+        stateId: any;
+        instanceId: any;
+        className: any;
+        classDescription: any;
+        isOwner: any;
+        manager: any;
+        initValues: any;
+        filter: any;
+    });
     /**
      * Id of the state
      * @type {Number}