Repository: artusjs/core
Pull request #256 (2023-07-10 10:18:01)

fix(plugin): optional plugin also need to calc order
此前的设计中，因为拓扑排序阶段的所有节点（边的端点）都需要有效且存在，所以 optional 依赖（可能不存在）会在边列表中全部排除，不参与顺序计算；

但即便是 optional 的依赖，也可能排在当前插件后面，不进行顺序计算将带来非预期行为（如奇怪的容器内元素不存在报错，插件开了，因为顺序不对拿不到）；

所以在依赖检查环节补充本 MR 的逻辑，对于已开启并存在的 optional 依赖，也要参与边生成/算序环节


Patch:
diff --git a/src/plugin/impl.ts b/src/plugin/impl.ts
index 900ddc6..fe42bbc 100644
--- a/src/plugin/impl.ts
+++ b/src/plugin/impl.ts
@@ -51,7 +51,12 @@ export class Plugin implements PluginType {
   }
 
   public checkDepExisted(pluginMap: PluginMap) {
-    for (const { name: pluginName, optional } of this.metadata.dependencies ?? []) {
+    if (!this.metadata.dependencies) {
+      return;
+    }
+
+    for (let i = 0; i < this.metadata.dependencies.length; i++) {
+      const { name: pluginName, optional } = this.metadata.dependencies[i];
       const instance = pluginMap.get(pluginName);
       if (!instance || !instance.enable) {
         if (optional) {
@@ -59,13 +64,16 @@ export class Plugin implements PluginType {
         } else {
           throw new Error(`Plugin ${this.name} need have dependency: ${pluginName}.`);
         }
+      } else {
+        // Plugin exist and enabled, need calc edge
+        this.metadata.dependencies[i]._enabled = true;
       }
     }
   }
 
   public getDepEdgeList(): [string, string][] {
     return this.metadata.dependencies
-      ?.filter(({ optional }) => !optional)
+      ?.filter(({ optional, _enabled }) => !optional || _enabled)
       ?.map(({ name: depPluginName }) => [this.name, depPluginName]) ?? [];
   }
 
diff --git a/src/plugin/types.ts b/src/plugin/types.ts
index 644a1f2..b271b07 100644
--- a/src/plugin/types.ts
+++ b/src/plugin/types.ts
@@ -15,6 +15,9 @@ export interface PluginMetadata {
 export interface PluginDependencyItem {
   name: string;
   optional?: boolean;
+
+  // Only exist on runtime, cannot config in meta.json
+  _enabled?: boolean;
 }
 
 export interface PluginConfigItem {


Test patch:
diff --git a/test/__snapshots__/scanner.test.ts.snap b/test/__snapshots__/scanner.test.ts.snap
index bc9ab15..c18bca0 100644
--- a/test/__snapshots__/scanner.test.ts.snap
+++ b/test/__snapshots__/scanner.test.ts.snap
@@ -278,7 +278,7 @@ Object {
       "pluginMetadata": Object {
         "dependencies": Array [
           Object {
-            "name": "plugin-e",
+            "name": "plugin-c",
             "optional": true,
           },
         ],
@@ -508,7 +508,7 @@ Object {
       "pluginMetadata": Object {
         "dependencies": Array [
           Object {
-            "name": "plugin-e",
+            "name": "plugin-c",
             "optional": true,
           },
         ],
diff --git a/test/fixtures/plugins/plugin_d/meta.json b/test/fixtures/plugins/plugin_d/meta.json
index 00741ac..d1a8da7 100644
--- a/test/fixtures/plugins/plugin_d/meta.json
+++ b/test/fixtures/plugins/plugin_d/meta.json
@@ -2,7 +2,7 @@
   "name": "plugin-d",
   "dependencies": [
       {
-          "name": "plugin-e",
+          "name": "plugin-c",
           "optional": true
       }
   ]
diff --git a/test/plugin.test.ts b/test/plugin.test.ts
index 64fca01..42ed5bc 100644
--- a/test/plugin.test.ts
+++ b/test/plugin.test.ts
@@ -201,10 +201,91 @@ describe('test/app.test.ts', () => {
         expect(plugin).toBeInstanceOf(Plugin);
         expect(plugin.enable).toBeTruthy();
       });
-      expect(mockWarnFn).toBeCalledWith(`Plugin plugin-d need have optional dependency: plugin-e.`);
+      expect(mockWarnFn).toBeCalledWith(`Plugin plugin-d need have optional dependency: plugin-c.`);
 
       // restore warn
       console.warn = originWarn;
     });
+
+    it('should not throw if optional dependence disabled', async () => {
+      const mockPluginConfig = {
+        'plugin-c': {
+          enable: false,
+          path: path.resolve(__dirname, `${pluginPrefix}/plugin_c`),
+          manifest: {
+            pluginMeta: {
+              path: path.resolve(__dirname, `${pluginPrefix}/plugin_c/meta.js`),
+              extname: '.js',
+              filename: 'meta.js',
+            },
+          },
+        },
+        'plugin-d': {
+          enable: true,
+          path: path.resolve(__dirname, `${pluginPrefix}/plugin_d`),
+          manifest: {
+            pluginMeta: {
+              path: path.resolve(__dirname, `${pluginPrefix}/plugin_d/meta.js`),
+              extname: '.js',
+              filename: 'meta.js',
+            },
+          },
+        },
+      };
+
+      // mock warn
+      const originWarn = console.warn;
+      const mockWarnFn = jest.fn();
+      console.warn = mockWarnFn;
+      const pluginList = await PluginFactory.createFromConfig(mockPluginConfig, {
+        logger: new Logger(),
+      });
+      expect(pluginList.length).toEqual(1);
+      pluginList.forEach(plugin => {
+        expect(plugin).toBeInstanceOf(Plugin);
+        expect(plugin.enable).toBeTruthy();
+      });
+      expect(mockWarnFn).toBeCalledWith(`Plugin plugin-d need have optional dependency: plugin-c.`);
+
+      // restore warn
+      console.warn = originWarn;
+    });
+
+    it('should calc order if optional dependence enabled', async () => {
+      const mockPluginConfig = {
+        'plugin-d': {
+          enable: true,
+          path: path.resolve(__dirname, `${pluginPrefix}/plugin_d`),
+          manifest: {
+            pluginMeta: {
+              path: path.resolve(__dirname, `${pluginPrefix}/plugin_d/meta.js`),
+              extname: '.js',
+              filename: 'meta.js',
+            },
+          },
+        },
+        'plugin-c': {
+          enable: true,
+          path: path.resolve(__dirname, `${pluginPrefix}/plugin_c`),
+          manifest: {
+            pluginMeta: {
+              path: path.resolve(__dirname, `${pluginPrefix}/plugin_c/meta.js`),
+              extname: '.js',
+              filename: 'meta.js',
+            },
+          },
+        },
+      };
+
+      const pluginList = await PluginFactory.createFromConfig(mockPluginConfig, {
+        logger: new Logger(),
+      });
+      expect(pluginList.length).toEqual(2);
+      pluginList.forEach(plugin => {
+        expect(plugin).toBeInstanceOf(Plugin);
+        expect(plugin.enable).toBeTruthy();
+      });
+      expect(pluginList.map(plugin => plugin.name)).toStrictEqual(['plugin-c', 'plugin-d']);
+    });
   });
 });