feat(config-provider): [config-provider] 支持合并主题配置 #3187
Conversation
WalkthroughThe changes add new Vue components in the demos for a configuration provider that supports nested theme configurations using Tiny UI components. Both Composition API and classic export styles are introduced with reactive form handling and validation. A new Playwright test verifies custom events and UI behavior. The documentation is updated with a new demo entry, the utilities module now includes a deep merge function, and the Vue config-provider package metadata and design configuration logic are updated to merge parent and child design settings. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as ConfigProviderComponent
participant H as handleSubmitPromise
participant V as Validator
participant A as Alert Component
U->>C: Fill in form fields (name, age)
U->>C: Click Submit
C->>H: Trigger form submission
H->>V: Validate form data
V-->>H: Return validation result
H->>A: Trigger alert feedback
A-->>U: Display notification
sequenceDiagram
participant C as ConfigProvider Component
participant I as Injector (hooks.inject)
participant D as deepMerge Function
participant P as provideDesignConfig
C->>I: Inject parent design via designSymbol
I-->>C: Return parentDesign (or empty object)
C->>D: Merge parentDesign with child design prop
D-->>C: Return merged currentDesign
C->>P: Provide merged design configuration
Suggested labels
Suggested reviewers
Poem
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
examples/sites/demos/pc/app/config-provider/merge.spec.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. examples/sites/demos/pc/app/config-provider/merge-composition-api.vueOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. examples/sites/demos/pc/app/config-provider/merge.vueOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
github-actions
bot
added
bug
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
hashiqi12138
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
examples/sites/demos/pc/app/config-provider/webdoc/config-provider.js (1)
21-33: The English translation could better reflect the nested usage concept.The Chinese name "嵌套使用" correctly describes the nested usage functionality, but the English name "Merge" doesn't completely convey this concept. Additionally, the English description is reused from the base demo rather than being specific to nested configuration providers.
Consider improving the English translation:
name: { 'zh-CN': '嵌套使用', - 'en-US': 'Merge' + 'en-US': 'Nested Usage' }, desc: { 'zh-CN': '支持嵌套使用', 'en-US': - 'Icons and logic for different design specifications can be customized through the <code>design</code> attribute configuration.' + 'Supports nested configuration providers where child providers can inherit and override parent configurations.' },packages/utils/src/object/index.ts (1)
453-461: Consider using isPlainObject instead of isObject for safer recursion.The current implementation uses
isObjectwhich may include arrays and other non-plain objects. For deep merging, it's safer to specifically check for plain objects to avoid issues with arrays or special objects.- if (isObject(sourceValue)) { + if (isPlainObject(sourceValue)) { if (isObject(targetValue)) { - // 递归合并对象 + // 递归合并普通对象 merge(targetValue, sourceValue) } else { // 目标属性不是对象，创建新对象并递归合并 target[key] = {} merge(target[key], sourceValue) }packages/vue/src/config-provider/src/index.vue (1)
72-77: Simplify parentDesign null checking expression.The expression
parentDesign?.value || parentDesign || {}is complex and tries to handle different possible types of the injected value. This could be simplified for better readability and maintainability.const currentDesign = hooks.computed(() => { - const parentValue = parentDesign?.value || parentDesign || {} + // Handle both reactive and non-reactive objects + const parentValue = (parentDesign && 'value' in parentDesign) ? parentDesign.value : (parentDesign || {}) return deepMerge(parentValue, design.value) })examples/sites/demos/pc/app/config-provider/merge.vue (3)
197-200: Form data is shared across all formsAll forms are using the same
formDataobject, which means changing a value in one form will update all forms simultaneously. If this is not the intended behavior, consider creating separate form data objects.Create separate form data objects for each form:
formData: { name: '', age: '' }, +formData1: { + name: '', + age: '' +}, +formData2: { + name: '', + age: '' +}Then update each form to use its respective data object.
158-196: Unused design configurationThe
designobject is defined but not used anywhere in the template.You can remove this unused object or add a comment explaining why it's defined but not used.
204-207: Error suppression without handlingThe validation method catches errors but does nothing with them, which might hide important validation failures.
Consider handling validation errors more explicitly:
handleSubmitPromise() { - this.$refs.ruleFormRef.validate().catch(() => {}) + this.$refs.ruleFormRef.validate() + .then(() => { + // Handle success case + }) + .catch((errors) => { + console.log('Validation failed:', errors) + // Handle validation failure + }) }examples/sites/demos/pc/app/config-provider/merge.spec.ts (2)
4-4: Error handling might mask test failuresThe current error handler suppresses all page errors by expecting them to be null, but this could hide legitimate issues during testing.
Consider a more specific error handling approach:
-page.on('pageerror', (exception) => expect(exception).toBeNull()) +page.on('pageerror', (exception) => { + console.error('Page error:', exception.message); + // Only fail the test for specific errors you care about + // expect(exception.message).not.toContain('Expected error phrase') +})
24-24: Short timeout for button state verificationThe 300ms timeout for verifying that the button is not disabled may be too short for slower environments, potentially causing flaky tests.
Consider increasing the timeout to a more reasonable value:
-await expect(demo.locator('.tiny-button')).not.toBeDisabled({ timeout: 300 }) +await expect(demo.locator('.tiny-button')).not.toBeDisabled({ timeout: 1000 })examples/sites/demos/pc/app/config-provider/merge-composition-api.vue (3)
133-148: Inconsistent reactive state patternMost design configurations are defined as refs (using
ref()), butnoParentDesignis defined as a plain object. This inconsistency could lead to reactivity issues.Make
noParentDesignreactive for consistency:-const noParentDesign = { +const noParentDesign = ref({ name: 'x-design-n', // 设计规范名称 version: '1.0.0', // 设计规范版本号 components: { Button: { props: { round: true } }, Form: { props: { hideRequiredAsterisk: true } } } -} +})Then update the binding in template:
-<tiny-config-provider :design="noParentDesign"> +<tiny-config-provider :design="noParentDesign.value">
150-188: Unused design configurationThe
designobject is defined but not used anywhere in the template, just like in the Options API version.You can remove this unused object or add a comment explaining why it's defined but not used.
190-192: Error suppression without handlingThe validation method catches errors but does nothing with them, which might hide important validation failures.
Consider handling validation errors more explicitly:
const handleSubmitPromise = () => { - ruleFormRef.value.validate().catch(() => {}) + ruleFormRef.value.validate() + .then(() => { + // Handle success case + }) + .catch((errors) => { + console.log('Validation failed:', errors) + // Handle validation failure + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/sites/demos/pc/app/config-provider/merge-composition-api.vue(1 hunks)examples/sites/demos/pc/app/config-provider/merge.spec.ts(1 hunks)examples/sites/demos/pc/app/config-provider/merge.vue(1 hunks)examples/sites/demos/pc/app/config-provider/webdoc/config-provider.js(1 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/object/index.ts(1 hunks)packages/vue/src/config-provider/package.json(1 hunks)packages/vue/src/config-provider/src/index.vue(2 hunks)
🔇 Additional comments (3)
packages/vue/src/config-provider/src/index.vue (1)
70-79: LGTM! The nested config provider implementation is well-structured.The implementation correctly handles merging parent and child design configurations using Vue's provide/inject pattern. The computed property ensures reactivity when either parent or child design changes.
packages/utils/src/index.ts (1)
113-125: LGTM! Export statement correctly includes deepMerge function.The multi-line export improves readability while properly including the new
deepMergefunction.packages/vue/src/config-provider/package.json (1)
14-14:✅ Verification successful
Added dependency needs to be utilized
You've added
@opentiny/utilsas a dependency, which is appropriate for the feature, but ensure it's properly imported and used in the config-provider component.
🏁 Script executed:
#!/bin/bash # Check if the utils dependency is actually used in the config-provider module echo "Searching for utils imports in config-provider files:" rg -l "@opentiny/utils" packages/vue/src/config-provider/Length of output: 267
Verified Dependency Usage: @opentiny/utils is Properly Utilized
The dependency is imported in the config-provider component (as seen in
packages/vue/src/config-provider/src/index.vue), which confirms its utilization. No further changes are required regarding this dependency.
|
|
||
| /** | ||
| * 深度合并普通对象：递归合并所有普通对象的属性，嵌套对象也会被合并。 | ||
| * 覆盖非对象值：数组、日期、函数等非普通对象值会被直接覆盖。 | ||
| * 不修改原对象：所有合并操作都在新对象上进行，原对象保持不变。 | ||
| * 处理null和undefined源：忽略null和undefined类型的源对象。 | ||
| */ | ||
| export function deepMerge(...sources) { | ||
| function merge(target, source) { | ||
| if (!isObject(source)) { | ||
| return | ||
| } | ||
| Object.keys(source).forEach((key) => { | ||
| const sourceValue = source[key] | ||
| const targetValue = target[key] | ||
|
|
||
| if (isObject(sourceValue)) { | ||
| if (isObject(targetValue)) { | ||
| // 递归合并对象 | ||
| merge(targetValue, sourceValue) | ||
| } else { | ||
| // 目标属性不是对象，创建新对象并递归合并 | ||
| target[key] = {} | ||
| merge(target[key], sourceValue) | ||
| } | ||
| } else { | ||
| // 直接赋值非对象或数组等类型的值 | ||
| target[key] = sourceValue | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| const target = {} | ||
| for (const source of sources) { | ||
| if (source !== null && source !== undefined) { | ||
| merge(target, source) | ||
| } | ||
| } | ||
| return target | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TypeScript type definitions missing in deepMerge function.
The deepMerge function is implemented without proper TypeScript type definitions, unlike other functions in this file. This could lead to type-checking issues and reduced code quality.
Add appropriate TypeScript type definitions:
- export function deepMerge(...sources) {
+ export function deepMerge(...sources: object[]): object {
- function merge(target, source) {
+ function merge(target: object, source: object): void {
if (!isObject(source)) {
return
}
Object.keys(source).forEach((key) => {
- const sourceValue = source[key]
- const targetValue = target[key]
+ const sourceValue = source[key as keyof typeof source]
+ const targetValue = target[key as keyof typeof target]
if (isObject(sourceValue)) {
if (isObject(targetValue)) {
// 递归合并对象
merge(targetValue, sourceValue)
} else {
// 目标属性不是对象，创建新对象并递归合并
- target[key] = {}
- merge(target[key], sourceValue)
+ target[key as keyof typeof target] = {}
+ merge(target[key as keyof typeof target], sourceValue)
}
} else {
// 直接赋值非对象或数组等类型的值
- target[key] = sourceValue
+ target[key as keyof typeof target] = sourceValue
}
})
}
const target = {}
for (const source of sources) {
if (source !== null && source !== undefined) {
merge(target, source)
}
}
return target
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * 深度合并普通对象：递归合并所有普通对象的属性，嵌套对象也会被合并。 | |
| * 覆盖非对象值：数组、日期、函数等非普通对象值会被直接覆盖。 | |
| * 不修改原对象：所有合并操作都在新对象上进行，原对象保持不变。 | |
| * 处理null和undefined源：忽略null和undefined类型的源对象。 | |
| */ | |
| export function deepMerge(...sources) { | |
| function merge(target, source) { | |
| if (!isObject(source)) { | |
| return | |
| } | |
| Object.keys(source).forEach((key) => { | |
| const sourceValue = source[key] | |
| const targetValue = target[key] | |
| if (isObject(sourceValue)) { | |
| if (isObject(targetValue)) { | |
| // 递归合并对象 | |
| merge(targetValue, sourceValue) | |
| } else { | |
| // 目标属性不是对象，创建新对象并递归合并 | |
| target[key] = {} | |
| merge(target[key], sourceValue) | |
| } | |
| } else { | |
| // 直接赋值非对象或数组等类型的值 | |
| target[key] = sourceValue | |
| } | |
| }) | |
| } | |
| const target = {} | |
| for (const source of sources) { | |
| if (source !== null && source !== undefined) { | |
| merge(target, source) | |
| } | |
| } | |
| return target | |
| } | |
| /** | |
| * 深度合并普通对象：递归合并所有普通对象的属性，嵌套对象也会被合并。 | |
| * 覆盖非对象值：数组、日期、函数等非普通对象值会被直接覆盖。 | |
| * 不修改原对象：所有合并操作都在新对象上进行，原对象保持不变。 | |
| * 处理null和undefined源：忽略null和undefined类型的源对象。 | |
| */ | |
| export function deepMerge(...sources: object[]): object { | |
| function merge(target: object, source: object): void { | |
| if (!isObject(source)) { | |
| return | |
| } | |
| Object.keys(source).forEach((key) => { | |
| const sourceValue = source[key as keyof typeof source] | |
| const targetValue = target[key as keyof typeof target] | |
| if (isObject(sourceValue)) { | |
| if (isObject(targetValue)) { | |
| // 递归合并对象 | |
| merge(targetValue, sourceValue) | |
| } else { | |
| // 目标属性不是对象，创建新对象并递归合并 | |
| target[key as keyof typeof target] = {} | |
| merge(target[key as keyof typeof target], sourceValue) | |
| } | |
| } else { | |
| // 直接赋值非对象或数组等类型的值 | |
| target[key as keyof typeof target] = sourceValue | |
| } | |
| }) | |
| } | |
| const target = {} | |
| for (const source of sources) { | |
| if (source !== null && source !== undefined) { | |
| merge(target, source) | |
| } | |
| } | |
| return target | |
| } |
| <tiny-config-provider :design="childDesign1"> | ||
| <div class="demo-form"> | ||
| <tiny-alert type="warning" description="全局配置组件的默认行为"></tiny-alert> | ||
| <tiny-form ref="ruleFormRef" :model="formData"> |
There was a problem hiding this comment.
Multiple forms use the same ref identifier
All three forms use the same ref name ruleFormRef, but only one will be accessible in your methods. When you call this.$refs.ruleFormRef.validate(), it will only validate the last form that was rendered.
Fix this by giving each form a unique ref name:
-<tiny-form ref="ruleFormRef" :model="formData">
+<tiny-form ref="ruleFormRef1" :model="formData">And similar changes for the other forms. Then update the method to target a specific form or validate all forms.
Also applies to: 27-27, 46-46
| <tiny-config-provider :design="childDesign1"> | ||
| <div class="demo-form"> | ||
| <tiny-alert type="warning" description="全局配置组件的默认行为"></tiny-alert> | ||
| <tiny-form ref="ruleFormRef" :model="formData"> |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
@hashiqi12138 代码有冲突哈 |
@hashiqi12138 There is a conflict in the code |
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #3086
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit