feat：route code output #937
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/vue-generator/src/plugins/genRouterPlugin.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/vue-generator/.eslintrc.cjs
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CanvasRouterView
participant CanvasPlaceholder
User->>CanvasRouterView: Render with tag prop
CanvasRouterView->>CanvasRouterView: Determine HTML tag
alt No content in slot
CanvasRouterView->>CanvasPlaceholder: Display placeholder
else Content exists
CanvasRouterView->>User: Render with provided content
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/vue-generator/src/plugins/genRouterPlugin.js (1)
Line range hint
97-115: Consider adding configurable routing modes.
The TODO comment at line 117 references supporting "hash" vs. "history" modes. Currently, "createWebHashHistory" is hard-coded. Exposing a plugin option to toggle between hash and history modes might be helpful for different deployment contexts.const exportSnippet = ` export default createRouter({ - history: createWebHashHistory(), + history: ${/* e.g., pass in an option or fallback to hash mode */ 'createWebHistory()'}, routes })`packages/canvas/render/src/render.js (1)
Line range hint
191-234: Enhance error handling robustnessWhile the current implementation handles both sync and async errors, there are opportunities for improvement:
- Error messages could be more specific
- The default response structure assumes table data format
- The sourceId check logic could be simplified
Consider this improved implementation:
export const generateFn = (innerFn, context) => { return (...args) => { - // 如果有数据源标识，则表格的fetchData返回数据源的静态数据 - const sourceId = collectionMethodsMap[innerFn.realName || innerFn.name] - if (sourceId) { - return innerFn.call(context, ...args) - } else { - let result = null + const sourceId = collectionMethodsMap[innerFn.realName || innerFn.name]; + let result = null; - // 这里是为了兼容用户写法报错导致画布异常，但无法捕获promise内部的异常 - try { - result = innerFn.call(context, ...args) - } catch (error) { - globalNotify({ - type: 'warning', - title: `函数:${innerFn.name}执行报错`, - message: error?.message || `函数:${innerFn.name}执行报错，请检查语法` - }) - } + try { + result = innerFn.call(context, ...args); + } catch (error) { + const fnName = innerFn.name || 'Anonymous'; + globalNotify({ + type: 'warning', + title: `Synchronous Execution Error in ${fnName}`, + message: error?.message || 'Syntax error detected' + }); + return sourceId ? null : {}; + } - // 这里注意如果innerFn返回的是一个promise则需要捕获异常，重新返回默认一条空数据 - if (result.then) { - result = new Promise((resolve) => { - result.then(resolve).catch((error) => { - globalNotify({ - type: 'warning', - title: '异步函数执行报错', - message: error?.message || '异步函数执行报错，请检查语法' - }) - // 这里需要至少返回一条空数据，方便用户使用表格默认插槽 - resolve({ - result: [{}], - page: { total: 1 } - }) - }) - }) - } + if (result?.then) { + return result.catch((error) => { + const fnName = innerFn.name || 'Anonymous'; + globalNotify({ + type: 'warning', + title: `Asynchronous Execution Error in ${fnName}`, + message: error?.message || 'Promise rejection occurred' + }); + return sourceId ? null : {}; + }); + } - return result - } + return result; } }Key improvements:
- More descriptive error messages distinguishing sync vs async errors
- Simplified control flow by removing nested conditions
- Consistent error response format regardless of source
- Added function name to error messages for better debugging
🧹 Nitpick comments (4)
packages/vue-generator/src/plugins/genRouterPlugin.js (1)
42-94: Ensure stable sorting and consistent router usage.
In "convertToNestedRoutes", the array is sorted with a simple comparison on "meta.router.length". This might be acceptable but be aware that JavaScript’s sort is not guaranteed to be stable, and items with the same length could be reordered unpredictably. If the route order is essential, consider using a stable sort or adding tie-break logic.packages/canvas/render/src/builtin/CanvasRouterView.vue (2)
1-7: Validate prop "tag" usage.
Rendering dynamic tags is flexible but can raise accessibility or SEO concerns if misused (e.g., using headings incorrectly). Consider documenting recommended tag usage or adding a console warning when using unusual tags.Would you like an example snippet in the docs explaining best practices when selecting an HTML tag?
9-21: Potential enhancement: watch for changes in "$attrs".
Currently, the component binds all attributes to the rendered element, which is great for flexibility, but if $attrs like "placeholder" changes at runtime, you could add watchers to handle side effects or re-renders if necessary.packages/canvas/render/src/builtin/builtin.json (1)
5-61: Confirm "RouterView" icon usage.
Using the "Box" icon for RouterView could be confusing from a UX standpoint. Consider a more descriptive icon for clarity (e.g., "Router" or "Flow" icon).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/canvas/render/src/builtin/CanvasRouterView.vue(1 hunks)packages/canvas/render/src/builtin/builtin.json(2 hunks)packages/canvas/render/src/builtin/index.js(1 hunks)packages/canvas/render/src/render.js(2 hunks)packages/vue-generator/src/plugins/genRouterPlugin.js(2 hunks)
🔇 Additional comments (5)
packages/vue-generator/src/plugins/genRouterPlugin.js (1)
8-40: Check for robust validation of "route" object properties.
The new "flattenRoutes" function relies on "route.path", "route.component", and "route.children". If any of these properties are missing or incorrect, it could lead to errors (e.g., ".find" on undefined). Consider adding validations or safe optional chaining to handle cases where these properties may be missing.
Would you like a script that scans your codebase to find any usage of "flattenRoutes" that might pass in incomplete route objects?
packages/canvas/render/src/builtin/index.js (1)
20-31: Imports and export look correct.
The addition of "CanvasRouterView" is consistent with existing naming and export patterns for built-in components. No issues found.
packages/canvas/render/src/builtin/builtin.json (1)
518-528: Snippet alignment with the new component.
The snippet for "RouterView" references your new component. This looks correct and consistent, ensuring quick insertion. Just remember to add relevant user-facing documentation describing how to configure the newly introduced RouterView component from this snippet.
packages/canvas/render/src/render.js (2)
31-32: LGTM: Component registration is consistent
The new components CanvasPlaceholder and CanvasRouterView are properly registered in both the imports and the Mapper object, following the established pattern.
Also applies to: 73-74
Line range hint 401-405: Verify the special case handling for CanvasPlaceholder
The early return of empty props for CanvasPlaceholder needs verification:
- Why does
CanvasPlaceholderrequire special handling? - Could this hide potential issues with prop binding?
- Are there any side effects of skipping the standard prop processing?
Let's analyze the usage of CanvasPlaceholder:
✅ Verification successful
Let me gather more information about the CanvasPlaceholder component itself:
Based on the gathered information, I can now provide the final response:
Special case for CanvasPlaceholder is justified and safe
The special case handling of CanvasPlaceholder with empty props is correct because:
- CanvasPlaceholder is a simple presentational component that only accepts an optional
placeholderprop with a default value - It serves as a drop target container in the canvas system, used by multiple parent components (CanvasBox, CanvasCollection, CanvasSlot, CanvasRouterView)
- The empty props return ({}) aligns with its minimal interface, preventing unnecessary prop processing for this utility component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of CanvasPlaceholder to understand its context
echo "Searching for CanvasPlaceholder component definition..."
ast-grep --pattern 'class CanvasPlaceholder {
$$$
}'
echo "Searching for CanvasPlaceholder usage in templates..."
rg -A 5 'CanvasPlaceholder'
echo "Searching for places where CanvasPlaceholder is injected..."
ast-grep --pattern 'componentName: "CanvasPlaceholder"'
Length of output: 5405
Script:
#!/bin/bash
# Check the CanvasPlaceholder.vue implementation
echo "Content of CanvasPlaceholder.vue:"
cat packages/canvas/render/src/builtin/CanvasPlaceholder.vue
# Find where it's used in templates
echo -e "\nSearching for CanvasPlaceholder template usage:"
rg -l '<CanvasPlaceholder' packages/canvas/render/src/
# Look for any props passed to CanvasPlaceholder
echo -e "\nSearching for props passed to CanvasPlaceholder:"
rg -A 2 '<CanvasPlaceholder.*?>'
Length of output: 854
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
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
CanvasRouterViewcomponent for dynamic HTML element rendering.RouterView, with a corresponding snippet for easy inclusion.Bug Fixes
Refactor
Chores
CanvasRouterView.