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: easy config #468

Merged
merged 2 commits into from
May 21, 2023
Merged

feat: easy config #468

merged 2 commits into from
May 21, 2023

Conversation

elrrrrrrr
Copy link
Member

Make the method for tegg integration mode to be more user-friendly.

  • 🤖 Automatically add config.cnpmcore type hints.
  • 🧶 Export the default cnpmcoreConfig , which needs to be explicitly declared for app config.
  • 📚 Supplement the documentation and field definitions.

对于 egg 集成模式,提供更加友好的自定义配置方式。

  • 🤖 自动添加 config.cnpmcore 类型提示
  • 🧶 输出默认的 cnpmcoreConfig 对象,应用集成需显式声明,防止新增配置丢失
  • 📚 补充文档及字段定义信息

image

@elrrrrrrr elrrrrrrr added documentation Improvements or additions to documentation enhancement New feature or request labels May 21, 2023
@elrrrrrrr elrrrrrrr requested a review from fengmk2 May 21, 2023 12:38
INTEGRATE.md Show resolved Hide resolved
interface EggAppConfig {
cnpmcore: typeof cnpmcoreConfig;
}
}

Choose a reason for hiding this comment

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

这段代码通过添加导入语句,扩展了AuthClient接口,并在EggAppConfig中添加了cnpmcore属性。我们无法进行完整的审核因为缺少上下文信息,但是根据这个补丁本身,这里有几点建议:

  • 检查config.ts文件或其他相关的文件,确保cnpmcoreConfig被正确地实现和使用。
  • 这个补丁中添加了一些注释屏蔽了TS2310错误,如果可能的话,最好避免在代码中使用这样的注释。
  • 确认这个补丁是否对代码库中其他部分造成了影响。

Copy link
Member Author

Choose a reason for hiding this comment

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

会触发循环引用 bug,用 ts-ignore 可以避免编译直接输出到 d.ts 😈

如果去除的话,需要手动在应用集成时手动赋值。

config.cnpmcore = {
  ...cnpmcoreConfig,
} as typeof cnpmcoreConfig;

@@ -0,0 +1 @@
export { cnpmcoreConfig } from '../../config/config.default';

Choose a reason for hiding this comment

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

这段代码补丁很简单,主要是将一个名为“cnpmcoreConfig”的导出对象从默认配置文件中导出。在没有更多上下文的情况下,很难评估此代码是否存在任何错误风险或改进建议。但是,如果你能提供更多信息,那么我就可以更好地帮助你进行代码审查。

@@ -27,7 +27,7 @@ type LegacyInfo = {

type SiteEnvInfo = {
sync_model: string;
sync_binary: string;
sync_binary: boolean;
instance_start_time: Date;
node_version: string;
app_version: string;

Choose a reason for hiding this comment

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

这个代码提供了一个类型别名 LegacyInfo 和一个类型别名 SiteEnvInfo,其中 SiteEnvInfo 包括了一个名为 sync_binary 的字段,类型是由字符串改为了布尔值。代码的更改似乎没有引入任何风险。

有一个改进建议:命名约定中规定应该在布尔值变量名前添加动词 is 或者 has,这样做可以让代码更易读懂,比如 has_sync_binary 或者 is_sync_enabled

@codecov
Copy link

codecov bot commented May 21, 2023

Codecov Report

Merging #468 (1737c7f) into master (80ab054) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
- Coverage   97.53%   97.48%   -0.05%     
==========================================
  Files         166      167       +1     
  Lines       15437    15519      +82     
  Branches     2002     1999       -3     
==========================================
+ Hits        15056    15129      +73     
- Misses        381      390       +9     
Impacted Files Coverage Δ
app/common/typing.ts 100.00% <100.00%> (ø)
app/port/config.ts 100.00% <100.00%> (ø)
app/port/controller/HomeController.ts 100.00% <100.00%> (ø)
config/config.default.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

INTEGRATE.md Show resolved Hide resolved
interface EggAppConfig {
cnpmcore: typeof cnpmcoreConfig;
}
}

Choose a reason for hiding this comment

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

这段代码加入了对cnpmcoreConfig的导入,并且定义了一个EggAppConfig的接口,
将cnpmcoreConfig设置为Egg应用程序配置的一部分。没有看到与原始代码的比较,难以确定是否存在风险或其它建议。

@@ -0,0 +1 @@
export { cnpmcoreConfig } from '../../config/config.default';

Choose a reason for hiding this comment

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

代码补丁的作用是将cnpmcoreConfig从文件config/config.default中导出。根据这段代码本身来看,没有发现明显的风险或错误。如果需要更全面的代码审查,请提供更多相关代码信息。至于改进建议,需要更多背景知识才能给出具体的意见。

@@ -27,7 +27,7 @@ type LegacyInfo = {

type SiteEnvInfo = {
sync_model: string;
sync_binary: string;
sync_binary: boolean;
instance_start_time: Date;
node_version: string;
app_version: string;

Choose a reason for hiding this comment

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

这段代码改动较小,主要是将 sync_binary 的类型从字符串改为布尔值。没有明显的风险和潜在的错误。

不过,可以考虑以下几点改进:

  1. 建议使用更加明确的命名,例如 is_sync_binary_enabled 或者 has_sync_binary等,增强可读性和易用性。
  2. 由于 sync_modelsync_binary 是相关联的,建议在代码中增加一些注释,来表达两者之间的关系和含义。

export default () => {
const config = {};
config.cnpmcore = {
...cnpmcoreConfig,
Copy link
Member Author

Choose a reason for hiding this comment

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

集成时应当手动对 cnpmcore 进行赋值,目前 mergeConfig 时不会进行deepMerge。如果 cnpmcoreConfig 内新增了默认配置,而应用没有修改,会丢失配置。

Copy link
Member

@fengmk2 fengmk2 left a comment

Choose a reason for hiding this comment

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

+1

@fengmk2
Copy link
Member

fengmk2 commented May 21, 2023

image
好厉害,给 tegg 贡献一份这个最佳实践文档。

*/
redirectNotFound: true,
/**
* enable unpkg features, https://github.com/cnpm/cnpmcore/issues/452
Copy link
Member

Choose a reason for hiding this comment

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

@elrrrrrrr 我将注释改成可以提示的
image

@fengmk2 fengmk2 merged commit 9208392 into master May 21, 2023
13 of 14 checks passed
@fengmk2 fengmk2 deleted the easy-config branch May 21, 2023 13:27
fengmk2 pushed a commit that referenced this pull request May 21, 2023
[skip ci]

## [3.21.0](v3.20.3...v3.21.0) (2023-05-21)

### Features

* easy config ([#468](#468)) ([9208392](9208392))
@github-actions
Copy link

🎉 This PR is included in version 3.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants