Repository: artusjs/core
Pull request #129 (2022-07-08 08:42:42)

chore: plugin has own excluded

wengeezhang: 背景：
用户如果设置了excluded属性，这个值会被应用到插件扫描过程中。最终结果是，有的插件目录可能没法被扫描。

因此，这里做下调整，尊重插件的excluded：

* 如果plugin有声明excluded，则用default excluded + plugin excluded
* 如果plugin没有，则用default excluded + 用户 excluded
wengeezhang: 还有一种策略：
* 如果plugin有声明excluded，则用default excluded + plugin excluded
* 如果plugin没有，则用default excluded

这种情况下，完全尊重plugin的设置，隔离用户设置带来的污染
hyj1991: > 如果plugin没有，则用default excluded + 用户 excluded

这个策略下用户的 exclude 还是会带来问题吧？exclude 其实不同于 `config.xxx.ts` 这样的配置，我觉得用户不需要（其实也无法）去关注使用的插件需要忽略哪些目录，这个就应该是插件 / 框架自身的一个属性
hyj1991: 用户只要关注应用代码需要忽略哪些即可
wengeezhang: 
> 这个策略下用户的 exclude 还是会带来问题吧？exclude 其实不同于 `config.xxx.ts` 这样的配置，我觉得用户不需要（其实也无法）去关注使用的插件需要忽略哪些目录，这个就应该是插件 / 框架自身的一个属性

下面的策略，是不是你的意思：

* 如果plugin有声明excluded，则用default excluded + plugin excluded
* 如果plugin没有，则用default excluded
hyj1991: 对，我在想 `exclude` 属于 app / framework / plugin 自身属性的话，其实用户配置只应该影响到 app，这个点需要简单确认下

cc @noahziheng  @DuanPengfei 
hyj1991: 另外是，现在的 `exclude` 貌似是走的文件（夹）名匹配，这是否意味着用户自己的 `controller` 目录之类的也无法定义 `exclude` 中的名称目录？
noahziheng: > 我在想 `exclude` 属于 app / framework / plugin 自身属性的话，其实用户配置只应该影响到 app，这个点需要简单确认下

确实对于 `exclude` 来说，有作用域属性，但也有一些文件可能对于用户来说是需要批量忽略的，如内联插件中可能出现的根目录 `index.js` 或 `pnpm-lock.yaml`；

这些在 DEFAULT_EXCLUDE 中可能很难彻底覆盖，对于扫描参数中的 `exclude` 感觉更应该是贯穿整个扫描过程的，对于 app 中的需要忽略的项需要单独找个地方作为元信息声明（类同 `meta.yaml` 中增加的 `exclude`）？

> 另外是，现在的 exclude 貌似是走的文件（夹）名匹配，这是否意味着用户自己的 controller 目录之类的也无法定义 exclude 中的名称目录？

这个事情我没理解，貌似现在的格式是 glob，此例应该是 `controller/**/*`？
wengeezhang: > 另外是，现在的 `exclude` 貌似是走的文件（夹）名匹配，这是否意味着用户自己的 `controller` 目录之类的也无法定义 `exclude` 中的名称目录？

目前看这个能力是有的，现在走的是minimatch方式来判断
hyj1991: > 这个事情我没理解，貌似现在的格式是 glob，此例应该是 controller/**/*？

不是，按现在的规则，比如 `exclude` 定义了 `test`，那么 `app/controller/test` 这种用户其实非 `test` 目录也会被忽略掉，这是否是一个 bug？
JerrysShan: 应该算一个bug 吧 ，是否把 `test` 规则改为 `test/`
wengeezhang: > > 这个事情我没理解，貌似现在的格式是 glob，此例应该是 controller/**/*？
> 
> 不是，按现在的规则，比如 `exclude` 定义了 `test`，那么 `app/controller/test` 这种用户其实非 `test` 目录也会被忽略掉，这是否其实不符合预期？

如果改成了方案二，其实这种场景可以当做“特性”来处理，而非bug，是可以接受的。
hyj1991: > 如果改成了方案二，其实这种场景可以当做“特性”来处理，而非bug，是可以接受的。

