Repository: finos/git-proxy

Issue:
Improve plugin loading functionality
**Is your feature request related to a problem? Please describe.**
After the TS + ESM refactor PRs, we've been having all kinds of issues with the `plugin-loader` library. The runtime errors were fixed by using `tsx` for running the app (`"server": "tsx index.ts",`), and the test errors were fixed by using `ts-mocha` to run the tests. However, there are still issues when loading plugins that are written in ESM: #1088 

Based on the discussion there, we should probably look for a way to remove the `load-plugin` library entirely and replace with something without TS/ESM compatibility issues.

Discussion:
@coopernetes: FWIW here's the internal patch I've applied to get the plugin loader to work with the current project's build & publishing process wrt modules & the emitted JS. This would resolve #1088 which I determined was frankly not worth pursuing except as a back port into 1.x.

If plugins are desired to be supported directly within Node via this mechanism, we still have to deal with this issue even if the underlying library is removed as it's [incredibly simple & minimal](https://github.com/wooorm/load-plugin/blob/main/lib/index.js).

I guess what I'm trying to say is - is this plugin mechanism worth continuing to support or are there other ways of providing extensibility that isn't so invasive? I'm open to suggestions but have none to offer at this time since I have a working implementation with the current design.

```typescript
/**
 * Registers and loads plugins used by git-proxy
 */
class PluginLoader {
  targets: string[];
  pushPlugins: PushActionPlugin[];
  pullPlugins: PullActionPlugin[];
  resolvePlugin: ((moduleSpecifier: string) => Promise<string>) | undefined;
  loadPlugin: ((module: string) => Promise<unknown>) | undefined;

...
  /**
   * Resolve & load a Node module from either a given specifier (file path, import specifier or package name) using load-plugin.
   * @param {string} target The module specifier to load
   * @return {Promise<Module>} A resolved & loaded Module
   */
  private async _loadPluginModule(target: string): Promise<any> {
    if (this.loadPlugin === undefined || this.resolvePlugin == undefined) {
      // The reason this exists is because the project keeps emitting CommonJS require statements
      // and we need to use dynamic import to load this ESM-only module for plugin loading.
      // When cjs is ditched, this can be removed and replaced with a simple `import { loadPlugin, resolvePlugin } from 'load-plugin';`
      const dynamicImport = new Function('specifier', 'return import(specifier)');
      const { loadPlugin, resolvePlugin } = await dynamicImport('load-plugin');
      this.loadPlugin = loadPlugin;
      this.resolvePlugin = resolvePlugin;
    }
    const resolvedModuleFile = await this.resolvePlugin!(target);
    return await this.loadPlugin!(resolvedModuleFile);
  }
```

Working both for pull and push plugins:

```
Service Listening on 8080
_userSettings during import: null
Configuration sources are disabled
Found 4 plugin modules
found pull plugin RunOnPullPlugin
found push plugin PushActionPlugin
found push plugin LogRequestPlugin
found push plugin GithubPullRequestPlugin
found push plugin OnboardValidatePlugin
Loaded plugin: PushActionPlugin
Loaded plugin: LogRequestPlugin
Loaded plugin: GithubPullRequestPlugin
Loaded plugin: OnboardValidatePlugin
Loaded plugin: RunOnPullPlugin
HTTP Proxy Listening on 8000
```@jescalada: Hi @coopernetes, just wanted to confirm whether the latest `PluginLoader` test fix also fixes this issue!@jescalada: Closing this to shrink the backlog - feel free to reopen if the issue hasn't been fully fixed!

