fix: deDuplicated BuiltinComponentsMap #1073
Conversation
1. Builtin 组件映射去重
|
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/vue-generator/src/plugins/parseSchemaPlugin.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/vue-generator/.eslintrc.cjs
packages/vue-generator/src/utils/generateImportStatement.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/vue-generator/.eslintrc.cjs
packages/vue-generator/test/testcases/generator/mockData.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/vue-generator/.eslintrc.cjs
WalkthroughThe pull request introduces several improvements across the Vue generator. The Changes
Sequence Diagram(s)sequenceDiagram
participant PS as parseSchema()
participant CM as schema.componentsMap
participant BCM as BUILTIN_COMPONENTS_MAP
PS->>BCM: Iterate over each built-in component
loop For each component in BCM
PS->>CM: Check if component exists (match by name and package)
alt Component not present
PS->>CM: Add component to componentsMap
else
Note over PS,CM: Skip duplicate component
end
end
sequenceDiagram
participant G as generateImportByPkgName()
participant Seen as seen Set
participant Imp as Import Statement Generator
G->>Seen: Check if component/alias already processed
alt Not in seen
Seen->>Seen: Add component/alias
G->>Imp: Generate and return import statement
else Component already seen
G->>Imp: Skip and return empty string
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/vue-generator/src/plugins/parseSchemaPlugin.js (2)
18-27: Consider optimizing the deduplication logic.While the deduplication logic correctly prevents duplicate components by checking both componentName and package, the current implementation using
find()in a loop could be inefficient for large component maps.Consider using a Map for better performance:
- for (const builtInComp of BUILTIN_COMPONENTS_MAP) { - if ( - !schema.componentsMap.find( - ({ componentName, package: packageName }) => - componentName === builtInComp.componentName && packageName === builtInComp.package - ) - ) { - schema.componentsMap.push(builtInComp) - } - } + const existingComps = new Map( + schema.componentsMap.map(comp => [ + `${comp.componentName}:${comp.package}`, + true + ]) + ); + + for (const builtInComp of BUILTIN_COMPONENTS_MAP) { + const key = `${builtInComp.componentName}:${builtInComp.package}`; + if (!existingComps.has(key)) { + schema.componentsMap.push(builtInComp); + existingComps.set(key, true); + } + }
Line range hint
42-42: Consider documenting or adjusting the arbitrary depth limit.The
depth < 1000check seems arbitrary. Consider either:
- Documenting why this specific limit was chosen, or
- Making it a named constant with documentation, or
- Implementing a more meaningful limit based on actual requirements
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/vue-generator/src/generator/vue/sfc/genSetupSFC.js(1 hunks)packages/vue-generator/src/plugins/parseSchemaPlugin.js(1 hunks)packages/vue-generator/test/testcases/generator/expected/appdemo01/package.json(1 hunks)packages/vue-generator/test/testcases/generator/expected/appdemo01/src/router/index.js(1 hunks)packages/vue-generator/test/testcases/generator/expected/appdemo01/src/views/testCanvasRowCol.vue(1 hunks)packages/vue-generator/test/testcases/generator/mockData.js(1 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- packages/vue-generator/test/testcases/generator/expected/appdemo01/src/router/index.js
- packages/vue-generator/test/testcases/generator/expected/appdemo01/src/views/testCanvasRowCol.vue
- packages/vue-generator/src/generator/vue/sfc/genSetupSFC.js
- packages/vue-generator/test/testcases/generator/mockData.js
🧰 Additional context used
🪛 Gitleaks (8.21.2)
packages/vue-generator/test/testcases/generator/mockData.js
2049-2049: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/vue-generator/test/testcases/generator/expected/appdemo01/package.json (2)
14-15: Good improvement: Fixed versions instead of "latest".Replacing "latest" with fixed versions (0.1.16) for @opentiny/vue and @opentiny/vue-icon improves build reproducibility and prevents unexpected breaking changes.
22-22: Verify the new dependency's compatibility.The addition of @opentiny/tiny-engine-builtin-component aligns with the PR's objective of managing built-in components.
Let's verify the compatibility of this new dependency:
✅ Verification successful
Dependency @opentiny/tiny-engine-builtin-component@2.0.0 is compatible
The dependency is stable with minimal requirements and aligns with the project's Vue ecosystem. Both 2.0.0 and 2.1.0 versions have identical dependencies, indicating no breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the specified version exists and get its publish date curl -s https://registry.npmjs.org/@opentiny/tiny-engine-builtin-component | jq '."dist-tags", ."time"."2.0.0"'Length of output: 240
Script:
#!/bin/bash # Get detailed package info for versions 2.0.0 and 2.1.0 curl -s https://registry.npmjs.org/@opentiny/tiny-engine-builtin-component | jq '[.versions["2.0.0"], .versions["2.1.0"]] | map({version: .version, peerDependencies, dependencies})'Length of output: 518
Script:
#!/bin/bash # Find and check Vue version in package.json files fd package.json | xargs cat | jq -r 'select(.dependencies.vue or .devDependencies.vue or .peerDependencies.vue) | {name: .name, vue: (.dependencies.vue // .devDependencies.vue // .peerDependencies.vue)}'Length of output: 3920
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/vue-generator/src/utils/generateImportStatement.js (1)
20-44: Good use of Set for deduplication.The implementation effectively prevents duplicate imports by tracking both component names and aliases. This aligns well with the PR's objective of deduplicating the BuiltinComponentsMap.
However, consider adding a comment explaining the deduplication logic:
+ // Track seen components and aliases to prevent duplicates in import statements const seen = new Set()packages/vue-generator/test/testcases/generator/mockData.js (1)
1879-2050: Well-structured grid layout schema.The new testCanvasRowCol schema demonstrates good use of nested grid components with proper gap spacing and responsive behavior. The structure follows a logical hierarchy of container > row > column.
However, consider adding comments to document the purpose of this test case:
{ + // Test case for grid layout using CanvasRowColContainer + // Demonstrates nested rows and columns with responsive behavior state: { dataDisk: [1, 2, 3] }, ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/vue-generator/src/plugins/parseSchemaPlugin.js(1 hunks)packages/vue-generator/src/utils/generateImportStatement.js(1 hunks)packages/vue-generator/test/testcases/generator/mockData.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vue-generator/src/plugins/parseSchemaPlugin.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/vue-generator/src/utils/generateImportStatement.js (1)
1-16: LGTM! The base import statement generation looks good.The function correctly handles different import scenarios including aliasing and destructuring.
packages/vue-generator/test/testcases/generator/mockData.js (1)
1880-1882: Verify the purpose of dataDisk state.The state contains a dataDisk array [1, 2, 3] but its purpose is not clear from the schema. Consider documenting its usage or remove if unused.
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
【问题描述】
页面中存在多个 CanvasCol、CanvasRow、CanvasRowColContainer 组件的时候，会重复生成多个相同的 import 语句。
【问题分析】
componentsMap 中包含多个内置组件（被导入了多次）。
比如：
然后页面中，包含多个
CanvasRow组件。就会生成重复的：import { CanvasRow, CanvasRow } from '@opentiny/tiny-engine-builtin-component'【解决方案】
生成 import 语句时，需要进行查重。
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Builtin 组件映射去重
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Chores