fix: form setValues not support dayjs and Date value
#5064
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/@core/ui-kit/form-ui/src/form-api.tsOops! Something went wrong! :( ESLint: 9.16.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/@core/ui-kit/form-ui/src/form-api.ts (2)
255-258: Enhance comment clarity regarding the dayjs object handlingWhile the comment explains the limitation, it could be more specific about the implemented solution. Consider updating it to explicitly mention that dayjs objects are excluded from deep merging via the
$isDayjsObjectcheck.- /** - * 合并算法有待改进，目前的算法不支持object类型的值。 - * 但是Antd的日期时间类型的组件值为dayjs对象类型，也是一个object类型，这里将dayjs对象排除深度合并 - */ + /** + * The merge strategy has a limitation with object-type values. + * For Ant Design's date/time components that use dayjs objects, + * we explicitly exclude them from deep merging by checking the + * $isDayjsObject property to preserve their integrity. + */
255-275: Consider a more generic special object type handling mechanismWhile the current solution fixes the dayjs object issue, consider implementing a more generic mechanism for handling special object types in the form values. This could be achieved through:
- A registry of special object types and their merge strategies
- A pluggable system for custom object type handlers
- Metadata-based merge strategy selection
This would make the form API more extensible for future special object types beyond dayjs.
Would you like me to propose a detailed design for this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@core/ui-kit/form-ui/src/form-api.ts(1 hunks)
🔇 Additional comments (1)
packages/@core/ui-kit/form-ui/src/form-api.ts (1)
258-270: Verify edge cases in the merge function
The merge function correctly handles dayjs objects, but consider these edge cases:
- Nested dayjs objects in arrays
- Null/undefined values that might have
$isDayjsObjectproperty - Objects that might coincidentally have
$isDayjsObjectproperty but aren't dayjs objects
Let's verify the dayjs object handling:
✅ Verification successful
Based on the gathered evidence, I can now generate the final response:
The dayjs object handling in the merge function is appropriate and safe
The implementation correctly handles dayjs objects by checking the $isDayjsObject property before applying the merge. The codebase shows consistent handling of dayjs objects:
- Dayjs objects are primarily used in date picker form fields (as seen in playground examples)
- The form handles time-related values through dedicated transformation logic (
handleRangeTimeValue) - The merge function's edge cases are properly covered:
- Objects are checked for
$isDayjsObjectbefore any merge attempts - Arrays are explicitly excluded from merging
- The check is combined with
isObjectvalidation
- Objects are checked for
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dayjs object usage patterns in the codebase
# to ensure consistent handling
# Look for dayjs object assignments to form values
rg -l "setValues.*dayjs"
# Look for other places where dayjs objects might be handled
ast-grep --pattern 'dayjs($$$)'
Length of output: 137
Script:
#!/bin/bash
# Let's search for more context about dayjs usage and form handling
# Look for dayjs type definitions or interfaces
rg -A 5 "type.*dayjs|interface.*dayjs"
# Search for form value handling patterns
rg -A 5 "form\.values|setValues"
# Look for tests involving dayjs and form values
rg -A 5 "test.*dayjs.*form|describe.*dayjs.*form"
# Search for any validation or type checking related to dayjs
ast-grep --pattern 'isDayjsObject|$isDayjsObject'
Length of output: 11037
Script:
#!/bin/bash
# Let's look for the actual implementation of dayjs object handling
# Search for dayjs object creation and type checking
rg -A 5 "\$isDayjsObject"
# Look for the createMerge utility implementation
ast-grep --pattern 'function createMerge'
# Search for any dayjs-related tests or validation
rg -A 5 "isDayjs|dayjs\(" packages/@core/ui-kit/form-ui/__tests__/
# Look for form value transformation logic
rg -A 5 "handleRangeTimeValue|transformDateValue"
Length of output: 1321
mynetfan
changed the title
dayjs object valuedayjs and Date value
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/@core/ui-kit/form-ui/src/form-api.ts (2)
257-262: Enhance documentation with examplesWhile the comment explains the limitations well, it would be more helpful to include examples of the problematic cases and how they're now handled.
/** * 合并算法有待改进，目前的算法不支持object类型的值。 * antd的日期时间相关组件的值类型为dayjs对象 * element-plus的日期时间相关组件的值类型可能为Date对象 * 以上两种类型需要排除深度合并 + * + * Example: + * // Before (problematic): + * form.setValues({ + * date: dayjs('2023-12-25'), // dayjs object + * time: new Date() // Date object + * }); + * + * // After (fixed): + * // These objects are now excluded from deep merging */
266-269: Add test coverage for date object handlingThe new date object handling logic should be covered by unit tests to ensure it works correctly with both dayjs and Date objects.
Would you like me to help create unit tests for these scenarios:
- Setting form values with dayjs objects
- Setting form values with Date objects
- Setting form values with mixed object types
- Multiple setValues calls with date objects
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@core/base/shared/src/utils/date.ts(1 hunks)packages/@core/ui-kit/form-ui/src/form-api.ts(2 hunks)
🔇 Additional comments (4)
packages/@core/base/shared/src/utils/date.ts (1)
20-26: LGTM! Well-implemented type guards
The new type guard functions are correctly implemented:
isDate: Proper use ofinstanceoffor Date objectsisDayjsObject: Correct usage ofdayjs.isDayjs()for type checking- Both use TypeScript type predicates appropriately
packages/@core/ui-kit/form-ui/src/form-api.ts (3)
17-18: LGTM! Appropriate imports added
The new type guard imports are correctly added and maintain the import order.
276-280: 🛠️ Refactor suggestion
Enhance error handling for better debugging
The current error handling could be improved to include more context and implement error recovery.
try {
form.setValues(filteredFields, shouldValidate);
} catch (error) {
- console.error('setValues error:', error);
+ console.error('Failed to set form values:', {
+ error,
+ fields: filteredFields,
+ shouldValidate,
+ });
+ throw new Error(`Failed to set form values: ${error.message}`);
}266-269: Verify form value handling across the codebase
Let's verify that this change doesn't affect other form value scenarios.
✅ Verification successful
Changes to date object handling are safe to proceed
The verification shows that:
- The date handling utilities (
isDateandisDayjsObject) are properly implemented and used correctly in the form-api - The test suite doesn't reveal any special cases for date handling that could be affected
- The form value merging logic correctly preserves both native Date and Dayjs objects by skipping deep merging for these types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impacts on form value handling
# Search for setValues usage with date/time related fields
echo "Checking setValues usage with date/time fields..."
rg -l "setValues.*date|time|moment|dayjs" --type ts
# Search for form components using date/time pickers
echo "Checking date/time picker component usage..."
rg -l "DatePicker|TimePicker|Calendar" --type vue --type tsx
Length of output: 1101
Script:
#!/bin/bash
# Let's analyze the form-api.ts implementation and its usage more precisely
# Check the implementation of form value handling in form-api.ts
echo "Checking form-api.ts implementation..."
rg -A 5 "setValues|isDate|isDayjsObject" packages/@core/ui-kit/form-ui/src/form-api.ts
# Look for tests related to form value handling
echo "Checking form-related tests..."
fd "form.*test" -e ts -x rg -l "date|dayjs|setValues"
# Search for direct imports of form-api
echo "Checking form-api imports..."
rg "from.*form-api" --type ts
Length of output: 1513
Script:
#!/bin/bash
# Let's check the test file to understand the expected behavior with dates
# Check form-api test implementation
echo "Checking form-api test cases..."
cat packages/@core/ui-kit/form-ui/__tests__/form-api.test.ts
# Look for the actual implementation of isDayjsObject and isDate utilities
echo "Checking utility functions implementation..."
rg -A 3 "export.*function.*isDayjsObject|export.*function.*isDate" packages/@core/base/shared/src/utils/
Length of output: 5920
Description
修复setFormValues的值类型合并算法没有考虑dayjs对象类型值以及Date类型值的问题。antd的日期时间相关组件的值类型为dayjs对象，而ElementPlus的日期时间相关组件的值可能为Date类型
fix #5061
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
dayjsandDateobjects.isDateandisDayjsObject.Bug Fixes
dayjsandDateobjects during value setting.