refactor(plugin): use standard dfs instead of topological for stable …

…sort

src/plugin/common.ts

@@ -1,37 +1,54 @@
 import path from 'path';
 import compatibleRequire from '../utils/compatible_require';
 import { PluginType } from './types';
+import { LoggerType } from '../logger';
 
-// A utils function that toplogical sort plugins
-export function topologicalSort(pluginInstanceMap: Map<string, PluginType>, pluginDepEdgeList: [string, string][]): string[] {
-  const res: string[] = [];
-  const indegree: Map<string, number> = new Map();
+export function sortPlugins(pluginInstanceMap: Map<string, PluginType>, logger: LoggerType) {
+  const sortedPlugins: PluginType[] = [];
+  const visited: Record<string, boolean> = {};
 
-  pluginDepEdgeList.forEach(([to]) => {
-    indegree.set(to, (indegree.get(to) ?? 0) + 1);
-  });
+  while (sortedPlugins.length < pluginInstanceMap.size) {
+    let added = false;
 
-  const queue: string[] = [];
+    for (const [pluginName, plugin] of pluginInstanceMap) {
+      if (visited[pluginName]) {
+        continue;
+      }
+
+      let depsSatisfied = true;
+      for (const dep of plugin.metadata.dependencies ?? []) {
+        const depPlugin = pluginInstanceMap.get(dep.name);
+        if (!depPlugin || !depPlugin.enable) {
+          if (dep.optional) {
+            logger?.warn(`Plugin ${plugin.name} need have optional dependency: ${dep.name}.`);
+          } else {
+            throw new Error(`Plugin ${plugin.name} need have dependency: ${dep.name}.`);
+          }
+        } else if (!visited[dep.name]) { // Plugin exist and enabled, need check visited
+          depsSatisfied = false;
+        }
+      }
 
-  for (const [name] of pluginInstanceMap) {
-    if (!indegree.has(name)) {
-      queue.push(name);
+      if (depsSatisfied) {
+        sortedPlugins.push(plugin);
+        visited[plugin.name] = true;
+        added = true;
+      }
     }
-  }
 
-  while(queue.length) {
-    const cur = queue.shift()!;
-    res.push(cur);
-    for (const [to, from] of pluginDepEdgeList) {
-      if (from === cur) {
-        indegree.set(to, (indegree.get(to) ?? 0) - 1);
-        if (indegree.get(to) === 0) {
-          queue.push(to);
+    if (!added) {
+      const cyclePluginNames: string[] = [];
+      const sortedPluginSet = new Set(sortedPlugins.map(p => p.name));
+      for (const pluginName of pluginInstanceMap.keys()) {
+        if (!sortedPluginSet.has(pluginName)) {
+          cyclePluginNames.push(pluginName);
         }
       }
+      throw new Error(`Circular dependency found in plugins: ${cyclePluginNames.join(', ')}`);
     }
   }
-  return res;
+
+  return sortedPlugins;
 }
 
 // A util function of get package path for plugin

src/plugin/factory.ts

@@ -1,33 +1,17 @@
-import { topologicalSort } from './common';
+import { sortPlugins } from './common';
 import { Plugin } from './impl';
 import { PluginConfigItem, PluginCreateOptions, PluginMap, PluginType } from './types';
 
 export class PluginFactory {
-  static async create(name: string, item: PluginConfigItem, opts?: PluginCreateOptions): Promise<PluginType> {
-    const pluginInstance = new Plugin(name, item, opts);
-    await pluginInstance.init();
-    return pluginInstance;
-  }
-
   static async createFromConfig(config: Record<string, PluginConfigItem>, opts?: PluginCreateOptions): Promise<PluginType[]> {
     const pluginInstanceMap: PluginMap = new Map();
     for (const [name, item] of Object.entries(config)) {
-      const pluginInstance = await PluginFactory.create(name, item, opts);
-      if (pluginInstance.enable) {
+      if (item.enable) {
+        const pluginInstance = new Plugin(name, item);
+        await pluginInstance.init();
         pluginInstanceMap.set(name, pluginInstance);
       }
     }
-    let pluginDepEdgeList: [string, string][] = [];
-    // Topological sort plugins
-    for (const [_name, pluginInstance] of pluginInstanceMap) {
-      pluginInstance.checkDepExisted(pluginInstanceMap);
-      pluginDepEdgeList = pluginDepEdgeList.concat(pluginInstance.getDepEdgeList());
-    }
-    const pluginSortResult: string[] = topologicalSort(pluginInstanceMap, pluginDepEdgeList);
-    if (pluginSortResult.length !== pluginInstanceMap.size) {
-      const diffPlugin = [...pluginInstanceMap.keys()].filter(name => !pluginSortResult.includes(name));
-      throw new Error(`There is a cycle in the dependencies, wrong plugin is ${diffPlugin.join(',')}.`);
-    }
-    return pluginSortResult.map(name => pluginInstanceMap.get(name)!);
+    return sortPlugins(pluginInstanceMap, opts?.logger);
   }
 }

src/plugin/impl.ts

@@ -2,9 +2,8 @@ import path from 'path';
 import { loadMetaFile } from '../utils/load_meta_file';
 import { exists } from '../utils/fs';
 import { PLUGIN_META_FILENAME } from '../constant';
-import { PluginConfigItem, PluginCreateOptions, PluginMap, PluginMetadata, PluginType } from './types';
+import { PluginConfigItem, PluginMetadata, PluginType } from './types';
 import { getPackagePath } from './common';
-import { LoggerType } from '../logger';
 
 export class Plugin implements PluginType {
   public name: string;
@@ -13,9 +12,7 @@ export class Plugin implements PluginType {
   public metadata: Partial<PluginMetadata>;
   public metaFilePath = '';
 
-  private logger?: LoggerType;
-
-  constructor(name: string, configItem: PluginConfigItem, opts?: PluginCreateOptions) {
+  constructor(name: string, configItem: PluginConfigItem) {
     this.name = name;
     this.enable = configItem.enable ?? false;
     if (this.enable) {
@@ -34,10 +31,9 @@ export class Plugin implements PluginType {
     if (configItem.metadata) {
       this.metadata = configItem.metadata;
     }
-    this.logger = opts?.logger;
   }
 
-  async init() {
+  public async init() {
     if (!this.enable) {
       return;
     }
@@ -50,33 +46,6 @@ export class Plugin implements PluginType {
     }
   }
 
-  public checkDepExisted(pluginMap: PluginMap) {
-    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) {
-          this.logger?.warn(`Plugin ${this.name} need have optional dependency: ${pluginName}.`);
-        } 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, _enabled }) => !optional || _enabled)
-      ?.map(({ name: depPluginName }) => [this.name, depPluginName]) ?? [];
-  }
-
   private async checkAndLoadMetadata() {
     // check metadata from configItem
     if (this.metadata) {

src/plugin/types.ts

@@ -39,6 +39,4 @@ export interface PluginType {
   metaFilePath: string;
 
   init(): Promise<void>;
-  checkDepExisted(map: PluginMap): void;
-  getDepEdgeList(): [string, string][];
 }

test/plugin.test.ts

@@ -4,7 +4,7 @@ import { Logger, Plugin, PluginFactory } from '../src';
 
 const pluginPrefix = 'fixtures/plugins';
 
-describe('test/app.test.ts', () => {
+describe('test/plugin.test.ts', () => {
   describe('app with config', () => {
     it('should load plugin with dep order', async () => {
       const mockPluginConfig = {
@@ -111,7 +111,7 @@ describe('test/app.test.ts', () => {
       };
       expect(async () => {
         await PluginFactory.createFromConfig(mockPluginConfig);
-      }).rejects.toThrowError(new Error(`There is a cycle in the dependencies, wrong plugin is plugin-wrong-a,plugin-wrong-b.`));
+      }).rejects.toThrowError(new Error(`Circular dependency found in plugins: plugin-wrong-a, plugin-wrong-b`));
     });
 
     it('should throw if dependencies missing', async () => {