这样其实也讲得通，譬如 `.gitignore` 中的规则和我们现在的 `exclude` 是一致的，不过我们是否应该也提供和 `.gitignore` 类似的 `!app/controller/test` 这样的规则来保证出现误判时用户有办法将实际不需要 exclude 的文件继续扫描进来

至于本 issue 的 fix 方案，我个人比较倾向方案二，plugin 的编写和 app 是分离的，用户的 exclude 配置不应该影响到 plugin 的文件
wengeezhang: 最新改动：
* framework新增meta.yaml逻辑，和plugin保持一致
* 目前plugin和framework中的meta中声明的excluded和configDir优先级高
  * 如果没有设置，则使用默认值，不使用用户的
* plugin和framework中meta.yaml暂时不支持extensions，即extensions始终以用户声明为准
  * 这块可以后续看看是否有必要再调整
  
@noahziheng @hyj1991 
wengeezhang: > 测试 case 没有体现 plugin 在 meta 中自定义 `exclude` 的例子
样例和单测我补一下



Patch:
diff --git a/src/constant.ts b/src/constant.ts
index 59a92bd..2242e86 100644
--- a/src/constant.ts
+++ b/src/constant.ts
@@ -43,6 +43,8 @@ export const DEFAULT_EXCLUDES = [
   'LICENSE'
 ];
 
+export const DEFAULT_EXTENSIONS = ['.js', '.json', '.node', '.yaml'];
+
 export const FRAMEWORK_PATTERN = 'framework.*';
 export const PLUGIN_CONFIG_PATTERN = 'plugin.*';
 export const CONFIG_PATTERN = 'config.*';
diff --git a/src/framework/handler.ts b/src/framework/handler.ts
index 7c3237e..1275d91 100644
--- a/src/framework/handler.ts
+++ b/src/framework/handler.ts
@@ -1,7 +1,8 @@
 import path from 'path';
 import ConfigurationHandler, { ConfigObject } from '../configuration';
-import { ManifestItem } from '../types';
-
+import { ManifestItem, Metadata } from '../types';
+import { exisis } from '../utils/fs';
+import { loadMetaFile } from '../utils/load_meta_file';
 export interface FrameworkConfig {
   path?: string,
   package?: string
@@ -33,4 +34,40 @@ export class FrameworkHandler {
       throw new Error(`load framework faild: ${err}, framework config: ${JSON.stringify(config)}`);
     }
   }
+  static async checkAndLoadMetadata(frameworkDir: string): Promise<Metadata>{
+    // check import path
+    if (!await exisis(frameworkDir)) {
+      throw new Error(`load framework import path ${frameworkDir} is not exists.`);
+    }
+
+    let find = false;
+    const fileNameList = [
+      'meta.yaml',
+      'meta.yml',
+      'meta.json',
+    ];
+    let metadata;
+    for (const fileName of fileNameList) {
+      const metaFilePath = path.resolve(frameworkDir, fileName);
+      try {
+        if (!await exisis(metaFilePath)) {
+          continue;
+        }
+        metadata = await loadMetaFile({
+          path: metaFilePath,
+          extname: path.extname(metaFilePath),
+          filename: fileName,
+        });
+        find = true;
+        break;
+      } catch (e) {
+        throw new Error(`load framework metadata <${frameworkDir}> failed, err: ${e}`);
+      }
+    }
+
+    if (!find) {
+      throw new Error(`load framework import path ${frameworkDir} can't find meta file.`);
+    }
+    return metadata;
+  }
 }
diff --git a/src/plugin/types.ts b/src/plugin/types.ts
index aeef934..b3b892d 100644
--- a/src/plugin/types.ts
+++ b/src/plugin/types.ts
@@ -1,13 +1,12 @@
+import { Metadata } from '../types';
 export enum PluginType {
   simple = 'simple',
   module = 'module',
 }
 
