Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: basePlugin getPath support paths #159

Closed
wants to merge 1 commit into from

Conversation

wengeezhang
Copy link
Collaborator

背景:
目前 plugin 的寻址策略是由 artus/core 底层确定的,这个策略可能会随着时间而改动。 上层协议或者 cli 工具直接调用 BasePlugin.getPath 即可。

现在的 BasePlugin.getPath 不支持 paths , 导致在某些场景(比如全局安装 cli )下,找不到

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2022

Codecov Report

Merging #159 (aba45e8) into master (df834fc) will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##           master     #159   +/-   ##
=======================================
  Coverage   89.25%   89.26%           
=======================================
  Files          51       51           
  Lines        1089     1090    +1     
  Branches      178      179    +1     
=======================================
+ Hits          972      973    +1     
  Misses        117      117           
Impacted Files Coverage Δ
src/plugin/base.ts 81.81% <33.33%> (+0.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df834fc...aba45e8. Read the comment docs.

Copy link
Member

@noahziheng noahziheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,但仍需注意直接在外部使用 BasePlugin.getPath 不是设计初衷,后续可能影响对 Plugin 逻辑的重构 cc @hyj1991

@hyj1991
Copy link
Member

hyj1991 commented Aug 2, 2022

没太看懂,这里增加的 paths 属性,对于协议实现或者上层封装要如何使用呢?现在这里的寻址只在 scanner 中定义了

@hyj1991
Copy link
Member

hyj1991 commented Aug 2, 2022

另外,这个改动的目的是什么,需要在 test case 中补充一个需要这样使用的例子加以说明

@wengeezhang
Copy link
Collaborator Author

没太看懂,这里增加的 paths 属性,对于协议实现或者上层封装要如何使用呢?现在这里的寻址只在 scanner 中定义了

现在 plugin 的importPath (即找寻 meta.json)是这样定的:

path.resolve(require.resolve(packageName, requireOpts), '..');

这个策略,应该只在artus/core 维护即可。 上层协议或者 cli 工具需要用到这个策略。由于cli是全局按照的,所以必须传入一个paths才能找到对应的包。

@hyj1991
Copy link
Member

hyj1991 commented Aug 2, 2022

这个策略,应该只在artus/core 维护即可。 上层协议或者 cli 工具需要用到这个策略。由于cli是全局按照的,所以必须传入一个paths才能找到对应的包。

上层协议为啥会用到这里,举个伪代码的例子看看呢

@wengeezhang
Copy link
Collaborator Author

这个策略,应该只在artus/core 维护即可。 上层协议或者 cli 工具需要用到这个策略。由于cli是全局按照的,所以必须传入一个paths才能找到对应的包。

上层协议为啥会用到这里,举个伪代码的例子看看呢

// todo: this need to be synced with artus/core's BasePlugin.getPath
// const importPath = BasePlugin.getPath(plugin.name, {paths: process.cwd()})
const importPath = path.resolve(require.resolve(plugin.name, { paths: [process.cwd()] }), '..');
// todo: this need to be synced with artus/core's ArtusPlugin.checkAndLoadMetadata
const metaFilePath = path.resolve(importPath, PLUGIN_META_FILENAME);

这里的 cli 工具,获取 importPath时,需要跟BasePlugin 的保持一致。 不如调用BasePlugin。

@wengeezhang
Copy link
Collaborator Author

如果不调用BasePlugin.getPath,万一那天plugin的策略变化了,cli 工具就必须跟着改。

@hyj1991
Copy link
Member

hyj1991 commented Aug 2, 2022

@wengeezhang 这种是不是在 core 里给个接口直接返回 plugin path 比较好,如果还是调用 BasePlugin.getPath,传入的 paths 参数也是不可控的,感觉会造成问题

@wengeezhang
Copy link
Collaborator Author

@wengeezhang 这种是不是在 core 里给个接口直接返回 plugin path 比较好,如果还是调用 BasePlugin.getPath,传入的 paths 参数也是不可控的,感觉会造成问题

require.resolve(xx, {paths? : [./]}) 这个没有不可控,是比较常见的使用场景。

这种是不是在 core 里给个接口直接返回 plugin path 比较好

其实最终还是要调用 BasePlugin.getPath。这个策略只适合在一个地方维护。所以BasePlugin.getPath 总要支持传paths的

@DuanPengfei
Copy link
Collaborator

这个传入的 paths,其实有可能会影响 BasePlugin 的行为,那是不是反过来也能达成目的,就是 BasePlugin#getPath 依然接受传入 package name,然后返回值把 BasePlugin#getPath 自有逻辑里找到的 package path 返回出来?
/cc @wengeezhang @noahziheng @hyj1991

@hyj1991
Copy link
Member

hyj1991 commented Aug 2, 2022

这个传入的 paths,其实有可能会影响 BasePlugin 的行为,那是不是反过来也能达成目的,就是 BasePlugin#getPath 依然接受传入 package name,然后返回值把 BasePlugin#getPath 自有逻辑里找到的 package path 返回出来? /cc @wengeezhang @noahziheng @hyj1991

我和你的想法是一样的,不要用这个内置方法寻址,而是直接从 core 里拿到地址

@DuanPengfei
Copy link
Collaborator

这个传入的 paths,其实有可能会影响 BasePlugin 的行为,那是不是反过来也能达成目的,就是 BasePlugin#getPath 依然接受传入 package name,然后返回值把 BasePlugin#getPath 自有逻辑里找到的 package path 返回出来? /cc @wengeezhang @noahziheng @hyj1991

我和你的想法是一样的,不要用这个内置方法寻址,而是直接从 core 里拿到地址

刚跟 @wengeezhang 电话碰了一下,咱俩应该是没理解对 😄,按咱们理解错误的确实是这样;但是聊完发现 BasePlugin#getPath 应该是有 bug,他即使不接受 paths,内部也应该要指定好 paths,因为什么都不指定是通过 cwd 去处理的,那就会出现 cwd 不同时出现不可控甚至运行不了的情况了。

@noahziheng
Copy link
Member

duplicated with #161

@noahziheng noahziheng closed this Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants