refactor(plugin): use standard dfs instead of topological for stable sort (#262)

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

* refactor(plugin): use recursively dfs to stable sort result & add test case

Author: noahziheng

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@artus/core",
-  "version": "2.0.5",
+  "version": "2.1.0",
   "description": "Core package of Artus",
   "main": "./lib/index.js",
   "types": "./lib/index.d.ts",

src/plugin/common.ts

@@ -1,62 +1,86 @@
-import path from 'path';
-import compatibleRequire from '../utils/compatible_require';
-import { PluginType } from './types';
+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,
+): PluginType[] {
+  const sortedPlugins: PluginType[] = [];
+  const visited: Record<string, boolean> = {};
 
-  pluginDepEdgeList.forEach(([to]) => {
-    indegree.set(to, (indegree.get(to) ?? 0) + 1);
-  });
+  const visit = (pluginName: string, depChain: string[] = []) => {
+    if (depChain.includes(pluginName)) {
+      throw new Error(
+        `Circular dependency found in plugins: ${depChain.join(", ")}`,
+      );
+    }
 
-  const queue: string[] = [];
+    if (visited[pluginName]) return;
 
-  for (const [name] of pluginInstanceMap) {
-    if (!indegree.has(name)) {
-      queue.push(name);
-    }
-  }
+    visited[pluginName] = 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);
+    const plugin = pluginInstanceMap.get(pluginName);
+    if (plugin) {
+      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 {
+          // Plugin exist and enabled, need visit
+          visit(dep.name, depChain.concat(pluginName));
         }
       }
+      sortedPlugins.push(plugin);
     }
+  };
+
+  for (const pluginName of pluginInstanceMap.keys()) {
+    visit(pluginName);
   }
-  return res;
+
+  return sortedPlugins;
 }
 
 // A util function of get package path for plugin
-export function getPackagePath(packageName: string, paths: string[] = []): string {
+export function getPackagePath(
+  packageName: string,
+  paths: string[] = [],
+): string {
   const opts = {
     paths: paths.concat(__dirname),
   };
   return path.dirname(require.resolve(packageName, opts));
 }
 
-export async function getInlinePackageEntryPath(packagePath: string): Promise<string> {
+export async function getInlinePackageEntryPath(
+  packagePath: string,
+): Promise<string> {
   const pkgJson = await compatibleRequire(`${packagePath}/package.json`);
-  let entryFilePath = '';
+  let entryFilePath = "";
   if (pkgJson.exports) {
     if (Array.isArray(pkgJson.exports)) {
       throw new Error(`inline package multi exports is not supported`);
-    } else if (typeof pkgJson.exports === 'string') {
+    } else if (typeof pkgJson.exports === "string") {
       entryFilePath = pkgJson.exports;
-    } else if (pkgJson.exports?.['.']) {
-      entryFilePath = pkgJson.exports['.'];
+    } else if (pkgJson.exports?.["."]) {
+      entryFilePath = pkgJson.exports["."];
     }
   }
   if (!entryFilePath && pkgJson.main) {
     entryFilePath = pkgJson.main;
   }
   // will use package root path if no entry file found
-  return entryFilePath ? path.resolve(packagePath, entryFilePath, '..') : packagePath;
+  return entryFilePath
+    ? path.resolve(packagePath, entryFilePath, "..")
+    : packagePath;
 }

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][];
 }