Refactor: split renderer #888
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on the integration of a new renderer package, Changes
Suggested labels
Suggested reviewers
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: 10
🧹 Outside diff range and nitpick comments (13)
packages/renderer/src/context.js (1)
22-22: Consider enhancing documentation and using English comments.
The Chinese comment should be translated to English for better international collaboration and maintainability.
-let canvasFlag = true // 是否表现画布内特征的标志，如拖拽事件、原生事件是否触发等
+let canvasFlag = true // Flag controlling canvas features such as drag events and native event triggerspackages/renderer/package.json (3)
2-3: Consider starting with version 1.0.0-alpha.1 for the new package.
For a newly extracted package, it's generally recommended to start with version 1.0.0-alpha.1 to indicate it's the first major version. The current version 2.0.0-alpha.4 suggests this is a second major version, which might be confusing for a new package.
7-11: Add test and lint scripts to ensure code quality.
The scripts section is missing common development workflow commands. Consider adding:
"scripts": {
"dev": "vite",
"build": "vite build",
- "preview": "vite preview"
+ "preview": "vite preview",
+ "test": "vitest",
+ "lint": "eslint .",
+ "type-check": "vue-tsc --noEmit"
}47-53: Add essential development dependencies.
The devDependencies section is missing some important tools for Vue development:
"devDependencies": {
"@opentiny/tiny-engine-vite-plugin-meta-comments": "workspace:*",
"@vitejs/plugin-vue": "^5.1.2",
"@vitejs/plugin-vue-jsx": "^4.0.1",
"rollup-plugin-polyfill-node": "^0.13.0",
- "vite": "^5.4.2"
+ "vite": "^5.4.2",
+ "vitest": "^1.0.0",
+ "vue-tsc": "^1.8.0",
+ "eslint": "^8.0.0",
+ "eslint-plugin-vue": "^9.0.0"
}packages/canvas/render/src/supportUmdBlock.js (1)
Line range hint 13-63: Consider isolating global window assignments.
While the changes look good, the function mutates the global window object directly. Consider:
- Isolating these global assignments to a separate function
- Adding a warning when running in strict mode
- Documenting the side effects in JSDoc
This would improve maintainability and make the global mutations more explicit.
Here's a suggested refactoring:
+ /**
+ * Extends the global window object with Vue-related properties.
+ * @warning This function mutates the global window object
+ */
+ function extendGlobalScope() {
+ if (typeof window === 'undefined') {
+ console.warn('supportUmdBlock: window is undefined');
+ return;
+ }
+ window.VueI18n = VueI18n;
+ window.TinyVue = TinyVue;
+ window.TinyVueIcon = TinyVueIcon;
+ window.TinyWebcomponentCore = TinyWebcomponentCore;
+ window.TinyI18nHost = TinyI18nHost;
+ }
export function supportUmdBlock() {
// ... existing Vue extension code ...
- window.VueI18n = VueI18n
- window.TinyVue = TinyVue
- window.TinyVueIcon = TinyVueIcon
- window.TinyWebcomponentCore = TinyWebcomponentCore
- window.TinyI18nHost = TinyI18nHost
+ extendGlobalScope();
}packages/renderer/src/builtin/CanvasCollection.vue (1)
Line range hint 36-106: Consider improving component maintainability.
The component could benefit from several structural improvements:
- Extract the isEmpty watch logic into a reusable composable
- Add error handling for fetchDataSourceDetail
- Consolidate handler management logic that's currently spread across watchers
Example error handling implementation:
if (props.dataSource) {
- fetchDataSourceDetail(props.dataSource).then((res) => {
- source.value = res
- })
+ fetchDataSourceDetail(props.dataSource)
+ .then((res) => {
+ source.value = res
+ })
+ .catch((error) => {
+ console.error('Failed to fetch data source:', error)
+ source.value = null
+ })
}packages/canvas/render/src/lowcode.js (1)
Line range hint 71-82: Consider enhancing error handling in the wrap function.
The error handling for accessor functions could be improved by:
- Adding more specific error types
- Including the function arguments in the error message for better debugging
- Implementing consistent error reporting across all function types
Consider applying this improvement:
return (...args) => {
try {
fn.apply(global, args)
} catch (error) {
globalNotify({
type: 'warning',
- title: `访问器函数:${fn.name}执行报错`,
- message: error?.message || `访问器函数:${fn.name}执行报错，请检查语法`
+ title: `Accessor function "${fn.name}" failed`,
+ message: `Error executing ${fn.name}(${args.join(', ')}): ${error?.message || 'Syntax error'}`
})
}
}packages/renderer/README.md (2)
1-8: Enhance the introduction section with better structure and clarity.
The introduction could be improved to make it more professional and easier to read:
# tiny-engine-canvas
+
+## Overview
+
+The tiny-engine-canvas module is a rendering engine that consists of two main components:
+- **Canvas Container**: The outer component that manages the rendering context
+- **Canvas**: The core rendering component that operates within an iframe
## build
-
-Note: tiny-engine-canvas module contains two parts， *canvas-container* and *canvas*, and canvas is rendered in iframe window
+Note: The tiny-engine-canvas module contains two parts: *canvas-container* and *canvas*. The canvas is rendered in an iframe window.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: # tiny-engine-canvas ## build Note: tiny-engine-canvas module contains two parts...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~5-~5: You might be missing the article “an” here.
Context: ...and canvas, and canvas is rendered in iframe window During the build phase, they are...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
1-36: Add missing essential documentation sections.
The README would benefit from additional standard sections that are currently missing:
Consider adding these sections:
- Installation instructions
- API documentation
- Troubleshooting guide
- Contributing guidelines
- Version compatibility matrix
- Performance considerations
Would you like me to help generate templates for these missing sections?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: # tiny-engine-canvas ## build Note: tiny-engine-canvas module contains two parts...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~5-~5: You might be missing the article “an” here.
Context: ...and canvas, and canvas is rendered in iframe window During the build phase, they are...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[style] ~20-~20: Consider using a different adverb to strengthen your wording.
Context: ...side the canvas. (That is, we can use completely different versions of vue inside or o...
(COMPLETELY_ENTIRELY)
packages/renderer/src/RenderMain.js (1)
467-474: Consider grouping related API functions
The new API exports could be better organized by grouping related functions together. For example:
- Canvas-related:
getCanvasFlag,setCanvasFlag - Rendering utilities:
generateFn,getComponent - Block-related:
getBlockSlotDataMap,getCollectionMethodsMap
This would improve maintainability and make the API surface more intuitive.
packages/canvas/container/src/container.js (1)
Line range hint 32-38: Consider adding unit tests for drag-and-drop edge cases.
The drag-and-drop implementation in this file handles complex scenarios like absolute positioning, container rules, and nested elements. Consider adding comprehensive unit tests to ensure reliability.
Would you like me to help generate unit test cases for the drag-and-drop functionality?
Also applies to: 600-600
packages/renderer/src/render.js (2)
60-61: Consider adding JSDoc comments for 'getBlockSlotDataMap'
To enhance maintainability and improve code clarity, consider adding JSDoc comments to the new exported function getBlockSlotDataMap.
84-85: Consider adding JSDoc comments for 'getCollectionMethodsMap'
Adding JSDoc comments to the new exported function getCollectionMethodsMap would improve readability and assist other developers in understanding its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- jsconfig.json (1 hunks)
- mockServer/src/mock/get/app-center/v1/apps/schema/918.json (0 hunks)
- packages/build/vite-config/src/vite-plugins/devAliasPlugin.js (1 hunks)
- packages/canvas/container/src/container.js (1 hunks)
- packages/canvas/render/src/lowcode.js (2 hunks)
- packages/canvas/render/src/runner.js (1 hunks)
- packages/canvas/render/src/supportUmdBlock.js (2 hunks)
- packages/renderer/.eslintrc.cjs (1 hunks)
- packages/renderer/.gitignore (1 hunks)
- packages/renderer/README.md (1 hunks)
- packages/renderer/package.json (1 hunks)
- packages/renderer/src/CanvasEmpty.vue (1 hunks)
- packages/renderer/src/RenderMain.js (4 hunks)
- packages/renderer/src/builtin/CanvasCollection.js (2 hunks)
- packages/renderer/src/builtin/CanvasCollection.vue (1 hunks)
- packages/renderer/src/constants.js (1 hunks)
- packages/renderer/src/context.js (2 hunks)
- packages/renderer/src/index.js (1 hunks)
- packages/renderer/src/render.js (5 hunks)
- packages/renderer/vite.config.js (1 hunks)
💤 Files with no reviewable changes (1)
- mockServer/src/mock/get/app-center/v1/apps/schema/918.json
✅ Files skipped from review due to trivial changes (5)
- packages/renderer/.eslintrc.cjs
- packages/renderer/.gitignore
- packages/renderer/src/CanvasEmpty.vue
- packages/renderer/src/constants.js
- packages/renderer/src/index.js
🧰 Additional context used
🪛 LanguageTool
packages/renderer/README.md
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: # tiny-engine-canvas ## build Note: tiny-engine-canvas module contains two parts...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~5-~5: You might be missing the article “an” here.
Context: ...and canvas, and canvas is rendered in iframe window During the build phase, they are...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[style] ~20-~20: Consider using a different adverb to strengthen your wording.
Context: ...side the canvas. (That is, we can use completely different versions of vue inside or o...
(COMPLETELY_ENTIRELY)
🔇 Additional comments (24)
packages/renderer/vite.config.js (1)
1-51: Verify integration with the main project build process.
The Vite configuration appears well-structured for building the renderer as a standalone package. However, please ensure that:
- This configuration aligns with the main project's build process
- The output format (ES modules) is compatible with all consuming applications
- The external dependencies list matches the package.json dependencies
✅ Verification successful
Let me analyze the build configuration alignment by checking other Vite configs in the project.
This will help verify:
- The main build configuration patterns
- The canvas-specific configuration referenced in the renderer
- Consistency of external dependency handling across packages
Build configuration is properly aligned with the project standards
The verification shows that:
- The external dependencies in
packages/renderer/vite.config.jsmatch the pattern used across other packages, particularly matchingpackages/canvas/vite.config.jsexactly - The ES module output format aligns with the
"type": "module"in package.json - All external dependencies declared in the Vite config are properly listed in package.json's peer/direct dependencies
- The build configuration follows the same patterns as other packages in the monorepo, including the base build config
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify build configuration alignment with project
# Check for other Vite configs in the project
echo "Checking other Vite configurations:"
fd -e js -e ts "vite.config"
# Check package.json dependencies
echo -e "\nChecking package dependencies:"
cat packages/renderer/package.json || echo "package.json not found"
Length of output: 4239
Script:
#!/bin/bash
# Check the build configuration of the main project and other related packages
echo "Checking build config in build package:"
cat packages/build/vite-config/vite.config.js
echo -e "\nChecking canvas vite config:"
cat packages/renderer/scripts/canvas-vite.config.js
# Check if other packages are using similar external dependencies
echo -e "\nChecking external dependencies in other configs:"
rg "external.*vue.*@opentiny" -A 2 packages/*/vite.config.js
Length of output: 3941
packages/renderer/src/context.js (1)
Line range hint 22-64: Verify the impact of this new flag across the codebase.
The introduction of canvasFlag could affect various canvas-related features. Let's verify its usage and potential impact.
✅ Verification successful
Let me analyze the usage of canvasFlag more deeply by checking its implementation context.
The canvasFlag is safely implemented and has a well-defined scope
The verification shows that canvasFlag is used specifically to control event propagation behavior in the canvas:
- It's used in
render.jsto conditionally attach event handlers (onMouseover,onFocus,onClickCapture) that stop event propagation - The implementation follows a clear pattern of preventing default browser events when the canvas is in an editable state
- The flag's usage is centralized and doesn't have unexpected side effects across the codebase
The changes are safe and follow the existing event handling patterns in the renderer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential canvas-related features that might be affected by this flag
echo "Searching for canvas-related event handlers and drag functionality..."
rg -l "drag|ondrag|mousedown|mousemove|mouseup" --type js
# Search for existing usage of the new flag
echo "Checking usage of canvasFlag..."
rg "getCanvasFlag|setCanvasFlag" --type js
Length of output: 2467
Script:
#!/bin/bash
# Check the implementation details in render.js where canvasFlag is used
echo "Checking render.js implementation..."
rg -A 5 "getCanvasFlag\(\)" packages/renderer/src/render.js
# Check how it's used in RenderMain.js
echo "Checking RenderMain.js implementation..."
rg -A 5 "getCanvasFlag|setCanvasFlag" packages/renderer/src/RenderMain.js
Length of output: 825
packages/renderer/package.json (1)
54-60: LGTM! Peer dependencies are well-defined.
The peer dependencies correctly specify the Vue ecosystem requirements with appropriate version ranges.
packages/canvas/render/src/supportUmdBlock.js (1)
8-8: LGTM! Clean import refactoring.
The import changes align well with the PR objective of extracting rendering logic into a standalone package.
Also applies to: 10-10
packages/renderer/src/builtin/CanvasCollection.vue (1)
11-11: LGTM! Verify the new module path.
The change from direct controller import to using the centralized api object aligns well with the refactoring objectives.
Let's verify the new module exists and exports the expected api object:
✅ Verification successful
Import change is valid and correctly structured
The verification confirms that:
- The
RenderMain.jsmodule exists and exports theapiobject containing multiple methods and utilities - The module is properly re-exported through
index.js - The import path
../RenderMaininCanvasCollection.vuecorrectly references this module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and exports of RenderMain module
# Expected: Find the api export in RenderMain.js/ts/vue
rg -t js -t ts -t vue "export.*api" "packages/renderer/src/RenderMain"
Length of output: 98
Script:
#!/bin/bash
# Let's try different approaches to verify the module and its exports
# 1. First, check if the RenderMain file exists with any extension
fd "RenderMain" "packages/renderer/src"
# 2. Search for api export pattern without file type restrictions
rg "export.*api" "packages/renderer/src/RenderMain"
# 3. Search for api export pattern in the entire renderer package
rg "export.*api" "packages/renderer"
# 4. Look for imports of RenderMain to understand its usage
rg "from.*RenderMain.*api" "packages/renderer"
Length of output: 394
Script:
#!/bin/bash
# Let's examine the RenderMain.js file content to verify the api export
cat packages/renderer/src/RenderMain.js
# Also check the index.js that re-exports the api
cat packages/renderer/src/index.js
Length of output: 11731
packages/canvas/render/src/runner.js (1)
16-16: LGTM! Verify package resolution in different environments.
The import change aligns with the PR objective of modularizing the renderer. However, let's verify the package resolution works correctly.
✅ Verification successful
Package resolution is properly configured and working
The import change is safe and properly configured:
- Package exists with correct structure and entry point at
packages/renderer/src/index.js - Package resolution is configured in jsconfig.json and build tools
- The package exports the required
Rendereras default andapinamed export - Multiple files in the codebase are already using this import pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the package resolution configuration exists
# Test 1: Check if the package is properly configured in jsconfig.json
echo "Checking jsconfig.json configuration..."
rg -l "@opentiny/tiny-engine-renderer" --type json
# Test 2: Check if the package is properly configured in build/dev tools
echo "Checking build/dev configuration..."
fd -e js -e ts "devAliasPlugin|vite\.config" --exec rg "@opentiny/tiny-engine-renderer" {}
Length of output: 454
Script:
#!/bin/bash
# Let's verify the package structure and dependencies
# Check if the renderer package exists and its structure
echo "Checking renderer package structure..."
fd . packages/renderer -t f
# Check package.json of renderer for main entry and dependencies
echo -e "\nChecking renderer package.json..."
cat packages/renderer/package.json
# Check the actual entry point exists
echo -e "\nVerifying entry point exists..."
cat packages/renderer/src/index.js
# Check if the import is used in other places
echo -e "\nChecking usage across codebase..."
rg "@opentiny/tiny-engine-renderer" -l
Length of output: 3828
packages/canvas/render/src/lowcode.js (3)
Line range hint 1-106: Overall implementation looks good!
The changes successfully move towards a more modular architecture by:
- Properly separating the renderer into its own package
- Maintaining consistent access patterns
- Preserving existing functionality
The implementation aligns well with the PR's objective of refactoring the rendering logic into a standalone package.
65-68: Review the function name manipulation logic.
The code slices the function name to handle build-time modifications. This approach is fragile and could break if the build process changes.
Consider documenting this behavior or implementing a more robust solution.
15-17: Verify package dependency configuration.
The import from @opentiny/tiny-engine-renderer aligns with the PR's modularization goal. However, we should ensure this dependency is properly configured.
✅ Verification successful
Import from @opentiny/tiny-engine-renderer is properly configured
The import is valid as this is a pnpm monorepo workspace setup where:
- The renderer package exists as
@opentiny/tiny-engine-renderer(verified in packages/renderer/package.json) - Workspace dependencies are managed through pnpm-workspace.yaml
- The canvas package (containing lowcode.js) and renderer package are both part of the workspace
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package.json configuration for the new dependency
# Test 1: Check if the dependency is declared in package.json
echo "Checking package.json for @opentiny/tiny-engine-renderer dependency..."
jq '.dependencies["@opentiny/tiny-engine-renderer"] // .devDependencies["@opentiny/tiny-engine-renderer"]' package.json
# Test 2: Verify the import path exists in jsconfig.json or tsconfig.json paths
echo "Checking for path configuration..."
fd -e json -x jq -r '.compilerOptions.paths["@opentiny/tiny-engine-renderer"]' {} \;
Length of output: 4208
Script:
#!/bin/bash
# Let's try a different approach to verify the dependency configuration
# Check package.json in the canvas/render package directory
echo "Checking canvas/render package.json..."
cat packages/canvas/render/package.json
# Check if the renderer package exists and its package.json
echo -e "\nChecking renderer package..."
cat packages/renderer/package.json 2>/dev/null || echo "Renderer package not found"
# Look for workspace configuration
echo -e "\nChecking workspace configuration..."
cat package.json | jq '.workspaces'
# Check for any lerna.json
echo -e "\nChecking lerna configuration..."
cat lerna.json 2>/dev/null || echo "No lerna.json found"
Length of output: 2491
Script:
#!/bin/bash
# Let's check the actual package location and its dependencies
# Find the correct package directory
echo "Looking for lowcode.js file location..."
fd lowcode.js
# Once found, let's check its package.json
echo -e "\nChecking package.json in the canvas directory..."
cat packages/canvas/package.json 2>/dev/null || echo "Not found in canvas directory"
# Check if using pnpm workspace
echo -e "\nChecking for pnpm workspace config..."
cat pnpm-workspace.yaml 2>/dev/null || echo "No pnpm-workspace.yaml found"
Length of output: 2544
packages/renderer/README.md (2)
23-36: Ensure consistency between English and Chinese documentation.
The Chinese translation is comprehensive but could be better structured to match the English section's organization. Consider maintaining parallel structure between both language sections.
Consider adding clear section separators:
+---
+
## 构建Let's verify the consistency of technical terms between English and Chinese sections:
10-18: Improve the development setup documentation with a structured format.
The development setup instructions could be clearer with a more organized structure:
-Develop and debug the canvas module in development mode requires:
+## Development Setup
+### Prerequisites
1) Setting the Vite config with `devAlias` (`resolve.alias`), which points the canvas package name to the canvas module source code.
2) Using the `canvas-dev-external` plugin.
+### Technical Details
Vite uses esbuild in development. The `devAlias` configuration makes the source code of the canvas module work and allows for joint debugging.Let's verify if the development setup matches the actual configuration:
✅ Verification successful
Let's verify if the development setup matches the actual configuration by searching for the presence of the canvas-dev-external plugin and devAlias configuration in the codebase.
Let's examine the actual implementation of the canvas-dev-external plugin and resolve.alias configuration in the vite config files to verify the development setup documentation.
Let's examine the implementation of the canvas-dev-external plugin and the alias configuration to verify the development setup documentation.
Documentation matches the implementation with minor improvements needed.
The development setup documentation is accurate but could be more detailed based on the implementation found in the codebase:
- The
devAliasconfiguration is implemented through thedevAliasPluginwhich is only active in development (serve) mode - The
canvas-dev-externalplugin is correctly used in conjunction withdevAliaswhenuseSourceAliasis enabled - The implementation shows additional functionality not mentioned in the docs:
- Handling of block WebComponent loading
- Special handling for @opentiny/vue dependencies
- Automatic importmap generation for node_modules dependencies
-Develop and debug the canvas module in development mode requires:
+## Development Setup for Canvas Module
+### Prerequisites
1) Setting the Vite config with `devAlias` (`resolve.alias`), which points the canvas package name to the canvas module source code.
2) Using the `canvas-dev-external` plugin.
+### Technical Details
Vite uses esbuild in development. The `devAlias` configuration makes the source code of the canvas module work and allows for joint debugging.
However, esbuild won't perceive the external configuration for rollup; it will resolve `vue` and other dependencies (that we originally wanted to exclude and let them naturally point to the package addresses in the `importmap`) to `node_modules`.
For this reason, we need the `canvas-dev-external` plugin. It can externalize the dependencies specified in the `importmap` of *canvas* for esbuild.
On the other hand, externalizing the dependencies will affect all other packages in the same runtime. This plugin will generate another `importmap` that points all affected dependencies to `node_modules` to eliminate the side effects.
+
+### Additional Features
+- Handles block WebComponent loading with required externals
+- Special handling for @opentiny/vue dependencies
+- Automatic importmap generation for node_modules dependencies🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the presence of canvas-dev-external plugin and devAlias configuration
rg -l "canvas-dev-external"
fd "vite.config" -x cat {} \; | rg "devAlias|resolve.alias"
Length of output: 159004
Script:
#!/bin/bash
# Search for canvas-dev-external plugin and devAlias configuration
rg -l "canvas-dev-external"
fd "vite.config" -x cat {} \; | rg "devAlias|resolve.alias|canvas-dev-external"
Length of output: 299
Script:
#!/bin/bash
# Let's check the content of the files that mention canvas-dev-external
rg -A 5 -B 5 "canvas-dev-external" packages/build/vite-config/src/default-config.js
rg -A 5 -B 5 "resolve.alias" packages/build/vite-config/src/default-config.js
Length of output: 836
Script:
#!/bin/bash
# Let's check the content of the canvas-dev-external.js file and look for resolve.alias in the entire vite-config directory
cat packages/build/vite-config/src/canvas-dev-external.js
rg -A 5 -B 5 "resolve.alias|devAlias" packages/build/vite-config/
Length of output: 6258
jsconfig.json (1)
10-10: LGTM! The path mapping follows the established pattern.
The new path mapping for @opentiny/tiny-engine-renderer aligns with the PR objective of extracting the rendering logic into a standalone package and follows the same pattern as other package mappings in the project.
Let's verify that the target path exists:
✅ Verification successful
Path mapping is valid and correctly configured
The verification confirms that the target path packages/renderer/src/index.js exists, along with an additional index file in the builtin subdirectory. The path mapping in jsconfig.json is correctly configured and points to a valid entry point.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the renderer package and its entry point exist
# Expected: The path should exist and contain an index.js file
fd -t f "index.js" packages/renderer/src/
Length of output: 111
packages/build/vite-config/src/vite-plugins/devAliasPlugin.js (1)
59-59: LGTM! The alias addition aligns with the PR objectives.
The new alias mapping for @opentiny/tiny-engine-renderer correctly integrates the extracted renderer package into the development build configuration, supporting the PR's goal of modularizing the rendering logic.
Let's verify the renderer package structure:
✅ Verification successful
The alias configuration is correctly set up and actively used across the codebase
The verification confirms:
- The renderer package exists at the expected location with proper structure
- The package is correctly named in its package.json
- The alias is consistently used across multiple components:
- Canvas render runner
- Canvas render support
- Canvas lowcode
- Canvas container
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the renderer package structure and its integration
# Expected: Find the renderer package and its main entry point
# Check if the renderer package exists and has the expected structure
fd --type d "^renderer$" packages/
fd --type f "index.js" packages/renderer/src/
# Verify that the renderer is being imported using the new alias
rg "@opentiny/tiny-engine-renderer"
Length of output: 916
packages/renderer/src/builtin/CanvasCollection.js (3)
47-47: LGTM! Controller access refactored correctly.
The change to use api.getController() aligns well with the PR's objective of creating a more modular renderer package.
78-78: LGTM! Material configuration access refactored correctly.
The change to use api.getController() for material configuration access maintains consistency with the modular architecture.
Line range hint 1-266: Consider refactoring to reduce code duplication and improve robustness.
Several opportunities for improvement:
- The
genVarNamefunction is duplicated across handlers. Consider extracting it to a shared utility. - State management patterns are repeated. Consider creating a common state management helper.
- Add type checking for sourceRef.value.data structure to prevent runtime errors.
Here's a suggested approach for the shared utilities:
// Shared utilities
const stateManagement = {
genVarName: (prefix, schemaId) => `${prefix}${schemaId}`,
handleState: (pageSchema, variableName, sourceRef, data, options = {}) => {
api.setState({ [variableName]: data })
pageSchema.state[variableName] = data
if (sourceRef.value?.data?.option?.isSync) {
genRemoteMethodToLifeSetup(variableName, sourceRef, pageSchema)
}
}
}
// Usage in handlers
const TinyTreeHandler = ({ node, sourceRef, schemaId, pageSchema }) => {
const variableName = stateManagement.genVarName(NAME_PREFIX.tree, schemaId)
// ... rest of the handler
}Let's verify the impact of this refactoring:
packages/renderer/src/RenderMain.js (2)
19-29: LGTM: Clean import organization
The new imports are well-organized and properly separated between rendering utilities and context management, aligning with the PR's refactoring objectives.
Also applies to: 40-42
467-474: Verify usage of newly exported functions
Let's verify that the newly exported functions are properly used across the codebase to ensure the refactoring is complete.
✅ Verification successful
All newly exported functions are properly used across the codebase
The verification shows that all the newly exported functions are being used appropriately:
generateFn: Used in render.js and lowcode.js for function generationgetCollectionMethodsMap: Used in render.js and lowcode.js for method mappinggetBlockSlotDataMap: Used in supportUmdBlock.js for slot data handlinggetComponent: Used extensively in render.js and supportUmdBlock.js for component resolutiongetCanvasFlag: Used in context.js and render.js for canvas state managementsetCanvasFlag: Defined in context.js and properly exported
The refactoring appears to be complete with all functions being properly imported and used where needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of newly exported functions
echo "Checking usage of new exports..."
for func in "generateFn" "getCollectionMethodsMap" "getBlockSlotDataMap" "getComponent" "getCanvasFlag" "setCanvasFlag"; do
echo "=== Checking $func usage ==="
rg "\b${func}\b" --type js --type vue
done
Length of output: 1784
Script:
#!/bin/bash
# Search for usages of newly exported functions, fixing the file type issue
echo "Checking usage of new exports..."
for func in "generateFn" "getCollectionMethodsMap" "getBlockSlotDataMap" "getComponent" "getCanvasFlag" "setCanvasFlag"; do
echo "=== Checking $func usage ==="
# Using fd to find all JS and Vue files, then using xargs with rg
fd -e js -e vue | xargs rg "\b${func}\b"
done
Length of output: 4281
packages/canvas/container/src/container.js (2)
24-24: LGTM! Import change aligns with refactoring goal.
The change to import Builtin from @opentiny/tiny-engine-renderer is consistent with the PR's objective of extracting renderer logic into a standalone package.
Line range hint 600-600: LGTM! API change maintains compatibility.
The addition of Builtin to canvasApi maintains API compatibility while moving the implementation to the renderer package.
Let's verify that all consumers of this API are updated:
✅ Verification successful
The previous search was too specific. Let's broaden our search to catch any references to "builtin" in the codebase and examine the import patterns more thoroughly.
API change is correctly implemented and consumers are updated
The verification shows that:
- The
Builtinis correctly imported from the renderer package inpackages/canvas/container/src/container.js - The consumers are properly using the new API:
packages/plugins/materials/src/composable/useMaterial.jscorrectly usescanvasApi.Builtin- No other direct imports of the old builtin.json location exist except in the renderer package itself where it's now defined
The API change maintains compatibility and all consumers have been updated to use the new location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of Builtin from the old location
rg -l "from '.*builtin\.json'"
# Search for usage of canvasApi.Builtin to ensure consumers are using the new API
rg "canvasApi\.Builtin"
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for Builtin usage patterns
rg -i "builtin" -A 2
# Also search for any import statements containing builtin
ast-grep --pattern 'import $_ from "$_builtin$_"'
# Check if there are any JSON files that might be related to builtin
fd -e json builtin
Length of output: 16096
packages/renderer/src/render.js (4)
22-23: Imports updated appropriately
The import statements have been correctly updated to include getCanvasFlag and the renamed constants DESIGN_UIDKEY, DESIGN_TAGKEY, and DESIGN_LOOPID.
101-103: Updated function declaration enhances consistency
Changing setController from an arrow function to a function declaration improves consistency and may benefit hoisting behavior within the module.
568-571: Verify the impact of added event handlers on user interactions
The addition of onMouseover and onFocus event handlers that call stopEvent when getCanvasFlag() is true may prevent default behaviors and stop event propagation. Please ensure this change does not unintentionally disrupt user interactions or accessibility features.
578-580: Confirm the conditional addition of 'onClickCapture' handler
The condition for adding the onClickCapture event handler now checks both clickCapture(componentName) and getCanvasFlag(). Verify that this aligns with the intended behavior and does not adversely affect click event handling in components.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/canvas/container/src/CanvasContainer.vue (1)
134-140: LGTM! Consider caching the canvas flag.
The canvas flag check is a good defensive measure to prevent event processing outside the canvas context. However, since this flag is accessed in multiple event handlers, consider caching it at the component level to avoid repeated getter calls.
setup(props, { emit }) {
const iframe = ref(null)
const insertPanel = ref(null)
const insertPosition = ref(false)
+ const canvasFlag = computed(() => canvasApi.getRenderer().getCanvasFlag())
win.addEventListener('mousedown', (event) => {
- const canvasFlag = canvasApi.getRenderer().getCanvasFlag()
-
// 非画布不触发节点事件
- if (!canvasFlag) {
+ if (!canvasFlag.value) {
return
}packages/canvas/container/src/container.js (1)
Line range hint 898-906: Add JSDoc comments for the new data source map methods.
While the implementation is correct, adding JSDoc comments would improve code documentation and maintainability.
BuiltinBundle,
+/**
+ * Sets the data source map in the renderer
+ * @param {...*} args - Arguments to pass to the renderer's setDataSourceMap
+ * @returns {*} Result from the renderer's setDataSourceMap
+ */
setDataSourceMap: (...args) => {
return canvasState.renderer.setDataSourceMap(...args)
},
+/**
+ * Gets the data source map from the renderer
+ * @param {...*} args - Arguments to pass to the renderer's getDataSourceMap
+ * @returns {*} Result from the renderer's getDataSourceMap
+ */
getDataSourceMap: (...args) => {
return canvasState.renderer.getDataSourceMap(...args)
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/canvas/container/src/CanvasContainer.vue (2 hunks)
- packages/canvas/container/src/container.js (2 hunks)
- packages/plugins/materials/src/composable/useMaterial.js (1 hunks)
- packages/renderer/package.json (1 hunks)
- packages/renderer/src/index.js (1 hunks)
- packages/renderer/vite.config.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/renderer/package.json
- packages/renderer/src/index.js
- packages/renderer/vite.config.js
🔇 Additional comments (2)
packages/plugins/materials/src/composable/useMaterial.js (1)
402-404: LGTM! Change aligns with renderer refactoring.
The modification to use BuiltinBundle from the canvas API aligns well with the PR's objective of extracting rendering logic into a standalone package. This change maintains the same functionality while improving modularity.
Let's verify that BuiltinBundle is properly exported from the renderer package:
✅ Verification successful
Let me gather more information about the BuiltinBundle implementation and its data structure:
Let me check the content of the builtin.json file and the container implementation:
Verified: BuiltinBundle integration is correctly implemented
The verification confirms that:
BuiltinBundleis properly exported from the renderer package (packages/renderer/src/index.js)- It's correctly imported in the canvas container (
packages/canvas/container/src/container.js) - The
builtin.jsoncontains the expected materials data structure with components and snippets - The usage in
useMaterial.jscorrectly accesses the materials viaBuiltinBundle.data.materials
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BuiltinBundle export from renderer package
# Expected: Find the export of BuiltinBundle in the renderer package
# Search for BuiltinBundle export
rg -g '!node_modules' -g '*.{js,ts,vue}' 'export.*BuiltinBundle'
# Search for BuiltinBundle usage to ensure consistency
rg -g '!node_modules' -g '*.{js,ts,vue}' 'BuiltinBundle.*materials'
Length of output: 299
Script:
#!/bin/bash
# Check BuiltinBundle definition and initialization
ast-grep --pattern 'const BuiltinBundle = $_'
# Look for BuiltinBundle class/object definition
rg -g '!node_modules' -g '*.{js,ts,vue}' -B 2 -A 2 'BuiltinBundle.*=.*{'
# Check if materials data structure is consistent
rg -g '!node_modules' -g '*.{js,ts,vue}' -B 2 'data:\s*{\s*materials'
Length of output: 409
Script:
#!/bin/bash
# Check BuiltinBundle definition with fixed regex
rg -g '!node_modules' -g '*.{js,ts,vue}' -B 2 -A 2 'BuiltinBundle = '
# Look for materials data structure
rg -g '!node_modules' -g '*.{js,ts,vue}' -B 2 'data: {'
# Check imports of BuiltinBundle
rg -g '!node_modules' -g '*.{js,ts,vue}' 'import.*BuiltinBundle'
Length of output: 462
Script:
#!/bin/bash
# Check builtin.json content
cat packages/renderer/src/builtin/builtin.json
# Check how container uses BuiltinBundle
rg -g '!node_modules' -g '*.{js,ts,vue}' -B 3 -A 3 'BuiltinBundle.*materials'
# Check container API exposure
ast-grep --pattern 'export const api = {
$$$
}'
Length of output: 17606
packages/canvas/container/src/container.js (1)
24-24: LGTM! Import statement updated to use renderer package.
The change aligns with the PR objective of extracting renderer functionality into a standalone package.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/renderer/README.zh-CN.md (2)
1-6: Enhance the introduction section.
The introduction section could be more comprehensive. Consider adding:
- Installation instructions
- Prerequisites/requirements
- Links to full API documentation
- Version compatibility information
1-28: Add missing documentation sections.
Consider adding the following important sections to make the documentation more complete:
- API Reference
- Detailed explanation of each API method
- Parameter types and requirements
- Return values and possible errors
- Schema Structure
- Documentation of the expected schema format
- Examples of valid schemas
- Error Handling
- Common error scenarios
- Troubleshooting guide
- Advanced Usage
- Custom configurations
- Performance optimization tips
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/renderer/README.md (1 hunks)
- packages/renderer/README.zh-CN.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/renderer/README.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/canvas/render/src/lowcode.js (1)
65-65: Consider making function name handling more robust.The current implementation relies on slicing the function name to handle extra characters added during block packaging. This approach might be fragile if the packaging behavior changes.
Consider implementing a more robust solution:
- else if (fnName && getCollectionMethodsMap()[fnName.slice(0, -1)]) { + else if (fnName) { + // Extract base function name using a regex pattern that matches known suffixes + const baseFnName = fnName.replace(/[0-9_]$/, ''); + if (getCollectionMethodsMap()[baseFnName]) {This change would:
- Make the function name handling more explicit
- Allow for different packaging suffixes
- Improve maintainability by centralizing the name transformation logic
packages/renderer/src/RenderMain.js (4)
402-419: Add JSDoc comments for propsWhile the props are well-structured with proper types and defaults, adding JSDoc comments would improve maintainability by documenting the purpose and expected format of each prop.
Example improvement:
props: { + /** Schema definition for the renderer + * @type {Object} + */ schema: { type: Object, default: () => ({}) }, // Add similar documentation for other props }
449-454: Consider optimizing props synchronizationThe watchEffect combines multiple state updates which could potentially cause unnecessary re-renders. Consider:
- Using individual watchers for better granularity
- Adding debounce/throttle for performance optimization
- Implementing proper cleanup in case of rapid prop changes
Example improvement:
- watchEffect(() => { - setUtils(props.utils) - setGlobalState(props.globalState) - setDataSourceMap(props.dataSource) - setSchema(props.schema) - }) + // Watch individual props for better control and performance + watch(() => props.utils, setUtils) + watch(() => props.globalState, setGlobalState) + watch(() => props.dataSource, setDataSourceMap) + watch(() => props.schema, setSchema)
492-499: Document the expanded API surfaceThe API has been expanded with new functionality, but lacks documentation. Consider:
- Adding JSDoc comments for each exported function
- Creating API reference documentation
- Including usage examples
This is particularly important as this is now a standalone package that other developers will need to understand and use.
371-371: Consider implementing a renderer registry patternThe current implementation uses a single
canvasRenderervariable. As this is now a standalone package, consider implementing a more flexible renderer registry pattern that would allow:
- Multiple renderer registrations
- Plugin-style renderer extensions
- Better testing isolation
This would make the package more extensible for future requirements while maintaining current functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/canvas/render/src/lowcode.js(3 hunks)packages/renderer/src/RenderMain.js(6 hunks)
🔇 Additional comments (4)
packages/canvas/render/src/lowcode.js (2)
53-56: LGTM! Clean refactoring of global object properties.
The changes maintain functionality while improving code clarity by consistently using the destructured API functions.
15-17: LGTM! Verify all imported functions are used.
The migration to the external renderer package aligns with the PR's objective of splitting the renderer into a standalone package.
Let's verify the usage of all imported functions:
✅ Verification successful
All imported functions are properly utilized in the codebase
The verification confirms that all destructured functions from the api import are actively used within the file:
getCollectionMethodsMap: Used in conditional logic for function name checksgenerateFn: Used for function generationglobalNotify: Used for notification handlinggetUtils: Used as a gettergetDataSourceMap: Used in dataSourceMap configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of imported functions from api
# Expected: Each function should be used at least once in the file
for func in "getCollectionMethodsMap" "generateFn" "globalNotify" "getUtils" "getDataSourceMap"; do
echo "Checking usage of $func:"
rg "\b$func\b" "packages/canvas/render/src/lowcode.js"
done
Length of output: 1861
packages/renderer/src/RenderMain.js (2)
19-29: LGTM: Well-structured imports for renderer functionality
The new imports are well-organized and clearly named, aligning with the PR objective of splitting the renderer into a standalone package.
40-42: LGTM: Canvas-related context functions
The new context functions for canvas flag management are appropriately placed with other context-related imports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/renderer/README.zh-CN.md (2)
1-6: Enhance documentation with more comprehensive information.The introduction section could be more detailed. Consider adding:
- Installation instructions
- Prerequisites/requirements
- Links to API documentation
- Version compatibility information
Example addition:
## 安装 ```bash npm install @opentiny/tiny-engine-renderer要求
- Vue 3.x
- Node.js >= 14
文档
详细的 API 文档请访问：API 文档
--- `1-28`: **Add examples for common use cases.** The README would benefit from additional examples showing: 1. How to handle dynamic schema updates 2. How to integrate with data sources 3. How to use utility functions 4. How to manage global state Would you like me to generate these additional examples? </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 948b74176f7d4154631eebadbb03c9b731ca7a75 and 3658f574a243c7697e563fcd1c0efe367b47b8ac. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `packages/renderer/README.md` (1 hunks) * `packages/renderer/README.zh-CN.md` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * packages/renderer/README.md </details> <details> <summary>🔇 Additional comments (1)</summary> <details> <summary>packages/renderer/README.zh-CN.md (1)</summary> `9-28`: _:warning: Potential issue_ **Multiple issues in the example code need attention.** 1. The example has inconsistent parameter naming (`dataSourceList` vs `dataSourceMap`) 2. Missing error handling for `schema.children` 3. Props should be properly typed 4. Not all props need to be reactive Apply this diff to improve the example: ```diff ```javascript // xxx.vue -import { h, reactive } from 'vue' -import Main from '@opentiny/tiny-engine-renderer' +import { h, defineComponent } from 'vue' +import Main, { type Schema, type Utils, type GlobalState, type DataSource } from '@opentiny/tiny-engine-renderer' -export default { +export default defineComponent({ render() { - // 页面schema - const schema = reactive({}) - // 工具类 - const utils = reactive({}) - // 全局状态 - const globalState = reactive([]) - // 数据源 - const dataSourceMap = reactive({}) + try { + // 页面schema + const schema: Schema = { + children: [] + // ... other schema properties + } + + // 工具类 + const utils: Utils = { + // ... utility methods + } + + // 全局状态 + const globalState: GlobalState = { + // ... global state + } + + // 数据源 + const dataSourceMap: Record<string, DataSource> = { + // ... data sources + } - return schema.children.length ? h(Main, { schema, utils, globalState, dataSourceList }) : null + if (!schema?.children) { + throw new Error('Invalid schema: missing children property') + } + + return schema.children.length + ? h(Main, { + schema, + utils, + globalState, + dataSourceMap // Fixed parameter name + }) + : null + } catch (error) { + console.error('Renderer initialization failed:', error) + return h('div', 'Failed to initialize renderer') + } } -} +})
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/canvas/package.json (1)
Line range hint
2-2: Document breaking changes in the version bump.The version bump to
2.0.0-alpha.4correctly follows semver for breaking changes. However, the removal of@opentiny/tiny-engine-builtin-componentand migration to the renderer package represents a breaking change that should be documented.Consider:
- Adding a CHANGELOG.md entry
- Updating migration guides
- Documenting the new renderer package's API
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/canvas/package.json(1 hunks)packages/renderer/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/renderer/package.json
yy-wow
removed
ready_for_review
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?
将以renderMain为入口的渲染逻辑抽取为独立包，作为schema渲染引擎可以独立使用
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
CanvasEmptyto guide users in building pages.CanvasContainer.vuefor improved mouse event handling within the canvas.tiny-engine-rendererto assist users with integration.@opentiny/tiny-engine-rendererto streamline development.Bug Fixes
Documentation
.gitignorefile to enhance code quality and streamline version control.tiny-engine-renderer.Chores
package.jsonfor the@opentiny/tiny-engine-rendererpackage, setting up scripts and dependencies.