-export interface PluginMetadata {
-  name: string;
+export interface PluginMetadata extends Metadata {
   dependencies?: PluginDependencyItem[];
   type?: PluginType;
-  configDir?: string
 }
 
 export interface PluginDependencyItem {
diff --git a/src/scanner/scan.ts b/src/scanner/scan.ts
index 762c5df..999a1ad 100644
--- a/src/scanner/scan.ts
+++ b/src/scanner/scan.ts
@@ -7,10 +7,12 @@ import {
   ARTUS_DEFAULT_CONFIG_ENV,
   DEFAULT_CONFIG_DIR,
   DEFAULT_EXCLUDES,
+  DEFAULT_EXTENSIONS,
   DEFAULT_LOADER_LIST_WITH_ORDER,
   LOADER_NAME_META,
 } from '../constant';
 import { LoaderFactory, Manifest, ManifestItem } from '../loader';
+import { Metadata } from '../types';
 import { ScannerOptions, WalkOptions } from './types';
 import ConfigurationHandler, { ConfigObject } from '../configuration';
 import { FrameworkConfig, FrameworkHandler } from '../framework';
@@ -18,7 +20,6 @@ import { BasePlugin, PluginFactory } from '../plugin';
 import { ScanUtils } from './utils';
 
 export class Scanner {
-  private moduleExtensions = ['.js', '.json', '.node'];
   private options: ScannerOptions;
   private itemMap: Map<string, ManifestItem[]> = new Map();
   private tmpConfigStore: Map<string, ConfigObject[]> = new Map();
@@ -32,8 +33,8 @@ export class Scanner {
       configDir: DEFAULT_CONFIG_DIR,
       loaderListGenerator: (defaultLoaderList: string[]) => defaultLoaderList,
       ...options,
-      excluded: DEFAULT_EXCLUDES.concat(options.excluded ?? []),
-      extensions: [...new Set(this.moduleExtensions.concat(options.extensions ?? [], ['.yaml']))],
+      exclude: [...new Set(DEFAULT_EXCLUDES.concat(options.exclude ?? []))],
+      extensions: [...new Set(DEFAULT_EXTENSIONS.concat(options.extensions ?? []))],
     };
   }
 
@@ -94,10 +95,11 @@ export class Scanner {
     // 1. scan all file in framework
     const frameworkDirs = await this.getFrameworkDirs(config.framework, root, env);
     for (const frameworkDir of frameworkDirs) {
-      await this.walk(frameworkDir, this.formatWalkOptions('framework', frameworkDir));
+      const frameworkMetadata = await FrameworkHandler.checkAndLoadMetadata(frameworkDir);
+      const frameworkOptions = this.formatWalkOptions('framework', frameworkDir, frameworkDir, frameworkMetadata);
+      await this.walk(frameworkDir, frameworkOptions);
     }
 
-
     // 2. scan all file in plugin
     if (this.tmpConfigStore.has(env)) {
       const configList = this.tmpConfigStore.get(env) ?? [];
@@ -108,10 +110,8 @@ export class Scanner {
     for (const plugin of pluginSortedList) {
       if (!plugin.enable) continue;
       this.setPluginMeta(plugin);
-      await this.walk(
-        plugin.importPath,
-        this.formatWalkOptions('plugin', plugin.importPath, plugin.name, plugin.metadata.configDir)
-      );
+      const pluginOpts = this.formatWalkOptions('plugin', plugin.importPath, plugin.name, plugin.metadata as Metadata);
+      await this.walk(plugin.importPath, pluginOpts);
     }
 
     // 3. scan all file in app
@@ -150,9 +150,9 @@ export class Scanner {
     const container = new Container(ArtusInjectEnum.DefaultContainerName);
     container.set({ type: ConfigurationHandler });
     const loaderFactory = LoaderFactory.create(container);
-    const configItemList: (ManifestItem|null)[] = await Promise.all(configFileList.map(async filename => {
+    const configItemList: (ManifestItem | null)[] = await Promise.all(configFileList.map(async filename => {
       const extname = path.extname(filename);
-      if (ScanUtils.isExclude(filename, extname, this.options.excluded, this.options.extensions)) {
+      if (ScanUtils.isExclude(filename, extname, this.options.exclude, this.options.extensions)) {
         return null;
       }
       let loader = await loaderFactory.findLoaderName({
@@ -215,22 +215,51 @@ export class Scanner {
     return await this.getFrameworkDirs(configInFramework.framework, frameworkBaseDir, env, dirs);
   }
 
-  private formatWalkOptions(source: string, baseDir: string, unitName?: string, configDir?: string): WalkOptions {
+  private formatWalkOptions(source: string, baseDir: string, unitName?: string, metadata?: Metadata): WalkOptions {
     const commonOptions = {
       extensions: this.options.extensions,
-      excluded: this.options.excluded,
+      exclude: this.options.exclude,
       itemMap: this.itemMap,
     };
 
     unitName ??= baseDir;
-    configDir ??= this.options.configDir;
+    const configDir = this.options.configDir;
 
-    return Object.assign({}, commonOptions, {
+    let result: WalkOptions = Object.assign({}, commonOptions, {
       source,
       baseDir,
       unitName,
       configDir,
     });
+    // metadata takes priority
+    if (metadata) {
+      result = this.amendOptions(result, metadata);
+    }
+    return result;
+  }
+
+  private amendOptions(walkOptions: WalkOptions, metadata): WalkOptions {
+    // plugin/framework exclude take priority over user app's
+    if (metadata?.exclude) {
+      walkOptions.exclude = DEFAULT_EXCLUDES.concat(metadata.exclude);
+    } else {
+      walkOptions.exclude = DEFAULT_EXCLUDES;
+    }
+
+    // plugin/framework extensions take priority over user app's
+    // if (metadata?.extensions) {
+    //   walkOptions.extensions = DEFAULT_EXTENSIONS.concat(metadata.extensions);
+    // } else {
+    //   walkOptions.extensions = DEFAULT_EXTENSIONS;
+    // }
+
+    // plugin/framework configDir take priority over user app's
+    if (metadata?.configDir) {
+      walkOptions.configDir = metadata.configDir;
+    } else {
+      walkOptions.configDir = DEFAULT_CONFIG_DIR;
+    }
+    return walkOptions;
   }
 
   private getItemsFromMap(relative: boolean, appRoot: string): ManifestItem[] {
diff --git a/src/scanner/types.ts b/src/scanner/types.ts
index 38df515..a16247b 100644
--- a/src/scanner/types.ts
+++ b/src/scanner/types.ts
@@ -5,7 +5,7 @@ export interface ScannerOptions {
   extensions: string[];
   needWriteFile: boolean;
   useRelativePath: boolean;
-  excluded: string[];
+  exclude: string[];
   configDir: string;
   envs?: string[];
   loaderListGenerator: (defaultLoaderList: string[]) => (string | typeof BaseLoader)[];
@@ -16,7 +16,7 @@ export interface WalkOptions {
   baseDir: string;
   configDir: string;
   extensions: string[];
-  excluded: string[];
+  exclude: string[];
   itemMap: Map<string, ManifestItem[]>;
   unitName?: string;
 }
diff --git a/src/scanner/utils.ts b/src/scanner/utils.ts
index 20fa7df..a9d7e7e 100644
--- a/src/scanner/utils.ts
+++ b/src/scanner/utils.ts
@@ -32,7 +32,7 @@ export class ScanUtils {
     for (const item of items) {
       const realPath = path.resolve(root, item);
       const extname = path.extname(realPath);
-      if (this.isExclude(item, extname, options.excluded, options.extensions)) {
+      if (this.isExclude(item, extname, options.exclude, options.extensions)) {
         continue;
       }
       const itemStat = await fs.stat(realPath);
@@ -78,10 +78,10 @@ export class ScanUtils {
   }
 
   static isExclude(filename: string, extname: string,
-    excluded: string[], extensions: string[]): boolean {
+    exclude: string[], extensions: string[]): boolean {
     let result = false;
-    if (!result && excluded) {
-      result = isMatch(filename, excluded);
+    if (exclude) {
+      result = isMatch(filename, exclude);
     }
 
     if (!result && extname) {
diff --git a/src/types.ts b/src/types.ts
index 7e4760e..0682f39 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -35,5 +35,11 @@ export interface TriggerType {
   initContext(...args): Promise<BaseContext>;
   startPipeline(...args): Promise<void>;
 }
+export interface Metadata {
+  name: string;
+  exclude?: string[];
+  extensions?: string[];
+  configDir?: string
+}
 
 export * from './loader/types';


Test patch:
diff --git a/test/fixtures/app_koa_with_ts/src/redis_plugin/meta.yaml b/test/fixtures/app_koa_with_ts/src/redis_plugin/meta.yaml
index 8a1d960..d489d75 100644
--- a/test/fixtures/app_koa_with_ts/src/redis_plugin/meta.yaml
+++ b/test/fixtures/app_koa_with_ts/src/redis_plugin/meta.yaml
@@ -1,1 +1,2 @@
 name: redis
+exclude: ['not_to_be_scanned_file.ts', 'not_to_be_scanned_dir']
\ No newline at end of file
diff --git a/test/fixtures/app_koa_with_ts/src/redis_plugin/not_to_be_scanned_dir/module.ts b/test/fixtures/app_koa_with_ts/src/redis_plugin/not_to_be_scanned_dir/module.ts
new file mode 100644
index 0000000..6ff776d
--- /dev/null
+++ b/test/fixtures/app_koa_with_ts/src/redis_plugin/not_to_be_scanned_dir/module.ts
@@ -0,0 +1,14 @@
+import 'reflect-metadata';
+import { Injectable, ScopeEnum } from '@artus/injection';
+
+@Injectable({
+  scope: ScopeEnum.EXECUTION
+})
+export default class NotToBeScannedModule {
+  async index () {
+    return {
+      status: 200,
+      content: 'Hello Artus'
+    };
+  }
+}
\ No newline at end of file
diff --git a/test/fixtures/app_koa_with_ts/src/redis_plugin/not_to_be_scanned_file.ts b/test/fixtures/app_koa_with_ts/src/redis_plugin/not_to_be_scanned_file.ts
new file mode 100644
index 0000000..0c71936
--- /dev/null
+++ b/test/fixtures/app_koa_with_ts/src/redis_plugin/not_to_be_scanned_file.ts
@@ -0,0 +1,14 @@
+import 'reflect-metadata';
+import { Injectable, ScopeEnum } from '@artus/injection';
+
+@Injectable({
+  scope: ScopeEnum.EXECUTION
+})
+export default class NotToBeScannedController {
+  async index () {
+    return {
+      status: 200,
+      content: 'Hello Artus'
+    };
+  }
+}
\ No newline at end of file
diff --git a/test/fixtures/frameworks/bar/meta.yaml b/test/fixtures/frameworks/bar/meta.yaml
new file mode 100644
index 0000000..2545912
--- /dev/null
+++ b/test/fixtures/frameworks/bar/meta.yaml
@@ -0,0 +1,1 @@
+name: bar
\ No newline at end of file
diff --git a/test/fixtures/frameworks/layer/foo/foo1/meta.yaml b/test/fixtures/frameworks/layer/foo/foo1/meta.yaml
new file mode 100644
index 0000000..9725622
--- /dev/null
+++ b/test/fixtures/frameworks/layer/foo/foo1/meta.yaml
@@ -0,0 +1,1 @@
+name: "foo1"
\ No newline at end of file
diff --git a/test/fixtures/frameworks/layer/foo/foo2/meta.yaml b/test/fixtures/frameworks/layer/foo/foo2/meta.yaml
new file mode 100644
index 0000000..a90255c
--- /dev/null
+++ b/test/fixtures/frameworks/layer/foo/foo2/meta.yaml
@@ -0,0 +1,1 @@
+name: "foo2"
\ No newline at end of file
diff --git a/test/scanner.test.ts b/test/scanner.test.ts
index f56a78c..96e23da 100644
--- a/test/scanner.test.ts
+++ b/test/scanner.test.ts
@@ -14,6 +14,11 @@ describe('test/scanner.test.ts', () => {
         // console.log('manifest', manifest);
         expect(manifest.items.length).toBe(11);
 
+        const excludes = manifest.items.filter(one => {
+          return ['not_to_be_scanned_file.ts', 'module.ts'].includes(one.filename)
+        })
+        expect(excludes).toHaveLength(0);
+
         expect(manifest.items.filter(item => item.loader === 'plugin-config').length).toBe(0);
         expect(manifest.items.filter(item => item.loader === 'plugin-meta').length).toBe(1);
         expect(manifest.items.filter(item => item.loader === 'exception').length).toBe(1);