fix(form): 修复 formitem 的值如果是对象会自动重置为空对象的问题 #2952
Conversation
演练此次更改主要涉及 变更
可能相关的 PR
建议的审阅者
诗歌
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #2952 +/- ##
==========================================
+ Coverage 84.03% 84.19% +0.15%
==========================================
Files 220 267 +47
Lines 17917 18099 +182
Branches 2628 2661 +33
==========================================
+ Hits 15057 15238 +181
- Misses 2855 2856 +1
Partials 5 5 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/merge.ts (2)
45-51: 优化isPlainObject函数的实现方式当前的实现方法可能影响性能，且代码较为复杂。可以使用更简洁的方法来检查是否为纯对象。
建议使用如下实现：
export function isPlainObject(input: unknown): input is Record<string, any> { - if (input === null || typeof input !== 'object') return false - if (Object.getPrototypeOf(input) === null) return true - let ref = input - while (Object.getPrototypeOf(ref) !== null) ref = Object.getPrototypeOf(ref) - return Object.getPrototypeOf(input) === ref + return Object.prototype.toString.call(input) === '[object Object]' }
38-38: 在使用for...in循环时应考虑属性的可枚举性直接使用
for...in可能会遍历到对象原型链上的可枚举属性，建议在循环中加上hasOwnProperty检查，确保只遍历对象自身的属性。修改如下：
- for (const index in input) output[index] = clone((input as any)[index]) + for (const index in input) { + if (Object.prototype.hasOwnProperty.call(input, index)) { + output[index] = clone((input as any)[index]) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/merge.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/utils/merge.ts
[error] 71-71: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/utils/merge.ts (1)
60-62: 递归合并时需要考虑数组和非纯对象的处理在递归合并过程中，目前仅对纯对象进行了处理，可能会忽略数组类型或其他可合并的结构。建议在递归函数中增加对数组类型的支持，或明确处理方式。
请确认在递归合并时，是否需要处理数组类型，避免潜在的问题。
xiaoyatong
changed the title
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/packages/form/__tests__/merge.spec.ts (1)
147-165: 完善测试用例描述建议为递归合并的测试用例添加更具体的描述，以便更好地理解每个测试的目的。
- it('merges recursively', () => { + it('should merge nested objects without mutating unrelated properties', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/packages/form/__tests__/merge.spec.ts(1 hunks)src/packages/form/useform.taro.ts(2 hunks)src/packages/form/useform.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/packages/form/__tests__/merge.spec.ts
[error] 89-89: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 91-91: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 132-132: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 134-134: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (3)
src/packages/form/useform.taro.ts (1)
3-3: 导入并使用 recursive 函数修复对象值重置问题使用
recursive替换merge是一个很好的改进，它可以正确处理嵌套对象的合并，防止表单项的对象值被重置为空对象。Also applies to: 101-101
src/packages/form/useform.ts (1)
3-3: 使用 recursive 函数确保深度合并使用
recursive(true, ...)替换merge可以确保对象值的正确合并，这与 useform.taro.ts 中的修改保持一致。Also applies to: 101-101
src/packages/form/__tests__/merge.spec.ts (1)
1-263: 测试覆盖全面测试用例覆盖了所有关键功能，包括对象合并、克隆、递归合并以及边界情况的处理，这对确保代码质量非常重要。
🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 91-91: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 132-132: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 134-134: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
src/packages/form/useform.ts
Outdated
| @@ -202,7 +202,9 @@ class FormStore { | |||
|
|
|||
| resetFields = () => { | |||
| this.errors.length = 0 | |||
| console.log('xxx', this.initialValues) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
移除调试日志
建议移除这些控制台日志，因为它们不应该出现在生产代码中。
resetFields = () => {
this.errors.length = 0
- console.log('xxx', this.initialValues)
const nextStore = merge({}, this.initialValues)
- console.log(nextStore)
this.updateStore(nextStore)
this.fieldEntities.forEach((entity: FormFieldEntity) => {Also applies to: 207-207
| if (typeof outputValue === 'number' && isNaN(outputValue)) { | ||
| // eslint-disable-next-line no-restricted-globals | ||
| expect(isNaN(inputValue), key).toBeTruthy() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
使用 Number.isNaN 替换不安全的全局 isNaN
为了避免类型强制转换带来的潜在问题，建议使用 Number.isNaN。
- if (typeof outputValue === 'number' && isNaN(outputValue)) {
+ if (typeof outputValue === 'number' && Number.isNaN(outputValue)) {
- expect(isNaN(inputValue), key).toBeTruthy()
+ expect(Number.isNaN(inputValue), key).toBeTruthy()
// 同样在其他位置也需要修改：
- if (typeof cloned === 'number' && isNaN(cloned)) {
+ if (typeof cloned === 'number' && Number.isNaN(cloned)) {
- expect(isNaN(value)).toBeTruthy()
+ expect(Number.isNaN(value)).toBeTruthy()Also applies to: 132-134
🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 91-91: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🤔 这个变动的性质是？
🔗 相关 Issue
#2870
💡 需求背景和解决方案
merge 方法存在 bug
☑️ 请求合并前的自查清单
Summary by CodeRabbit
新功能
改进
merge和recursive函数的实现Bug 修复
测试