Skip to content

Conversation

@elrrrrrrr
Copy link
Member

New CnpmcoreConfig type to handle compatibility issues #475

  1. 🆕 add a new CnpmcoreConfig in port/config, referenced in config.default.ts
  2. 🆕 add ChangesStreamMode enums.

新增 CnpmcoreConfig 类型处理兼容问题 #475

  1. 🆕 新增 CnpmcoreConfig,config.default.ts 中进行引用
  2. 🆕 新增 ChangesStreamMode 枚举

image

@elrrrrrrr elrrrrrrr added the enhancement New feature or request label May 25, 2023
@elrrrrrrr elrrrrrrr requested a review from fengmk2 May 25, 2023 06:24
}
export enum SyncDeleteMode {
ignore = 'ignore',
block = 'block',

Choose a reason for hiding this comment

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

这是一个 TypeScript 模块。代码本身很短,只有一些枚举。代码看起来没有风险。
下面是一些可以改进的建议:

  • 没有必要添加注释 export enum SyncMode 等。
  • 对于每个枚举值,请考虑是否使用 PascalCase(例如 Existing 代替 exist)。

cnpmcore: typeof cnpmcoreConfig;
cnpmcore: CnpmcoreConfig;
}
}

Choose a reason for hiding this comment

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

此代码补丁主要是将导入的模块变量名、接口类型等适当修改。在修改前后的代码中,只有引用某个配置的变量声明和接口定义被改动。因此,代码风险较低。

针对这个补丁的改进建议如下:

  1. 如果 cnpmcoreConfig 配置仅作为类型定义使用,则可以直接删除相关的 import 和 typeof 引用,在 EggAppConfig 中直接使用一个与 CnpmcoreConfig 结构一致的对象字面量。这样可以减小代码复杂度。
  2. 如果 cnpmcore 配置是可变的,那么应该考虑通过配置文件实现动态加载,而不是直接放在源码中。

@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.39%. Comparing base (98e6366) to head (c4580a9).
⚠️ Report is 457 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
- Coverage   97.81%   97.39%   -0.43%     
==========================================
  Files         152      168      +16     
  Lines       13549    15680    +2131     
  Branches     2107     2012      -95     
==========================================
+ Hits        13253    15271    +2018     
- Misses        296      409     +113     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

}
export enum SyncDeleteMode {
ignore = 'ignore',
block = 'block',

Choose a reason for hiding this comment

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

这段代码是对枚举类型进行了定义和扩展。从代码来看,似乎没有明显的错误风险。

以下是一些改进建议:

  • 考虑给枚举类型的值加上注释,方便他人理解使用。
  • 可以考虑将变量命名更有意义和语义化,易于阅读和理解。
  • 对于某些枚举类型,可能需要添加更多的选项,以应对潜在的需求变化。

祝你好运!

cnpmcore: typeof cnpmcoreConfig;
cnpmcore: CnpmcoreConfig;
}
}

Choose a reason for hiding this comment

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

这是一段 TypeScript 代码,它主要做了一个导入修改和一个类型修改。

将原本导入的 { cnpmcoreConfig } 修改为了 { CnpmcoreConfig },这可能意味着 config.ts 文件中的 cnpmcoreConfig 已被重命名为 CnpmcoreConfig。需要确认该更改是否正确。

在 EggAppConfig 接口中,cnpmcore 属性的类型也由原来的 typeof cnpmcoreConfig 修改为了 CnpmcoreConfig 类型。需要确认 CnpmcoreConfig 类与 cnpmcoreConfig 值之间的类型兼容性,如果存在问题,则可能需要调整相应的类型定义。

代码挂载还未完成,我们检测不出其余的潜在风险。

const type = changesStreamRegistryMode === ChangesStreamMode.json ? RegistryType.Cnpmcore : RegistryType.Npm;
const registry = await this.createRegistry({
name: PresetRegistryName.default,
type,

Choose a reason for hiding this comment

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

这段代码主要的改动是将 PresetRegistryName 导入了一个新的常量 ChangesStreamMode,同时在某个函数中使用 changesStreamRegistryMode 去设置 type 属性。目前没有明显的 bug 风险,但以下是几点建议:

  1. 添加注释来解释为什么需要使用 ChangesStreamMode.json 更改 type。这样会让未来的维护者更好地理解代码。
  2. 当调用 createRegistry() 函数时,确保输入参数的合法性。

@fengmk2 fengmk2 changed the title refactor: config type fix(refactor): config type May 25, 2023
@fengmk2 fengmk2 changed the title fix(refactor): config type fix: refactor config type May 25, 2023
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 fengmk2 merged commit ebc8c98 into master May 25, 2023
@fengmk2 fengmk2 deleted the config-type branch May 25, 2023 07:19
fengmk2 pushed a commit that referenced this pull request May 25, 2023
[skip ci]

## [3.22.1](v3.22.0...v3.22.1) (2023-05-25)

### Bug Fixes

* refactor config type ([#476](#476)) ([ebc8c98](ebc8c98))
@github-actions
Copy link

🎉 This PR is included in version 3.22.1 🎉

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

enhancement New feature or request released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants