chore: plugin has own excluded #129

wengeezhang · 2022-07-08T08:42:42Z

No description provided.

wengeezhang · 2022-07-08T08:46:13Z

背景：
用户如果设置了excluded属性，这个值会被应用到插件扫描过程中。最终结果是，有的插件目录可能没法被扫描。

因此，这里做下调整，尊重插件的excluded：

如果plugin有声明excluded，则用default excluded + plugin excluded
如果plugin没有，则用default excluded + 用户 excluded

wengeezhang · 2022-07-08T08:55:28Z

还有一种策略：

如果plugin有声明excluded，则用default excluded + plugin excluded
如果plugin没有，则用default excluded

这种情况下，完全尊重plugin的设置，隔离用户设置带来的污染

noahziheng

LGTM
这个逻辑会阻断 excluded 参数在扫描插件时的传递，可能带来 Breaking Change，请 @hyj1991 注意下

hyj1991 · 2022-07-11T00:49:29Z

如果plugin没有，则用default excluded + 用户 excluded

这个策略下用户的 exclude 还是会带来问题吧？exclude 其实不同于 config.xxx.ts 这样的配置，我觉得用户不需要（其实也无法）去关注使用的插件需要忽略哪些目录，这个就应该是插件 / 框架自身的一个属性

hyj1991 · 2022-07-11T00:49:46Z

用户只要关注应用代码需要忽略哪些即可

cc @noahziheng @DuanPengfei

wengeezhang · 2022-07-11T01:42:37Z

这个策略下用户的 exclude 还是会带来问题吧？exclude 其实不同于 config.xxx.ts 这样的配置，我觉得用户不需要（其实也无法）去关注使用的插件需要忽略哪些目录，这个就应该是插件 / 框架自身的一个属性

下面的策略，是不是你的意思：

如果plugin有声明excluded，则用default excluded + plugin excluded
如果plugin没有，则用default excluded

hyj1991 · 2022-07-11T01:47:40Z

对，我在想 exclude 属于 app / framework / plugin 自身属性的话，其实用户配置只应该影响到 app，这个点需要简单确认下

cc @noahziheng @DuanPengfei

hyj1991 · 2022-07-11T01:55:10Z

另外是，现在的 exclude 貌似是走的文件（夹）名匹配，这是否意味着用户自己的 controller 目录之类的也无法定义 exclude 中的名称目录？

noahziheng · 2022-07-11T02:22:44Z

我在想 exclude 属于 app / framework / plugin 自身属性的话，其实用户配置只应该影响到 app，这个点需要简单确认下

确实对于 exclude 来说，有作用域属性，但也有一些文件可能对于用户来说是需要批量忽略的，如内联插件中可能出现的根目录 index.js 或 pnpm-lock.yaml；

这些在 DEFAULT_EXCLUDE 中可能很难彻底覆盖，对于扫描参数中的 exclude 感觉更应该是贯穿整个扫描过程的，对于 app 中的需要忽略的项需要单独找个地方作为元信息声明（类同 meta.yaml 中增加的 exclude）？

另外是，现在的 exclude 貌似是走的文件（夹）名匹配，这是否意味着用户自己的 controller 目录之类的也无法定义 exclude 中的名称目录？

这个事情我没理解，貌似现在的格式是 glob，此例应该是 controller/**/*？

wengeezhang · 2022-07-11T02:30:49Z

另外是，现在的 exclude 貌似是走的文件（夹）名匹配，这是否意味着用户自己的 controller 目录之类的也无法定义 exclude 中的名称目录？

目前看这个能力是有的，现在走的是minimatch方式来判断

hyj1991 · 2022-07-11T03:48:00Z

这个事情我没理解，貌似现在的格式是 glob，此例应该是 controller/**/*？

不是，按现在的规则，比如 exclude 定义了 test，那么 app/controller/test 这种用户其实非 test 目录也会被忽略掉，这是否其实不符合预期？

JerrysShan · 2022-07-11T03:51:40Z

应该算一个bug 吧，是否把 test 规则改为 test/

wengeezhang · 2022-07-11T06:19:29Z

这个事情我没理解，貌似现在的格式是 glob，此例应该是 controller/**/*？

不是，按现在的规则，比如 exclude 定义了 test，那么 app/controller/test 这种用户其实非 test 目录也会被忽略掉，这是否其实不符合预期？

如果改成了方案二，其实这种场景可以当做“特性”来处理，而非bug，是可以接受的。

hyj1991 · 2022-07-11T06:52:04Z

如果改成了方案二，其实这种场景可以当做“特性”来处理，而非bug，是可以接受的。

这样其实也讲得通，譬如 .gitignore 中的规则和我们现在的 exclude 是一致的，不过我们是否应该也提供和 .gitignore 类似的 !app/controller/test 这样的规则来保证出现误判时用户有办法将实际不需要 exclude 的文件继续扫描进来。

至于本 issue 的 fix 方案，我个人比较倾向方案二，plugin 的编写和 app 是分离的，用户的 exclude 配置不应该影响到 plugin 的文件。

wengeezhang · 2022-07-11T10:24:43Z

最新改动：

framework新增meta.yaml逻辑，和plugin保持一致
目前plugin和framework中的meta中声明的excluded和configDir优先级高
- 如果没有设置，则使用默认值，不使用用户的
plugin和framework中meta.yaml暂时不支持extensions，即extensions始终以用户声明为准
- 这块可以后续看看是否有必要再调整

@noahziheng @hyj1991

hyj1991

测试 case 没有体现 plugin 在 meta 中自定义 exclude 的例子

hyj1991 · 2022-07-11T11:26:13Z

src/scanner/scan.ts

@@ -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']))],
+      excluded: [...new Set(DEFAULT_EXCLUDES.concat(options.excluded ?? []))],


这个叫 excluded 其实比较奇怪，比如 tsconfig 中一般都是只命名为 exclude

exclude +1，和 TS 维持一致好一点，@wengeezhang 要不一块改下

wengeezhang · 2022-07-12T02:06:13Z

测试 case 没有体现 plugin 在 meta 中自定义 exclude 的例子

样例和单测我补一下

hyj1991

+1

noahziheng · 2022-07-12T06:56:45Z

src/framework/handler.ts

@@ -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>{


和 Plugin 的好像重复实现了？

noahziheng · 2022-07-12T06:57:31Z

src/framework/handler.ts

+    }
+
+    if (!find) {
+      throw new Error(`load framework import path ${frameworkDir} can't find meta file.`);


这个应该会造成无 meta.yaml 的 Framework 直接报错，会是 Breaking Change cc @hyj1991

noahziheng · 2022-07-12T06:58:25Z

src/types.ts

@@ -35,5 +35,11 @@ export interface TriggerType {
  initContext(...args): Promise<BaseContext>;
  startPipeline(...args): Promise<void>;
 }
+export interface Metadata {


命名太泛了？

noahziheng · 2022-07-12T06:59:44Z

src/scanner/scan.ts

+      walkOptions.exclude = DEFAULT_EXCLUDES;
+    }
+
+    // plugin/framework extensions take priority over user app's


先去掉吧，这是社区项目，留太多无用注释不好

noahziheng · 2022-07-12T07:01:04Z

src/scanner/scan.ts

+    return result;
+  }
+
+  private amendOptions(walkOptions: WalkOptions, metadata): WalkOptions {


昨天不是说把这个逻辑直接放在 formatWalkOptions 里？增加 “amend” 的意义在哪，这就是 formatWalkOptions 的职责吧

是啊，这不是在formatWalkOptions里面调用的嘛

noahziheng · 2022-07-12T07:03:22Z

src/scanner/scan.ts

-        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);


如果是 extends 的 interface，不需要 as 吧

noahziheng · 2022-07-12T07:05:18Z

test/fixtures/app_koa_with_ts/src/redis_plugin/meta.yaml

@@ -1 +1,2 @@
 name: redis
+exclude: ['not_to_be_scanned_file.ts', 'not_to_be_scanned_dir']


缺 glob 形式的验证，可以后续补充

This reverts commit 332fe1e.

chore: plugin has own excluded

b6d2e3f

noahziheng approved these changes Jul 8, 2022

View reviewed changes

wengeezhang added 5 commits July 11, 2022 16:29

refactor: framework check metadata

e768c31

