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(scanner): add parsed pluginConfig to manifest #197

Merged
merged 2 commits into from
Sep 15, 2022

Conversation

noahziheng
Copy link
Member

本 PR 向 Manifest 类型定义和 Scanner.scan 方法逻辑中增加了 pluginConfig 字段,值为经解析后的插件配置,用于:

  • 运行时数据上报 / 构建时依赖分析
  • 多 Env 场景下的运行时 Manifest 合并逻辑中,作为 plugin 启用判断的辅助数据

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2022

Codecov Report

Base: 90.71% // Head: 90.76% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (e0ccd9f) compared to base (d171511).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   90.71%   90.76%   +0.04%     
==========================================
  Files          48       48              
  Lines        1099     1104       +5     
  Branches      195      197       +2     
==========================================
+ Hits          997     1002       +5     
  Misses        102      102              
Impacted Files Coverage Δ
src/scanner/scan.ts 95.23% <100.00%> (+0.16%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hyj1991
Copy link
Member

hyj1991 commented Sep 15, 2022

Manifest 增加额外属性,是否可以考虑把 config 完整 dump 出来,plugin 则作为 config 的一个字段属性:

{
  items: [ ],
  config: { },
  relative: boolean
}

实践中一般 config 也需要静态 dump 用作分析,这样后面也无需增加额外字段来存储配置相关的静态信息了。

@noahziheng
Copy link
Member Author

@hyj1991 这里有个问题是现在 config 支持 Function 模型,这样的话其值是动态的 cc @JerrysShan

@hyj1991
Copy link
Member

hyj1991 commented Sep 15, 2022

这里有个问题是现在 config 支持 Function 模型,这样的话其值是动态的

这确实是一个问题,那么考虑将明确静态的配置导出到 config 字段下呢,其实比如 framework 继承链,有时候也需要导出用作分析(尤其在继承链复杂的场景)

@noahziheng
Copy link
Member Author

这里有个问题是现在 config 支持 Function 模型,这样的话其值是动态的

这确实是一个问题,那么考虑将明确静态的配置导出到 config 字段下呢,其实比如 framework 继承链,有时候也需要导出用作分析(尤其在继承链复杂的场景)

那感觉还是要特定化 frameworkConfig 可以放进来,对于其他的配置项,还得是走动态 dump app.config 这种运行时分析方案

@noahziheng noahziheng force-pushed the feat/manifest-add-plugin-config branch from 7e663ce to 55a7db9 Compare September 15, 2022 10:14
@hyj1991
Copy link
Member

hyj1991 commented Sep 15, 2022

其实 #196 和本 PR 有类似的问题,我在想既然 plugin & framework 处理这么特殊,是不是可以把定义从 config 目录中分离出来,放到比如 meta.json 里面作为元信息申明

好处是:

  • meta.json 是一个已有的设计,目前只存在 plugin & framework 中,那么考虑在 app 中也允许元信息的申明即可
  • 将配置的动态和静态部分分开,meta.json 中只会存在静态的模块相关的元信息
  • config 下的动态配置处理 loader 逻辑统一,不再需要特化的处理,保证配置处理部分的纯净

@noahziheng
Copy link
Member Author

@hyj1991 我个人认为有点激进了,对目前的设计冲击似乎太大,转个 Issue 讨论下吧

@hyj1991
Copy link
Member

hyj1991 commented Sep 15, 2022

嗯,我可以搞个 POC 看看对目前的逻辑的冲击,到时候和 RFC 一并放出来,本 PR 和 #196 作为关联项作为参考

@noahziheng noahziheng merged commit e3c7fde into artusjs:master Sep 15, 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.

3 participants