feat: customized canvas entry #850
Conversation
WalkthroughThe changes in this pull request introduce several enhancements across multiple files in the canvas package. Key updates include the addition of a new alias mapping for the Changes
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (13)
packages/canvas/render/src/context.js (1)
55-55: LGTM with a minor suggestion for robustness.The
getConditionfunction is a good addition that complements the existingsetConditionfunction. It provides a way to retrieve the visibility state of a condition, which aligns well with the module's purpose.Consider modifying the implementation to explicitly check for
falseto avoid potential issues with other falsy values:-export const getCondition = (id) => conditions[id] !== false +export const getCondition = (id) => conditions[id] !== false && conditions[id] !== undefinedThis change ensures that only explicitly set
falsevalues or undefined conditions (which haven't been set) returnfalse, providing more predictable behavior.packages/canvas/render/src/runner.js (4)
52-52: LGTM! Consider adding a comment for clarity.The changes to the
createfunction look good. Passing therendererobject tobeforeAppCreateinstead of a Vue object withhproperty simplifies the API and provides a more consistent interface.Consider adding a brief comment explaining the purpose of the
rendererobject for better code readability:- await beforeAppCreate({ api: renderer }) + // Pass renderer object containing API and app state to beforeAppCreate hook + await beforeAppCreate({ api: renderer })
Line range hint
72-72: Consider improving error handlingThe current error handler suppresses all errors, which might hide important issues:
App.config.errorHandler = () => {}Consider implementing a more robust error handling strategy. For example:
App.config.errorHandler = (err, instance, info) => { console.error('Vue Error:', err, instance, info) // Optionally, you can dispatch an event to notify the parent about the error dispatch('canvasError', { detail: { error: err, info } }) }
Line range hint
68-68: Consider alternatives to global propertiesSetting
lowcodeConfigas a global property might make it harder to track state changes:App.config.globalProperties.lowcodeConfig = window.parent.TinyGlobalConfigConsider using Vue's provide/inject API or a state management solution like Pinia for better maintainability.
Example using provide/inject:
// In your main component import { provide } from 'vue' provide('lowcodeConfig', window.parent.TinyGlobalConfig) // In child components import { inject } from 'vue' const lowcodeConfig = inject('lowcodeConfig')
Line range hint
74-76: Potential memory leak in ResizeObserverThe ResizeObserver is created but never disconnected, which could lead to memory leaks if the component is frequently unmounted and remounted.
Consider storing the observer instance and disconnecting it when appropriate:
let resizeObserver const create = async (config) => { // ... existing code ... resizeObserver = new ResizeObserver(() => { dispatch('canvasResize') }) resizeObserver.observe(document.body) // Add a cleanup function const cleanup = () => { if (resizeObserver) { resizeObserver.disconnect() resizeObserver = null } } // Call cleanup when appropriate, e.g., when unmounting the app App.unmount = () => { cleanup() createApp(App).unmount() } // ... rest of the function ... }packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
58-64: LGTM: New canvas source handling logic implementedThe new logic for handling canvas sources is well-implemented. It allows for a custom canvas source (
canvasSrc) or falls back to generating one (canvasSrcDoc), which aligns with the PR objectives. The use ofgetOptions(meta.id)provides component-specific configuration, and the fallback mechanism ensures backwards compatibility.Consider adding a brief comment explaining the purpose of this logic for better code readability.
const { canvasSrc = '' } = getOptions(meta.id) || {} let canvasSrcDoc = '' +// Use custom canvas source if provided, otherwise generate one if (!canvasSrc) { const { importMap, importStyles } = getImportMapData(getMergeMeta('engine.config')?.importMapVersion) canvasSrcDoc = initCanvas(importMap, importStyles) }packages/canvas/container/src/CanvasContainer.vue (2)
14-21: LGTM! Improved iframe rendering and source handling.The changes enhance the flexibility of the iframe's source handling while maintaining the existing loading state display logic. The use of a computed property for determining the attribute name is a clean approach.
Consider adding an
aria-labelto the iframe for better accessibility:<iframe id="canvas" ref="iframe" :[srcAttrName]="canvasSrc || canvasSrcDoc" style="border: none; width: 100%; height: 100%" + aria-label="Canvas content" ></iframe>
67-67: LGTM! New prop added for alternative iframe source.The addition of the
canvasSrcDocprop aligns well with the template changes, providing more flexibility in specifying the iframe's content.Consider adding a brief comment to document the purpose of this prop:
props: { controller: Object, canvasSrc: String, + // Alternative source for iframe content when canvasSrc is not provided canvasSrcDoc: String, materialsPanel: Object },packages/canvas/render/src/RenderMain.js (2)
446-447: LGTM: New methods added to API object.The
getConditionandgetConditionsmethods have been correctly added to theapiobject, expanding its functionality. This change aligns with the new imports and enhances the API surface for managing canvas conditions.Consider adding JSDoc comments for these new methods to improve API documentation. For example:
/** * Retrieves a specific condition from the canvas state. * @returns {any} The requested condition value. */ getCondition, /** * Retrieves all conditions from the canvas state. * @returns {Object} An object containing all condition key-value pairs. */ getConditions,
27-28: Overall impact: Enhanced condition management capabilities.The changes introduce new methods for managing conditions in the canvas rendering system. These additions are non-breaking and enhance the API's functionality without modifying existing code.
Consider the following suggestions to further improve the implementation:
- Ensure that proper error handling and type checking are implemented in the
getConditionandgetConditionsmethods (in the './context' module) to maintain robustness.- If these methods are part of a public API, consider adding them to the API documentation or README file to inform users of the new capabilities.
- Consider adding unit tests for these new methods to ensure their correct functionality and integration with the existing system.
Also applies to: 446-447
packages/toolbars/preview/src/Main.vue (1)
Line range hint
34-89: Consider Extracting Repetitive Logic into a Helper FunctionThe code pattern for checking if a function exists, calling it, and handling errors is repeated for
beforePreview,previewMethod, andafterPreview. Consider extracting this pattern into a helper function to reduce duplication and improve readability.Example of extracting the logic:
function executeHook(hookFunction, errorMessage) { if (typeof hookFunction === 'function') { return hookFunction().catch(error => { console.log(errorMessage, error) useNotify({ type: 'error', message: errorMessage }) }) } } const preview = async () => { const { beforePreview, previewMethod, afterPreview } = getOptions(meta.id) await executeHook(beforePreview, 'Error in beforePreview:') if (await executeHook(previewMethod, 'Error in previewMethod:')) { return } // existing preview logic... await executeHook(afterPreview, 'Error in afterPreview:') }This refactoring reduces code repetition and centralizes error handling for hook functions.
packages/toolbars/save/src/Main.vue (1)
Line range hint
105-121: Refactor auto-save functionality usingsetIntervalThe current implementation uses a recursive
setTimeoutinsaveSetTimeout, which can lead to potential issues like stack overflows or unintended multiple timeouts. UsingsetIntervalsimplifies the code and ensures consistent execution at specified intervals.Apply this diff to refactor the auto-save functionality:
-const saveSetTimeout = () => { - clearTimeout(state.preservationTime) - state.preservationTime = setTimeout(() => { - openApi() - saveSetTimeout() - }, state.timeValue * 60 * 1000) -} - -const autoSave = () => { - if (state.checked) { - saveSetTimeout() - } else { - clearTimeout(state.preservationTime) - } - state.saveVisible = false -} +let autoSaveInterval = null + +const startAutoSave = () => { + stopAutoSave() + autoSaveInterval = setInterval(() => { + openApi() + }, state.timeValue * 60 * 1000) +} + +const stopAutoSave = () => { + clearInterval(autoSaveInterval) + autoSaveInterval = null +} + +const autoSave = () => { + if (state.checked) { + startAutoSave() + } else { + stopAutoSave() + } + state.saveVisible = false +} + +onUnmounted(() => { + stopAutoSave() +})packages/settings/events/src/components/BindEventsDialog.vue (1)
Line range hint
175-210: Add error handling forbeforeSaveMethodto prevent unhandled rejections.Since
confirmis now anasyncfunction that awaitsbeforeSaveMethod, any errors thrown bybeforeSaveMethodcould result in unhandled promise rejections. To enhance stability and user experience, consider adding error handling around this call.You can wrap the call in a try-catch block:
if (typeof beforeSaveMethod === 'function') { + try { await beforeSaveMethod(method, state.bindMethodInfo) + } catch (error) { + // Handle the error appropriately + console.error('Error in beforeSaveMethod:', error) + // Optionally notify the user about the error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- packages/canvas/DesignCanvas/src/DesignCanvas.vue (4 hunks)
- packages/canvas/container/src/CanvasContainer.vue (4 hunks)
- packages/canvas/index.js (1 hunks)
- packages/canvas/render/src/RenderMain.js (2 hunks)
- packages/canvas/render/src/context.js (1 hunks)
- packages/canvas/render/src/runner.js (2 hunks)
- packages/settings/events/src/components/BindEventsDialog.vue (4 hunks)
- packages/toolbars/preview/src/Main.vue (3 hunks)
- packages/toolbars/save/src/Main.vue (1 hunks)
- packages/toolbars/save/src/js/index.js (3 hunks)
🧰 Additional context used
🔇 Additional comments (16)
packages/canvas/render/src/context.js (2)
54-57: Overall, good additions to enhance condition management.The new functions
getConditionandgetConditionscomplement the existingsetConditionfunction and provide useful ways to access condition states. They align well with the module's purpose of managing context and conditions for canvas rendering.A few minor improvements were suggested to enhance robustness and data protection. Additionally, more context about the use case for
getConditionswould be helpful in determining the best implementation approach.
57-57: Approved, but consider enhancing data protection and providing more context.The
getConditionsfunction provides a way to access all condition states, which can be useful for debugging or bulk operations.Consider the following suggestions:
Return a copy of the conditions object to prevent unintended modifications:
-export const getConditions = () => conditions +export const getConditions = () => ({ ...conditions })Add a comment explaining the intended use case for this function.
If bulk access is not necessary, consider removing this function to maintain better encapsulation.
To help determine the best course of action, let's check if this function is used elsewhere in the codebase:
Could you provide more context on why direct access to all conditions is necessary? This information would help in determining whether to keep this function or if there's a more specific approach we could implement.
packages/canvas/render/src/runner.js (1)
Line range hint
1-91: Overall assessment: Good changes with room for improvementThe modifications to this file, particularly the changes to the
createfunction, are generally positive. The main improvement is passing therendererobject tobeforeAppCreate, which simplifies the API and provides a more consistent interface.However, there are a few areas where the code could be further improved:
- Error handling could be more robust.
- Consider alternatives to using global properties for better state management.
- Address the potential memory leak with the ResizeObserver.
Addressing these points would further enhance the code's quality and maintainability.
packages/canvas/DesignCanvas/src/DesignCanvas.vue (5)
8-9: LGTM: New props align with PR objectivesThe addition of
canvas-srcandcanvas-src-docprops to the CanvasContainer component aligns well with the PR objective of allowing configuration of thesrcattribute. This change provides more flexibility in handling canvas sources.
30-31: LGTM: New imports support enhanced functionalityThe addition of
getMergeMetaandgetOptionsimports is consistent with the new functionality introduced in the component. These utilities likely support the new canvas source handling mechanism.
38-38: LGTM: Meta import addedThe addition of the
metaimport is appropriate, as it's used later in the code to retrieve options specific to this component.
173-174: LGTM: New canvas source variables exposed to templateThe addition of
canvasSrcandcanvasSrcDocto the returned object correctly exposes these new variables to the template. This change is consistent with the earlier modifications in the template and setup function.
Line range hint
1-194: Summary: Successfully implemented custom canvas source supportThe changes in this file successfully implement support for custom canvas sources, aligning well with the PR objectives. The new
canvasSrcandcanvasSrcDocproperties provide flexibility in handling canvas sources while maintaining backwards compatibility. The code quality is good, with only a minor suggestion for improved readability.Overall, these changes enhance the functionality of the DesignCanvas component without introducing breaking changes.
packages/canvas/container/src/CanvasContainer.vue (2)
78-78: LGTM! Computed property for dynamic attribute selection.The
srcAttrNamecomputed property is a clean and efficient way to determine the correct attribute for the iframe based on the available source. This approach enhances the component's flexibility and maintainability.
247-248: LGTM! Exposing new properties to the template.The addition of
loadingandsrcAttrNameto the returned object in the setup function correctly makes these properties available for use in the template. This change is consistent with the Vue 3 Composition API best practices.packages/canvas/render/src/RenderMain.js (1)
27-28: LGTM: New imports added correctly.The new imports for
getConditionandgetConditionsare appropriately placed with other related imports from the './context' module. This change aligns with the newly added methods in theapiobject.packages/toolbars/preview/src/Main.vue (1)
39-49: Check for Early Return Condition inpreviewMethodWhen
previewMethodreturns a truthystopvalue, the function exits early. Ensure that this behavior is intentional and properly documented, so other developers understand the control flow.Please confirm that returning
stopfrompreviewMethodis the desired behavior for halting the preview process.packages/toolbars/save/src/js/index.js (1)
163-169: Review error handling in thesavedhookWhen invoking the
savedhook, any errors are caught and logged, but no further action is taken. If thesavedhook performs critical operations, consider implementing additional error handling, such as notifying the user or retrying the operation, to ensure the application responds appropriately to failures.packages/settings/events/src/components/BindEventsDialog.vue (3)
35-35: ImportinggetOptionsis appropriate and necessary.The addition of
getOptionsimport is necessary for retrieving options likebeforeSaveMethod. This enhances the flexibility and configurability of the component.
197-203: Ensure consistent inclusion ofeventArgsin function parameters.Currently,
eventArgsis included in the function parameters only whenstate.enableExtraParamsistrue. Verify whethereventArgsshould always be included to ensure that event data is consistently available within the method. Inconsistencies might lead to unexpected behaviors or bugs.Run the following script to analyze how
eventArgsis used in function definitions:#!/bin/bash # Description: Find function definitions that include 'eventArgs' in their parameters. # Search for functions where 'eventArgs' is a parameter rg --type js --pcre2 'function\s+\w+\s*\(\s*eventArgs' -A 2Expected Result: The script will list all function definitions that use
eventArgsas a parameter, helping determine if it should be included consistently.
16-16: Verify necessity of passingdialogVisibleprop toBindEventsDialogSidebar.Please check if the
dialogVisibleprop is utilized within theBindEventsDialogSidebarcomponent. If it's not used, consider removing it to simplify the component interface and avoid passing unnecessary props.Run the following script to verify if
dialogVisibleis used withinBindEventsDialogSidebar:Expected Result: The script will output lines from
BindEventsDialogSidebar.vuewheredialogVisibleis defined inprops. If no output is returned, it indicates thatdialogVisibleis not used in the component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/toolbars/save/src/js/index.js (2)
100-119: LGTM: New hooks implemented with improved error handlingThe implementation of
beforeSave,saveMethod, andsavedhooks aligns well with the PR objectives. The addition of error handling is a good practice.Consider adding more specific error messages for different types of errors to provide better context to the user.
You could enhance the error handling by providing more specific error messages:
} catch (error) { let errorMessage = 'An unexpected error occurred during saving.'; if (error instanceof TypeError) { errorMessage = 'Invalid data type encountered during saving.'; } else if (error instanceof NetworkError) { errorMessage = 'Network error occurred while saving. Please check your connection.'; } useNotify({ type: 'error', message: `Error in saving: ${errorMessage}` }); }
164-174: LGTM: Improved error handling and hook implementationThe use of a
finallyblock to resetstate.disabledand the implementation of thesavedhook with error handling are good practices.Consider adding a more descriptive error message and potentially logging the error for debugging purposes.
You could enhance the error handling for the
savedhook:} catch (error) { console.error('Error in saved hook:', error); useNotify({ type: 'error', message: `An error occurred after saving. Some post-save operations may not have completed successfully. Error: ${error.message}` }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/canvas/index.js (1 hunks)
- packages/toolbars/preview/src/Main.vue (3 hunks)
- packages/toolbars/save/src/js/index.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/canvas/index.js
- packages/toolbars/preview/src/Main.vue
🧰 Additional context used
🔇 Additional comments (2)
packages/toolbars/save/src/js/index.js (2)
20-20: LGTM: Import statement updated correctlyThe addition of
getOptionsto the import statement is in line with the PR objectives to introduce new hooks for the save functionality.
Line range hint
1-174: LGTM: Overall implementation aligns with PR objectivesThe changes in this file successfully implement the new hooks for the save functionality as described in the PR objectives. The error handling has been improved, and the overall structure of the code remains clean and maintainable.
Great job on enhancing the save feature with these new hooks and improved error handling!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- package.json (1 hunks)
- packages/toolbars/preview/src/Main.vue (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/toolbars/preview/src/Main.vue
🧰 Additional context used
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/canvas/package.json (1)
17-24: LGTM! Consider adding a CommonJS export condition.The addition of the
exportsfield is a great improvement. It provides explicit control over how the package can be imported, which enhances compatibility with various module resolution systems.To further improve compatibility, consider adding a CommonJS export condition. This would allow the package to be used in both ES Module and CommonJS environments. Here's a suggested modification:
"exports": { ".": { - "import": "./dist/index.js" + "import": "./dist/index.js", + "require": "./dist/index.cjs" }, "./render": { - "import": "./dist/render.js" + "import": "./dist/render.js", + "require": "./dist/render.cjs" } },Note: This suggestion assumes that CommonJS versions of the files (with
.cjsextension) are or will be available in thedistdirectory. If not, you'll need to adjust your build process to generate these files.packages/canvas/vite.config.js (1)
37-38: LGTM! Consider updating documentation.The addition of the
renderentry point is a good improvement, allowing for more granular imports and potentially smaller bundle sizes. This change aligns well with the PR objectives of introducing new canvas functionality.Consider updating the package documentation to reflect this new entry point, explaining its purpose and how it relates to the new hooks for save and preview functionalities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/build/vite-config/src/vite-plugins/devAliasPlugin.js (1 hunks)
- packages/canvas/package.json (1 hunks)
- packages/canvas/render/src/context.js (1 hunks)
- packages/canvas/vite.config.js (1 hunks)
- packages/design-core/src/canvas/canvas.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/design-core/src/canvas/canvas.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/canvas/render/src/context.js
🧰 Additional context used
🔇 Additional comments (2)
packages/build/vite-config/src/vite-plugins/devAliasPlugin.js (2)
Line range hint
1-118: Overall assessment of changes in devAliasPlugin.jsThe changes made to this file are minimal and focused. The addition of the new alias mapping for
@opentiny/tiny-engine-canvas/renderis consistent with the existing pattern and doesn't introduce any apparent issues. The rest of the file, including thegetDevAliasfunction structure and thedevAliasPluginfunction, remains unchanged.To ensure the change is fully integrated:
- Verify that the new alias is being used correctly in other parts of the codebase (as suggested in the previous comment).
- Consider updating any relevant documentation to reflect this new alias, if necessary.
- Ensure that the build process correctly resolves this new alias in both development and production environments.
58-58: New alias mapping added for@opentiny/tiny-engine-canvas/renderThe new alias mapping has been correctly added to the
getDevAliasfunction. This addition allows for easier imports of the render module from the canvas package during development.To ensure consistency and proper usage, let's verify if this new alias is being used correctly in other parts of the codebase:
This will help us identify any places where the new alias is being used and ensure it's implemented correctly across the project.
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
Release Notes
New Features
createRenderfunction to facilitate rendering operations.renderentry.Improvements
Bug Fixes
Configuration Changes
"type": "module"property inpackage.jsonfor modern JavaScript standards.package.json.