chore: add meta.yaml to framwork

5acf889

chore: return metadata

2daabbe

chore: formatWalkOptions call amendOptions

94a0963

chore: delete empty line

95c4c49

hyj1991 requested changes Jul 11, 2022

View reviewed changes

wengeezhang requested review from noahziheng and hyj1991 July 12, 2022 02:04

wengeezhang added 2 commits July 12, 2022 14:41

chore: excluded to exclude;

a453e44

chore: exclude test

5473c91

hyj1991 approved these changes Jul 12, 2022

View reviewed changes

hyj1991 merged commit 332fe1e into artusjs:master Jul 12, 2022

noahziheng requested changes Jul 12, 2022

View reviewed changes

hyj1991 added a commit that referenced this pull request Jul 12, 2022

Revert "chore: plugin has own excluded (#129)"

0b45e03

This reverts commit 332fe1e.

hyj1991 mentioned this pull request Jul 12, 2022

Revert "chore: plugin has own excluded" #133

Merged

hyj1991 added a commit that referenced this pull request Jul 12, 2022

Revert "chore: plugin has own excluded (#129)" (#133)

61d92ac

This reverts commit 332fe1e.

noahziheng mentioned this pull request Jul 12, 2022

refactor(scanner): modify exclude option for plugin & excluded rename #134

Merged

wengeezhang mentioned this pull request Jul 13, 2022

plugin和framework的meta优先 #135

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: plugin has own excluded #129

chore: plugin has own excluded #129

wengeezhang commented Jul 8, 2022

wengeezhang commented Jul 8, 2022

wengeezhang commented Jul 8, 2022

noahziheng left a comment

hyj1991 commented Jul 11, 2022

hyj1991 commented Jul 11, 2022 •

edited

Loading

wengeezhang commented Jul 11, 2022

hyj1991 commented Jul 11, 2022

hyj1991 commented Jul 11, 2022

noahziheng commented Jul 11, 2022

wengeezhang commented Jul 11, 2022

hyj1991 commented Jul 11, 2022 •

edited

Loading

JerrysShan commented Jul 11, 2022

wengeezhang commented Jul 11, 2022

hyj1991 commented Jul 11, 2022 •

edited

Loading

wengeezhang commented Jul 11, 2022

hyj1991 left a comment

hyj1991 Jul 11, 2022

noahziheng Jul 11, 2022

wengeezhang Jul 12, 2022

wengeezhang commented Jul 12, 2022 •

edited

Loading

hyj1991 left a comment

noahziheng Jul 12, 2022

noahziheng Jul 12, 2022

noahziheng Jul 12, 2022

noahziheng Jul 12, 2022

noahziheng Jul 12, 2022

wengeezhang Jul 12, 2022

noahziheng Jul 12, 2022

noahziheng Jul 12, 2022

		@@ -1 +1,2 @@
		name: redis
		exclude: ['not_to_be_scanned_file.ts', 'not_to_be_scanned_dir']

chore: plugin has own excluded #129

chore: plugin has own excluded #129

Conversation

wengeezhang commented Jul 8, 2022

wengeezhang commented Jul 8, 2022

wengeezhang commented Jul 8, 2022

noahziheng left a comment

Choose a reason for hiding this comment

hyj1991 commented Jul 11, 2022

hyj1991 commented Jul 11, 2022 • edited Loading

wengeezhang commented Jul 11, 2022

hyj1991 commented Jul 11, 2022

hyj1991 commented Jul 11, 2022

noahziheng commented Jul 11, 2022

wengeezhang commented Jul 11, 2022

hyj1991 commented Jul 11, 2022 • edited Loading

JerrysShan commented Jul 11, 2022

wengeezhang commented Jul 11, 2022

hyj1991 commented Jul 11, 2022 • edited Loading

wengeezhang commented Jul 11, 2022

hyj1991 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wengeezhang commented Jul 12, 2022 • edited Loading

hyj1991 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyj1991 commented Jul 11, 2022 •

edited

Loading

hyj1991 commented Jul 11, 2022 •

edited

Loading

hyj1991 commented Jul 11, 2022 •

edited

Loading

wengeezhang commented Jul 12, 2022 •

edited

Loading