breaking(plugin): Drop factory method to use register class directly

collective-soundworks · Jan 20, 2025 · 1dce5a8 · 1dce5a8
1 parent b3b3a4a
commit 1dce5a8
Show file tree

Hide file tree

Showing 13 changed files with 240 additions and 329 deletions.
diff --git a/src/client/ClientPluginManager.js b/src/client/ClientPluginManager.js
@@ -82,7 +82,7 @@ class ClientPluginManager extends BasePluginManager {
    *
    * @param {string} id - Unique id of the plugin. Enables the registration of the
    *  same plugin factory under different ids.
-   * @param {Function} factory - Factory function that returns the Plugin class.
+   * @param {ClientPlugin} ctor - The client-side Class of the plugin.
    * @param {object} [options={}] - Options to configure the plugin.
    * @param {array} [deps=[]] - List of plugins' names the plugin depends on, i.e.
    *  the plugin initialization will start only after the plugins it depends on are
@@ -95,22 +95,12 @@ class ClientPluginManager extends BasePluginManager {
    * // server-side
    * server.pluginManager.register('user-defined-id', pluginFactory);
    */
-  register(id, factory, options = {}, deps = []) {
-    // Note that additional argument checks are done on the BasePluginManager
-
-    // @todo - review all this
-    const ctor = factory(ClientPlugin);
-
-    if (!(ctor.prototype instanceof ClientPlugin)) {
-      throw new TypeError(`Cannot execute 'register' on ClientPluginManager: argument 2 must be a factory function returning a class extending the "ClientPlugin" base class`);
+  register(id, ctor, options = {}, deps = []) {
+    // Note that other arguments are checked on the BasePluginManager
+    if (!ctor || !(ctor.prototype instanceof ClientPlugin)) {
+      throw new TypeError(`Cannot execute 'register' on ClientPluginManager: argument 2 must be a class that extends 'ClientPlugin'`);
     }
 
-    if (ctor.target === undefined || ctor.target !== 'client') {
-      throw new TypeError(`Cannot execute 'register' on ClientPluginManager: The plugin class must implement a 'target' static field with value 'client'`);
-    }
-
-    // @todo - check deps
-
     super.register(id, ctor, options, deps);
   }
 

diff --git a/src/client/index.js b/src/client/index.js
@@ -36,4 +36,5 @@
  */
 
 export { default as Client } from './Client.js';
+export { default as ClientPlugin } from './ClientPlugin.js';
 export { default as ClientContext } from './ClientContext.js';
diff --git a/src/server/ServerPluginManager.js b/src/server/ServerPluginManager.js
@@ -124,7 +124,7 @@ class ServerPluginManager extends BasePluginManager {
    *
    * @param {string} id - Unique id of the plugin. Enables the registration of the
    *  same plugin factory under different ids.
-   * @param {Function} factory - Factory function that returns the Plugin class.
+   * @param {ServerPlugin} ctor - The server-side Class of the plugin.
    * @param {object} [options={}] - Options to configure the plugin.
    * @param {array} [deps=[]] - List of plugins' names the plugin depends on, i.e.
    *  the plugin initialization will start only after the plugins it depends on are
@@ -137,22 +137,12 @@ class ServerPluginManager extends BasePluginManager {
    * // server-side
    * server.pluginManager.register('user-defined-id', pluginFactory);
    */
-  register(id, factory = null, options = {}, deps = []) {
-    // Note that additional argument checks are done on the BasePluginManager
-
-    // @todo - review all this
-    const ctor = factory(ServerPlugin);
-
-    if (!(ctor.prototype instanceof ServerPlugin)) {
-      throw new TypeError(`Cannot execute 'register' on ServerPluginManager: argument 2 must be a factory function returning a class extending the "ServerPlugin" base class`);
+  register(id, ctor, options = {}, deps = []) {
+    // Note that other arguments are checked on the BasePluginManager
+    if (!ctor || !(ctor.prototype instanceof ServerPlugin)) {
+      throw new TypeError(`Cannot execute 'register' on ServerPluginManager: argument 2 must be a class that extends 'ServerPlugin'`);
     }
 
-    if (ctor.target === undefined || ctor.target !== 'server') {
-      throw new TypeError(`Cannot execute 'register' on ServerPluginManager: The plugin class must implement a 'target' static field with value 'server'`);
-    }
-
-    // @todo - check deps
-
     super.register(id, ctor, options, deps);
   }
 

diff --git a/src/server/index.js b/src/server/index.js
@@ -63,4 +63,5 @@
  */
 
 export { default as Server } from './Server.js';
+export { default as ServerPlugin } from './ServerPlugin.js';
 export { default as ServerContext } from './ServerContext.js';
diff --git a/tests/essentials/Client.spec.js b/tests/essentials/Client.spec.js
@@ -2,13 +2,13 @@ import { assert }  from 'chai';
 import merge from 'lodash/merge.js';
 import { delay } from '@ircam/sc-utils';
 
-import { Server, ServerContext } from '../../src/server/index.js';
-import { Client, ClientContext } from '../../src/client/index.js';
+import { Server, ServerContext, ServerPlugin } from '../../src/server/index.js';
+import { Client, ClientContext, ClientPlugin } from '../../src/client/index.js';
 import {
   kClientOnStatusChangeCallbacks,
 } from '../../src/client/Client.js';
 
-import pluginDelayClient from '../utils/PluginDelayClient.js';
+import ClientPluginDelay from '../utils/ClientPluginDelay.js';
 import config from '../utils/config.js';
 
 describe('# Client', () => {
@@ -104,7 +104,7 @@ describe('# Client', () => {
       await server.start();
 
       const client = new Client({ role: 'test', ...config });
-      client.pluginManager.register('unknown-server-side', pluginDelayClient, { delayTime: 0 });
+      client.pluginManager.register('unknown-server-side', ClientPluginDelay, { delayTime: 0 });
 
       let errored = false;
       try {
@@ -275,11 +275,7 @@ describe('# 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 {
-          static target = 'server';
-        }
-      });
+      server.pluginManager.register('test-plugin', class TestPlugin extends ServerPlugin {});
 
       await server.init();
 
@@ -297,24 +293,21 @@ describe('# Client', () => {
 
       const client = new Client({ role: 'test', ...config });
 
-      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
-            // with some on-going processcf. sync
-            this._intervalId = setInterval(() => {}, 500);
-          }
-          async stop() {
-            await super.stop();
-
-            clearInterval(this._intervalId);
-            pluginStop = true;
-            // should be called context.stop()
-            counter += 1;
-            assert.equal(counter, 2);
-          }
+      client.pluginManager.register('test-plugin', class TestPlugin extends ClientPlugin {
+        async start() {
+          await super.start();
+          // just check that we are ok with that, and that we are not stuck
+          // with some on-going processcf. sync
+          this._intervalId = setInterval(() => {}, 500);
+        }
+        async stop() {
+          await super.stop();
+
+          clearInterval(this._intervalId);
+          pluginStop = true;
+          // should be called context.stop()
+          counter += 1;
+          assert.equal(counter, 2);
         }
       });
 

diff --git a/tests/essentials/Server.spec.js b/tests/essentials/Server.spec.js
@@ -11,8 +11,8 @@ import dotenv from 'dotenv';
 import merge from 'lodash.merge';
 import tcpp from 'tcp-ping';
 
-import { Server, ServerContext } from '../../src/server/index.js';
-import { Client } from '../../src/client/index.js';
+import { Server, ServerContext, ServerPlugin } from '../../src/server/index.js';
+import { Client, ClientPlugin } from '../../src/client/index.js';
 
 import {
   kServerOnStatusChangeCallbacks,
@@ -439,26 +439,22 @@ describe('# Server', () => {
       let contextStop = false;
 
       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
-            // with some on-going processcf. sync
-            this._intervalId = setInterval(() => {}, 500);
-          }
-          // should be called after context.stop()
-          async stop() {
-            await super.stop();
+      server.pluginManager.register('test-plugin', class TestPlugin extends ServerPlugin {
+        async start() {
+          await super.start();
+          // just check that we are ok with that, and that we are not stuck
+          // with some on-going process, cf. sync
+          this._intervalId = setInterval(() => {}, 500);
+        }
+        // should be called after context.stop()
+        async stop() {
+          await super.stop();
 
-            clearInterval(this._intervalId);
+          clearInterval(this._intervalId);
 
-            counter += 1;
-            pluginStop = true;
-            assert.equal(counter, 2);
-          }
+          counter += 1;
+          pluginStop = true;
+          assert.equal(counter, 2);
         }
       });
       await server.init();
@@ -474,6 +470,7 @@ describe('# Server', () => {
           assert.equal(counter, 1);
         }
       }
+
       const serverTestContext = new ServerTestContext(server, 'test');
 
       // we call start after creating the context, so it is started