Solution patch:
diff --git a/package-lock.json b/package-lock.json
index 4e2549c3..bbb0085e 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -55,7 +55,7 @@
         "yargs": "^17.7.2"
       },
       "bin": {
-        "git-proxy": "index.js",
+        "git-proxy": "dist/index.js",
         "git-proxy-all": "concurrently 'npm run server' 'npm run client'"
       },
       "devDependencies": {
diff --git a/package.json b/package.json
index c0f2bb29..56c5679d 100644
--- a/package.json
+++ b/package.json
@@ -29,6 +29,11 @@
       "import": "./dist/src/types/models.js",
       "require": "./dist/src/types/models.js",
       "types": "./dist/src/types/models.d.ts"
+    },
+    "./plugin": {
+      "import": "./dist/src/plugin.js",
+      "require": "./dist/src/plugin.js",
+      "types": "./dist/src/plugin.d.ts"
     }
   },
   "scripts": {



Test patch:
diff --git a/test/plugin/plugin.test.js b/test/plugin/plugin.test.js
index d6128749..c2692913 100644
--- a/test/plugin/plugin.test.js
+++ b/test/plugin/plugin.test.js
@@ -22,42 +22,34 @@ describe('loading plugins from packages', function () {
     spawnSync('npm', ['install'], { cwd: testPackagePath, timeout: 5000 });
   });
 
-  it.skip(
-    'should load plugins that are the default export (module.exports = pluginObj)',
-    async function () {
-      const loader = new PluginLoader([join(testPackagePath, 'default-export.js')]);
-      await loader.load();
-      expect(loader.pushPlugins.length).to.equal(1);
-      expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p))).to.be.true;
-      expect(loader.pushPlugins[0]).to.be.an.instanceOf(PushActionPlugin);
-    },
-  ).timeout(10000);
+  it('should load plugins that are the default export (module.exports = pluginObj)', async function () {
+    const loader = new PluginLoader([join(testPackagePath, 'default-export.js')]);
+    await loader.load();
+    expect(loader.pushPlugins.length).to.equal(1);
+    expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p))).to.be.true;
+    expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p, 'isGitProxyPushActionPlugin'))).to
+      .be.true;
+  }).timeout(10000);
 
-  it.skip(
-    'should load multiple plugins from a module that match the plugin class (module.exports = { pluginFoo, pluginBar })',
-    async function () {
-      const loader = new PluginLoader([join(testPackagePath, 'multiple-export.js')]);
-      await loader.load();
-      expect(loader.pushPlugins.length).to.equal(1);
-      expect(loader.pullPlugins.length).to.equal(1);
-      expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p))).to.be.true;
-      expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p, 'isGitProxyPushActionPlugin')))
-        .to.be.true;
-      expect(loader.pullPlugins.every((p) => isCompatiblePlugin(p, 'isGitProxyPullActionPlugin')))
-        .to.be.true;
-      expect(loader.pushPlugins[0]).to.be.instanceOf(PushActionPlugin);
-      expect(loader.pullPlugins[0]).to.be.instanceOf(PullActionPlugin);
-    },
-  ).timeout(10000);
+  it('should load multiple plugins from a module that match the plugin class (module.exports = { pluginFoo, pluginBar })', async function () {
+    const loader = new PluginLoader([join(testPackagePath, 'multiple-export.js')]);
+    await loader.load();
+    expect(loader.pushPlugins.length).to.equal(1);
+    expect(loader.pullPlugins.length).to.equal(1);
+    expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p))).to.be.true;
+    expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p, 'isGitProxyPushActionPlugin'))).to
+      .be.true;
+    expect(loader.pullPlugins.every((p) => isCompatiblePlugin(p, 'isGitProxyPullActionPlugin'))).to
+      .be.true;
+  }).timeout(10000);
 
-  it.skip('should load plugins that are subclassed from plugin classes', async function () {
+  it('should load plugins that are subclassed from plugin classes', async function () {
     const loader = new PluginLoader([join(testPackagePath, 'subclass.js')]);
     await loader.load();
     expect(loader.pushPlugins.length).to.equal(1);
     expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p))).to.be.true;
     expect(loader.pushPlugins.every((p) => isCompatiblePlugin(p, 'isGitProxyPushActionPlugin'))).to
       .be.true;
-    expect(loader.pushPlugins[0]).to.be.instanceOf(PushActionPlugin);
   }).timeout(10000);
 
   it('should not load plugins that are not valid modules', async function () {