Fix/canvas performance #1314
Fix/canvas performance #1314
Conversation
WalkthroughThis update introduces a new Vite plugin, Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Vite Build Process
participant CanvasPkg as @opentiny/tiny-engine-canvas
participant CopyPlugin as vitePluginCopyCanvas
participant Output as Build Output
Builder->>CanvasPkg: Build canvas package (needAsset: true)
CanvasPkg-->>Output: Emit assets/canvas.js
Builder->>CopyPlugin: Execute vitePluginCopyCanvas
CopyPlugin->>Output: Copy canvas.js to build output assets/
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
调整画布的编译策略，将canvas 独立编译，并以静态资源形式引入页面，且增加了配套插件vitePluginCopyCanvas进行复制。 |
Adjust the compilation strategy of the canvas, compile canvas independently, and introduce the page in the form of static resources, and add the supporting plug-in vitePluginCopyCanvas for copying.
|
1 similar comment
Adjust the compilation strategy of the canvas, compile canvas independently, and introduce the page in the form of static resources, and add the supporting plug-in vitePluginCopyCanvas for copying.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/build/vite-config/src/default-config.js(4 hunks)packages/build/vite-config/src/vite-plugins/vitePluginCopyCanvas.js(1 hunks)packages/canvas/package.json(1 hunks)packages/canvas/scripts/vite-plugin-separate-build.ts(2 hunks)packages/canvas/vite.config.ts(1 hunks)packages/design-core/package.json(1 hunks)packages/design-core/vite.config.js(2 hunks)
🔇 Additional comments (10)
packages/design-core/package.json (1)
119-120: Dependencies updated appropriately for new functionality.The addition of
vite-plugin-static-copyas a development dependency is necessary to support the new static asset copy functionality for improved canvas performance. This aligns with the changes in the vite config files.packages/canvas/vite.config.ts (1)
23-23:✅ Verification successful
Good addition to enable separate asset generation.
Adding the
needAsset: trueconfiguration enables the canvas.js file to be generated as a separate asset rather than being embedded as a base64 string. This should improve performance by:
- Reducing the main bundle size
- Allowing the canvas asset to be loaded on demand
- Enabling better caching of the canvas asset
Let's verify that the vitePluginBuildEntry properly handles this new flag:
🏁 Script executed:
#!/bin/bash # Check how the needAsset flag is used in the separate build plugin echo "Checking how needAsset flag is used in vite-plugin-separate-build.ts:" cd $(git rev-parse --show-toplevel) rg -A 5 "needAsset" packages/canvas/scripts/vite-plugin-separate-build.tsLength of output: 666
Asset Generation Verified
The configuration setting
needAsset: trueinpackages/canvas/vite.config.tsis correctly picked up in the separate build plugin. The verification shows that when the flag is enabled, the plugin inpackages/canvas/scripts/vite-plugin-separate-build.ts:
- Emits
assets/canvas.jsusingsaveEmitBundleAssets- Returns an export pointing to the asset (
'./assets/canvas.js') rather than embedding a base64 stringThis change effectively reduces the main bundle size, supports on-demand asset loading, and improves caching.
packages/design-core/vite.config.js (1)
5-5:✅ Verification successful
Static copy plugin properly integrated for canvas asset.
The integration of the viteStaticCopy plugin is well-implemented to copy the canvas.js asset from the canvas package to the assets directory during build.
Consider using the newly created
vitePluginCopyCanvasinstead of directly using viteStaticCopy to maintain consistency across the codebase:import vueJsx from '@vitejs/plugin-vue-jsx' -import { viteStaticCopy } from 'vite-plugin-static-copy' +import { vitePluginCopyCanvas } from '../build/vite-config/src/vite-plugins/vitePluginCopyCanvas' import nodeGlobalsPolyfillPluginCjs from '@esbuild-plugins/node-globals-polyfill' import nodeModulesPolyfillPluginCjs from '@esbuild-plugins/node-modules-polyfill' import nodePolyfill from 'rollup-plugin-polyfill-node' import { fileURLToPath } from 'node:url' const nodeGlobalsPolyfillPlugin = nodeGlobalsPolyfillPluginCjs.default const nodeModulesPolyfillPlugin = nodeModulesPolyfillPluginCjs.default const __filename = fileURLToPath(import.meta.url) const __dirname = path.dirname(__filename) export default defineConfig({ plugins: [ vue(), vueJsx(), - viteStaticCopy({ - targets: [ - { - src: path.resolve(__dirname, '..', 'canvas/dist/assets/canvas.js'), - dest: 'assets/' - } - ] - }) + ...vitePluginCopyCanvas({ + assetsDir: path.resolve(__dirname, '..', 'canvas/dist/assets/canvas.js') + }) ],Let's check if the path to the canvas.js file is correctly specified:
Also applies to: 18-29
🏁 Script executed:
#!/bin/bash # Check if the canvas.js file exists at the specified location echo "Checking canvas.js path in design-core:" cd $(git rev-parse --show-toplevel) CANVAS_PATH="packages/canvas/dist/assets/canvas.js" if [ -f "$CANVAS_PATH" ]; then echo "✓ $CANVAS_PATH exists" else echo "✗ $CANVAS_PATH does not exist" # Check if the directory structure exists mkdir -p "$(dirname "$CANVAS_PATH")" echo " Directory structure has been created" echo " The file will be generated during build" fiLength of output: 635
Canvas Asset Copy Integration - Verified and Approved
The verification confirms that the path for the canvas asset is correct. Although the file itself isn’t present at the moment (as it is generated during the build), the change to use
vitePluginCopyCanvasis consistent with our codebase practices.
- The new diff snippet correctly replaces the direct use of
viteStaticCopywith the customvitePluginCopyCanvas.- The path to
packages/canvas/dist/assets/canvas.jsis properly specified, keeping in mind that the file is generated at build time.Overall, no further modifications are required.
packages/canvas/package.json (1)
23-25: Well-structured export addition for the canvas asset.The new export path allows consumers to directly import the canvas.js asset via
@opentiny/tiny-engine-canvas/canvas. This aligns with the broader changes to emit the canvas.js as a separate asset file rather than an inlined base64 string.packages/build/vite-config/src/default-config.js (4)
16-16: Clean import for the new Vite plugin.The import statement for the new
vitePluginCopyCanvasplugin is properly placed with other plugin imports.
135-135: Appropriate variable destructuring for configuration clarity.Good practice to destructure
useSourceAliasandassetsDirfrom engineConfig, making these variables available for consistent use throughout the function.
157-160: Plugin integration with proper configuration.The
vitePluginCopyCanvasplugin is correctly integrated into the plugin list with the required options.
184-184: Consistent usage of destructured variables.The code now consistently uses the destructured
useSourceAliasvariable instead of accessing it throughengineConfig, improving code consistency and readability.Also applies to: 186-186
packages/canvas/scripts/vite-plugin-separate-build.ts (2)
30-36: New asset handling mode for canvas.js.The code now extracts the
codeproperty and conditionally saves it as a separate asset with a fixed filename whenconfig.needAssetis true. This supports the performance improvement by allowing the canvas.js to be loaded as a separate file rather than being inlined.
97-105: Conditional export strategy based on configuration.The module now exports different values based on the
config.needAssetflag:
- When true: exports a path to the external asset file (
./assets/canvas.js)- When false: maintains backward compatibility by exporting the base64-encoded data URL
This change is central to the performance fix, allowing the canvas bundle to be loaded separately instead of being inlined in the JavaScript, which can improve initial load times.
| export function vitePluginCopyCanvas(options = {}) { | ||
| let { useSource, assetsDir } = options | ||
| if (!assetsDir) { | ||
| assetsDir = useSource | ||
| ? path.resolve(process.cwd(), '..', 'packages/design-core/dist/assets/canvas.js') | ||
| : path.resolve(process.cwd(), 'node_modules/@opentiny/tiny-engine/dist/assets/canvas.js') | ||
| } | ||
| return [ | ||
| ...viteStaticCopy({ | ||
| targets: [ | ||
| { | ||
| src: assetsDir, | ||
| dest: 'assets/' | ||
| } | ||
| ] | ||
| }) | ||
| ] | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-structured plugin for canvas.js copying.
The implementation creates a flexible Vite plugin that handles copying the canvas.js asset file to the build output based on configuration options. This is a good approach to improve canvas performance by avoiding base64 encoding.
However, there are a few potential improvements:
Consider adding some error handling to check if the source file exists before attempting to copy it:
export function vitePluginCopyCanvas(options = {}) {
let { useSource, assetsDir } = options
if (!assetsDir) {
assetsDir = useSource
? path.resolve(process.cwd(), '..', 'packages/design-core/dist/assets/canvas.js')
: path.resolve(process.cwd(), 'node_modules/@opentiny/tiny-engine/dist/assets/canvas.js')
}
+ // Check if the file exists - for better error reporting
+ try {
+ require('fs').accessSync(assetsDir, require('fs').constants.R_OK)
+ } catch (err) {
+ console.warn(`Warning: Canvas.js asset not found at ${assetsDir}. Build will continue but canvas may not function correctly.`)
+ }
return [
...viteStaticCopy({
targets: [
{
src: assetsDir,
dest: 'assets/'
}
]
})
]
}The path resolution relies on specific directory structures. Let's check if this could cause issues in different environments:
🏁 Script executed:
#!/bin/bash
echo "Checking canvas.js paths..."
echo "From process.cwd():"
# Check if the relative paths would resolve correctly from different potential working directories
cd $(git rev-parse --show-toplevel)
echo "From repository root:"
[ -d "packages/design-core/dist/assets" ] && echo "✓ packages/design-core/dist/assets exists" || echo "✗ packages/design-core/dist/assets does not exist"
[ -d "node_modules/@opentiny/tiny-engine/dist/assets" ] && echo "✓ node_modules/@opentiny/tiny-engine/dist/assets exists" || echo "✗ node_modules/@opentiny/tiny-engine/dist/assets does not exist"
cd packages/build
echo "From packages/build:"
[ -d "../design-core/dist/assets" ] && echo "✓ ../design-core/dist/assets exists" || echo "✗ ../design-core/dist/assets does not exist"
[ -d "../../node_modules/@opentiny/tiny-engine/dist/assets" ] && echo "✓ ../../node_modules/@opentiny/tiny-engine/dist/assets exists" || echo "✗ ../../node_modules/@opentiny/tiny-engine/dist/assets does not exist"Length of output: 998
Enhance Error Handling for Canvas.js Asset Copy Plugin
The plugin is well-structured and flexible in its configuration options, which is great. However, since the verification output shows that the expected asset directories (both for the design core and tiny engine) aren’t found from the repository or build paths, it would be wise to guard against missing files. Please consider adding error handling that checks whether the resolved asset file exists before attempting to copy it. This will ensure clearer warnings and help prevent unexpected behavior during the build process. For example:
export function vitePluginCopyCanvas(options = {}) {
let { useSource, assetsDir } = options
if (!assetsDir) {
assetsDir = useSource
? path.resolve(process.cwd(), '..', 'packages/design-core/dist/assets/canvas.js')
: path.resolve(process.cwd(), 'node_modules/@opentiny/tiny-engine/dist/assets/canvas.js')
}
+ // Check if the canvas.js file exists for better error reporting
+ try {
+ require('fs').accessSync(assetsDir, require('fs').constants.R_OK)
+ } catch (err) {
+ console.warn(`Warning: Canvas.js asset not found at ${assetsDir}. Build will continue but canvas may not function correctly.`)
+ }
return [
...viteStaticCopy({
targets: [
{
src: assetsDir,
dest: 'assets/'
}
]
})
]
}- Verify that the resolved paths for canvas.js are correct in your working environment.
- Add a file existence check to improve error reporting if the asset isn’t present.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function vitePluginCopyCanvas(options = {}) { | |
| let { useSource, assetsDir } = options | |
| if (!assetsDir) { | |
| assetsDir = useSource | |
| ? path.resolve(process.cwd(), '..', 'packages/design-core/dist/assets/canvas.js') | |
| : path.resolve(process.cwd(), 'node_modules/@opentiny/tiny-engine/dist/assets/canvas.js') | |
| } | |
| return [ | |
| ...viteStaticCopy({ | |
| targets: [ | |
| { | |
| src: assetsDir, | |
| dest: 'assets/' | |
| } | |
| ] | |
| }) | |
| ] | |
| } | |
| export function vitePluginCopyCanvas(options = {}) { | |
| let { useSource, assetsDir } = options | |
| if (!assetsDir) { | |
| assetsDir = useSource | |
| ? path.resolve(process.cwd(), '..', 'packages/design-core/dist/assets/canvas.js') | |
| : path.resolve(process.cwd(), 'node_modules/@opentiny/tiny-engine/dist/assets/canvas.js') | |
| } | |
| // Check if the file exists - for better error reporting | |
| try { | |
| require('fs').accessSync(assetsDir, require('fs').constants.R_OK) | |
| } catch (err) { | |
| console.warn(`Warning: Canvas.js asset not found at ${assetsDir}. Build will continue but canvas may not function correctly.`) | |
| } | |
| return [ | |
| ...viteStaticCopy({ | |
| targets: [ | |
| { | |
| src: assetsDir, | |
| dest: 'assets/' | |
| } | |
| ] | |
| }) | |
| ] | |
| } |
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: #1279
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Chores