fix: localImportMap not works #1242
Conversation
WalkthroughThis update introduces a comprehensive local CDN solution across the project. It adds new environment variables, configuration files, and documentation to support localizing CDN dependencies for offline or intranet environments. The core build logic is refactored to use a dynamic Vite plugin for managing local CDN resources, replacing static import map copying. Several test files are added to verify the local CDN functionality, including dependency copying and environment variable handling. Supporting utilities and configuration enhancements are included, such as new helper functions, updated import map handling, and the integration of a static copy plugin. Documentation is expanded to describe the local CDN approach and its configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Vite as Vite Build System
participant LocalCDN as localCdnPlugin
participant FileSys as File System
Dev->>Vite: Run build with local CDN env vars enabled
Vite->>LocalCDN: Initialize localCdnPlugin with config
LocalCDN->>FileSys: Check/install missing packages
LocalCDN->>FileSys: Copy/copy+transform CDN files to local-cdn-static
LocalCDN-->>Vite: Register plugin for build
Vite->>FileSys: Generate build output (dist)
FileSys-->>Dev: Output includes local-cdn-static and updated assets
Poem
✨ Finishing Touches
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 (
|
chilingling
force-pushed
the
feat/localCDNPlugin
branch
from
76b33be to
1ef77f0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
designer-demo/vitest.config.js (1)
1-19: Well-structured Vitest configuration.The configuration correctly sets up the testing environment with Node and globals enabled. The extended timeouts (10 minutes) are appropriate for builds that might take longer to complete.
However, disabling file parallelism will significantly increase overall test execution time. Consider:
- Adding a comment explaining the total expected test execution time
- If possible, restructuring tests to be more isolated so they could run in parallel
packages/common/vite.config.ts (1)
28-41: Well-structured plugin configuration with clear purpose.The plugin configuration is now more readable with one plugin per line. The viteStaticCopy plugin is correctly set up to copy the import-map.json file to the output directory, and the comment clearly explains its purpose.
Consider specifying a subdirectory for the copied file rather than the root directory (
.) to avoid potential naming conflicts:viteStaticCopy({ targets: [ { src: './js/importMap/import-map.json', - dest: '.' + dest: './importMap' } ] })packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (5)
1-9: Consider using a dedicated logger.
While usingconsoleis acceptable, a dedicated logger can provide structured logging and log levels (info, warn, error, debug) for more robust debugging and production visibility.
31-37: Handle potential Babel transform failures.
When callingbabelReplaceImportPathWithCertainFileName, consider catching or reporting errors if the transformation fails, to avoid quietly returning original content.
48-60: Ensure comprehensive error handling.
TheextractInfofunction catches an error and logs it, but consider re-throwing or returning a fallback to prevent silent failures elsewhere in the pipeline.
251-315: Check for missing fallback scenarios.
This function relies heavily on file existence checks and user configurations. Consider handling rare corner cases, such as partially installed packages or mismatched file paths, to improve robustness.
318-386: Overall plugin logic is comprehensive.
Combining default and user-defined import maps is a thoughtful approach. For readability, you might split the logic into smaller helper functions if it grows more complex.designer-demo/tests/localCdnBundleDeps.test.js (1)
76-81: Build command may rely on pnpm availability.
UsingexecSync('pnpm run build:alpha')can fail if pnpm isn’t installed. Consider documenting or verifying that pnpm is installed in CI or development environments.packages/build/vite-config/src/localCdnFile/copyBundleDeps.js (1)
33-44: Bundle link replacement logic is clean.
Iterating overpackagesand matching URLs works well. Consider logging or warning if a link is not matched, to detect unhandled URLs.packages/canvas/DesignCanvas/src/importMap.ts (2)
26-37: Address TODO comments in the code.There are TODO comments indicating that certain parts of the code should be deleted, but they are still present. These should either be removed or addressed with a plan for future cleanup.
Consider creating a follow-up issue to address these TODOs or remove them if they are no longer relevant:
export function getImportMapData(canvasDeps = { scripts: [], styles: [] }) { // 以下内容由于区块WebComponent加载需要补充 const blockRequire = { imports: { - // TODO: 删除 '@opentiny/vue': getImportUrl('@opentiny/vue'), '@opentiny/vue-icon': getImportUrl('@opentiny/vue-icon'), // 'element-plus': getImportUrl('element-plus'), '@opentiny/tiny-engine-builtin-component': getImportUrl('@opentiny/tiny-engine-builtin-component') }, importStyles: [] }
39-46: Consider refactoring hard-coded dependency mappings.The hardcoded dependencies in
tinyVueRequiremake the code less maintainable. Consider moving these to a configuration file or another centralized location that can be more easily updated.- // TODO: 删除 const tinyVueRequire = { imports: { '@opentiny/vue-common': getImportUrl('@opentiny/vue-common'), '@opentiny/vue-locale': getImportUrl('@opentiny/vue-locale'), echarts: getImportUrl('echarts') } }designer-demo/tests/localCdnBasic.test.js (2)
20-39: Consider isolating tests from actual builds to improve test performance.The current test setup modifies real environment files and runs actual builds, which could make tests slow and potentially disrupt development environments if not run in isolation.
Consider mocking the build process or using a dedicated test environment to isolate these tests. This would make tests run faster and more reliably, especially in CI environments.
- // 执行构建 - execSync('pnpm run build:alpha', { - cwd: projectRoot, - stdio: 'inherit' - }) + // 在测试环境中执行构建 + execSync('pnpm run build:alpha', { + cwd: projectRoot, + stdio: 'inherit', + env: { + ...process.env, + NODE_ENV: 'test' + } + })
74-87: Make environment variable check more robust.The current method of checking for environment variable replacement by searching for a string in a file could be fragile if the build output format changes.
Consider using a more structured approach to validate the environment variable substitution, such as:
- // 检查文件内容 - const mainJsContent = fs.readFileSync(path.resolve(jsFiles[0]), 'utf-8') - - // 检查 VITE_CDN_DOMAIN 环境变量是否被替换为本地路径 - expect(mainJsContent).toContain('./local-cdn-static') + // 检查所有文件是否都正确替换了环境变量 + const allFilesContainLocalPath = jsFiles.every(file => { + const content = fs.readFileSync(file, 'utf-8') + return content.includes('./local-cdn-static') + }) + + expect(allFilesContainLocalPath).toBe(true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.gitignore(1 hunks)designer-demo/env/.env.alpha(1 hunks)designer-demo/env/.env.development(0 hunks)designer-demo/package.json(2 hunks)designer-demo/public/mock/bundle.json(1 hunks)designer-demo/tests/localCdnBasic.test.js(1 hunks)designer-demo/tests/localCdnBundleDeps.test.js(1 hunks)designer-demo/tests/localCdnCustomConfig.test.js(1 hunks)designer-demo/tests/utils/envHelpers.js(1 hunks)designer-demo/vitest.config.js(1 hunks)docs/catalog.json(1 hunks)docs/solutions/local-cdn.md(1 hunks)packages/build/vite-config/package.json(1 hunks)packages/build/vite-config/src/default-config.js(2 hunks)packages/build/vite-config/src/localCdnFile/copyBundleDeps.js(2 hunks)packages/build/vite-config/src/localCdnFile/copyImportMap.js(1 hunks)packages/build/vite-config/src/localCdnFile/index.js(1 hunks)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(1 hunks)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js(2 hunks)packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs(2 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(1 hunks)packages/canvas/DesignCanvas/src/importMap.ts(2 hunks)packages/common/js/importMap/import-map.json(2 hunks)packages/common/js/importMap/index.js(1 hunks)packages/common/package.json(1 hunks)packages/common/vite.config.ts(1 hunks)packages/design-core/package.json(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/design-core/src/preview/src/preview/importMap.js(1 hunks)packages/design-core/vite.config.js(2 hunks)
💤 Files with no reviewable changes (1)
- designer-demo/env/.env.development
🧰 Additional context used
🧠 Learnings (2)
packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/canvas/DesignCanvas/src/importMap.js:51-51
Timestamp: 2025-03-27T03:11:41.427Z
Learning: The `getImportMapData` function in `packages/canvas/DesignCanvas/src/importMap.js` has default parameter handling that initializes `canvasDeps` with empty arrays for `scripts` and `styles`, making additional null checks unnecessary.
packages/canvas/DesignCanvas/src/importMap.ts (1)
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/canvas/DesignCanvas/src/importMap.js:51-51
Timestamp: 2025-03-27T03:11:41.427Z
Learning: The `getImportMapData` function in `packages/canvas/DesignCanvas/src/importMap.js` has default parameter handling that initializes `canvasDeps` with empty arrays for `scripts` and `styles`, making additional null checks unnecessary.
🧬 Code Definitions (7)
packages/build/vite-config/src/localCdnFile/copyImportMap.js (1)
packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js (1)
getCdnPathNpmInfoForSingleFile(42-124)
designer-demo/tests/localCdnCustomConfig.test.js (3)
designer-demo/tests/localCdnBasic.test.js (5)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(15-15)localCdnDir(16-16)designer-demo/tests/localCdnBundleDeps.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(18-18)designer-demo/tests/utils/envHelpers.js (5)
backupPath(53-53)backupPath(64-64)backupEnvFile(51-56)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (4)
packages/build/vite-config/src/default-config.js (2)
logger(168-168)config(146-146)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js (12)
content(85-85)content(156-156)result(11-11)version(75-75)version(151-151)fs(7-7)src(77-77)src(152-152)sourceExist(78-78)sourceExist(153-153)packageNeedToInstall(209-218)dedupeCopyFiles(195-206)packages/build/vite-config/src/localCdnFile/copyBundleDeps.js (4)
result(16-16)fs(13-13)cdnFiles(54-56)packageNeedToInstall(58-58)packages/build/vite-config/src/localCdnFile/copyImportMap.js (1)
packageNeedToInstall(35-37)
designer-demo/tests/localCdnBundleDeps.test.js (3)
designer-demo/tests/localCdnBasic.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(15-15)designer-demo/tests/localCdnCustomConfig.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(16-16)distDir(18-18)designer-demo/tests/utils/envHelpers.js (4)
backupEnvFile(51-56)updateCdnDomain(35-44)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
packages/canvas/DesignCanvas/src/importMap.ts (2)
packages/design-core/src/preview/src/preview/importMap.js (2)
versionDelimiter(22-22)fileDelimiter(23-23)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
importMapConfig(334-334)
designer-demo/tests/localCdnBasic.test.js (3)
designer-demo/tests/localCdnBundleDeps.test.js (4)
__dirname(13-13)projectRoot(14-14)distDir(18-18)envAlphaPath(17-17)designer-demo/tests/localCdnCustomConfig.test.js (4)
__dirname(13-13)projectRoot(14-14)distDir(18-18)envAlphaPath(16-16)designer-demo/tests/utils/envHelpers.js (3)
backupEnvFile(51-56)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
designer-demo/tests/utils/envHelpers.js (1)
designer-demo/tests/localCdnCustomConfig.test.js (1)
backupPath(55-55)
🔇 Additional comments (62)
packages/build/vite-config/package.json (1)
34-34: Addition of fast-glob dependency looks appropriateThe addition of the fast-glob package will support file pattern matching functionality, which is likely needed for the local CDN plugin that this PR introduces.
packages/build/vite-config/src/localCdnFile/index.js (1)
7-7: New localCdnPlugin export enhances the module's capabilitiesThis addition exports the new localCdnPlugin functionality, which appears to be central to fixing the local import map issue mentioned in the PR title.
.gitignore (1)
9-9: Appropriate addition to .gitignoreAdding the bundle-deps directory for designer-demo to .gitignore is a good practice as these are likely generated files that shouldn't be committed to version control.
packages/common/js/importMap/index.js (1)
1-1: Good approach to centralizing import map configurationExporting the import map configuration from a central location improves maintainability and ensures consistency across the application. This change likely plays a key role in fixing the local import map functionality.
packages/design-core/package.json (1)
119-120: Added essential dependency for local import map functionality.The addition of
vite-plugin-static-copyis necessary for the local import map fix, as it enables copying static assets during the build process, which is crucial for the local CDN solution.docs/catalog.json (1)
81-82: Documentation improvement for local CDN solution.Adding documentation for the local CDN solution is valuable, especially since this PR involves fixing the local import map functionality. The placement in the "解决方案" (Solutions) section is appropriate.
packages/build/vite-config/src/localCdnFile/copyImportMap.js (1)
33-33:⚠️ Potential issueCritical fix for handling null values in style files processing.
Similar to the fix above, this
.filter(Boolean)call ensures that any null values from style URL processing are filtered out before further operations. This change is essential for robust handling of style URLs.There appears to be a syntax error in the function call. The last parameter
bundleTempDiris outside the function call parentheses:- .map((url) => getCdnPathNpmInfoForSingleFile(url, originCdnPrefix, base, dir, false), bundleTempDir) + .map((url) => getCdnPathNpmInfoForSingleFile(url, originCdnPrefix, base, dir, false, bundleTempDir))This means the
bundleTempDirparameter isn't being passed to the function, which could lead to unexpected behavior.Likely an incorrect or invalid review comment.
designer-demo/public/mock/bundle.json (1)
10166-10168: Correct import mapping for TinyGridColumn component.This change adds the
destructuring: trueproperty to properly indicate that the TinyGridColumn component should be imported using destructuring syntax from the package, which aligns with the expected behavior of the local import map system.This change is necessary for fixing the local import map functionality as mentioned in the PR title. When
destructuringis set to true, the component will be imported using the proper syntax:import { TinyGridColumn } from '@opentiny/vue'rather than as a default import.packages/common/package.json (1)
54-56: Appropriate addition of the vite-plugin-static-copy dependency.The new dependency is correctly added and will be used for copying the import-map.json file during the build process as implemented in the vite.config.ts file.
packages/common/vite.config.ts (1)
20-20: Correctly imported the viteStaticCopy plugin.The import statement properly brings in the plugin that will be used to copy the import-map.json file during build.
packages/design-core/src/preview/src/preview/importMap.js (1)
15-15:✅ Verification successful
Refactored to use module import instead of direct JSON import.
This change improves the codebase by using a centralized importMapConfig module rather than directly importing a JSON file. This approach provides better flexibility and maintainability.
Let's verify that the structure of importMapConfig matches the previously expected format:
🏁 Script executed:
#!/bin/bash # Check the structure of the importMapConfig module cat packages/common/js/importMap/index.js # Compare with the structure of the original import-map.json if it exists find packages -name "import-map.json" -exec cat {} \; 2>/dev/nullLength of output: 2612
Module import verified.
The file atpackages/common/js/importMap/index.jscorrectly re-exports the default export fromimport-map.json, and the JSON structure matches the expected format. This confirms that the refactoring to use a centralized module import improves flexibility and maintainability without altering the data structure.packages/canvas/DesignCanvas/src/DesignCanvas.vue (1)
80-80:✅ Verification successful
Simplification of import map data retrieval
The change removes the
importMapVersionparameter from thegetImportMapData()call, which aligns with the refactoring of this function in the codebase. This is a necessary fix for the local import map functionality to work correctly.
🏁 Script executed:
#!/bin/bash # Verify the updated getImportMapData function signature and implementation # Check the implementation of getImportMapData rg -A 5 "function getImportMapData" --type tsLength of output: 544
Verified: Removal of Unused Parameter in Import Map Data Retrieval
The refactoring of the
getImportMapDatafunction now omits theimportMapVersionparameter, which is confirmed by the implementation inpackages/canvas/DesignCanvas/src/importMap.ts. The call inDesignCanvas.vuehas been updated accordingly, and the changes align with the new function signature. No further modifications are needed.designer-demo/package.json (2)
9-11: Added testing support for local CDN functionalityThe addition of Vitest test scripts provides a way to validate the local CDN and import map functionality that's being fixed in this PR.
30-31: Added Vitest dependencyThe Vitest dependency is correctly added to support the new test scripts.
packages/design-core/vite.config.js (1)
9-9: Added viteStaticCopy plugin importThis import is necessary for copying the import-map.json file during the build process.
packages/design-core/src/preview/src/preview/Preview.vue (2)
25-25: Removed fetchImportMap importRemoving the
fetchImportMapimport is consistent with the changes to simplify the import map loading process.
67-69:❓ Verification inconclusive
Simplified getImportMap implementation
This change simplifies the function to always use
getInitImportMap()directly, removing conditional logic that's no longer needed since the PR enables local import maps by default through environment variables. This is a key part of fixing the local import map functionality.
🏁 Script executed:
#!/bin/bash # Verify the environment configuration for local import maps # Check for environment variables related to local import maps grep -r "VITE_LOCAL_IMPORT_MAPS" --include=".env*" . grep -r "VITE_LOCAL_BUNDLE_DEPS" --include=".env*" . # Check the getInitImportMap implementation rg -A 5 "export function getImportMap" --type jsLength of output: 1062
Simplified
getImportMapImplementation and Environment CheckThe updated function in
packages/design-core/src/preview/src/preview/Preview.vue
now directly returnsgetInitImportMap()—removing the old conditional logic—so that local import maps are consistently applied based on the environment configuration. The environment variable searches confirm that settings forVITE_LOCAL_IMPORT_MAPSandVITE_LOCAL_BUNDLE_DEPSexist in the various.envfiles (e.g. in.env.alphaversus development/production), even though the search for an exportedgetImportMapdidn’t return any results. This is expected since the function is defined as a constant rather than an exported function.Please verify manually that:
- The simplified function in the Vue component is correctly wired with the proper import and usage of
getInitImportMap().- The absence of conditional logic does not affect any other dependent functionality, given the diverse environment configurations.
designer-demo/env/.env.alpha (2)
7-8: Enable local import maps and bundle dependenciesThese changes enable both
VITE_LOCAL_IMPORT_MAPSandVITE_LOCAL_BUNDLE_DEPS, which is essential for resolving the issue mentioned in the PR title "localImportMap not works". By setting these totrue, the application will now use local versions of import maps and bundle dependencies rather than fetching from CDN.
4-4: CDN domain updated to match npmmirrorThe CDN domain has been changed to use
https://registry.npmmirror.com, which aligns with the new format support added in thelocateCdnNpmInfo.jsfile. This ensures that the import map resolver can correctly handle this CDN provider.packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs (3)
9-13: Documentation added for relativePathPattern functionClear JSDoc comments improve code maintainability by explaining the function's purpose, parameters, and return type.
18-23: Documentation added for resolvePath functionComprehensive JSDoc comments clarify how the function resolves paths, including parameter types and return values.
61-68: Enhanced documentation for babelReplaceImportPathWithCertainFileName functionDetailed JSDoc comments explain the function's purpose, parameters, and return structure, making it easier for other developers to understand and use correctly.
packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js (2)
51-75: Support for multiple CDN formats addedThis critical change adds support for both unpkg and npmmirror CDN formats by introducing separate regex patterns for each. The error handling with try-catch improves robustness, and the code now uses the first successful match (with npmmirror having priority).
This update directly addresses the root cause of the "localImportMap not works" issue by ensuring proper URL parsing regardless of the CDN format.
135-149: Multiple CDN format support for package infoSimilar to the previous change, this adds support for both unpkg and npmmirror formats in the
getCdnPathNpmInfoForPackagefunction, ensuring consistent handling across the codebase.packages/build/vite-config/src/default-config.js (2)
13-13: Replace copyPreviewImportMap with localCdnPluginUpdated import statement to use the new
localCdnPluginmodule instead ofcopyPreviewImportMap, indicating a shift to a more comprehensive plugin-based approach.
166-180: Implement local CDN plugin integrationThis new code block properly initializes and configures the local CDN plugin when local import maps are enabled. The implementation:
- Logs the initialization for better debugging
- Creates the plugin with appropriate configuration parameters
- Conditionally adds the plugins to the Vite config
This is a key part of the fix that replaces the previous implementation with a more robust plugin-based approach.
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (5)
10-23: Configuration object looks good.
Bundling default copy configurations for known packages is a clear and maintainable approach.
63-83: Environment variable replacement plugin is straightforward.
The plugin approach cleanly injects environment variables for local CDN usage. Looks solid.
176-225: Modular copy plugin is well-structured.
The plugin design is flexible and remains maintainable.
233-249: Version comparison logic is simplistic.
For caret (^) and tilde (~) comparisons, only the first or first two digits are checked. This may allow unintended mismatches (e.g.,^3.20.0vs.3.21.0). Thoroughly test different version scenarios.
91-149:❓ Verification inconclusive
Verify handling of non-text files in directory copy.
The function reads every file as UTF-8 and transforms it. This may cause issues for binary files (images, fonts, etc.). Consider skipping or handling binary files differently.
🏁 Script executed:
#!/bin/bash # Description: Identify potential binary files in localCDN directories. # Example: Searching for image or font extensions in a typical localCDN folder fd -e jpg -e png -egif -ewoff -eot -tt file local-cdn-staticLength of output: 231
Action needed: Review binary file handling in directory copy
Inpackages/build/vite-config/src/localCdnFile/localCdnPlugin.js(lines 91-149), all files are read with UTF-8 encoding and then transformed. This approach may inadvertently process binary files (images, fonts, etc.), potentially leading to corruption or errors. Please verify whether binary files should be skipped or handled differently. Also, note that the previous shell command error indicates that thefdcommand was used with an incorrect type flag – consider using-t finstead of-tt file.designer-demo/tests/localCdnBundleDeps.test.js (4)
1-4: Test documentation is helpful.
Clear JSDoc and naming improve maintainability.
48-61: Robust backup and restore logic.
Backing up the environment file before modifications is a good practice, ensuring no permanent side effects during tests.
97-115: Local resource validation test is thorough.
Verifying that the TinyVue script is localized while Element Plus remains external clarifies the intended partial-localization logic.
117-139: Ensures required packages are actually copied.
Checking for presence of TinyVue’s MJS and CSS files in the local CDN directory ensures an end-to-end test of the copy process.packages/build/vite-config/src/localCdnFile/copyBundleDeps.js (2)
15-30: Use of a Set for unique URLs is smart.
Switching to a set effectively avoids duplicates. Returning[...result]preserves the array format.
54-56: Filtering out falsy values is correct.
Usingfilter(Boolean)helps avoid errors caused by null or undefined mapping results.designer-demo/tests/localCdnCustomConfig.test.js (7)
1-4: Well-structured test file for local CDN custom configuration.The documentation comments provide a clear explanation of the test's purpose: validating custom CDN configurations in a local environment.
23-48: Function handles registry file updates properly.The
updateRegistryFilefunction correctly creates a backup of the original registry.js file before making modifications. The regex pattern for finding the config object is effective for injecting the custom importMap.
53-61: Good cleanup implementation for registry file restoration.The function properly checks for the existence of the backup file before attempting restoration, which prevents potential errors.
63-109: Thorough test setup with proper environment preparation.The
beforeAllhook appropriately:
- Backs up configuration files
- Injects custom CDN configuration
- Ensures environment variables are enabled
- Updates the registry.js file
- Executes the build process
This comprehensive setup ensures that the test environment accurately reflects a real-world scenario.
111-124: Complete cleanup in afterAll hook.The test properly cleans up all modifications made during setup, restoring the original state of configuration files. This is essential for preventing side effects in subsequent tests.
126-157: Comprehensive verification of custom importMap configuration.This test case thoroughly verifies that:
- The Vue package is correctly copied to the local CDN directory
- The directory structure is maintained as expected
- The required files exist in the specified locations
The use of file system checks provides strong validation of the feature's functionality.
159-172: Proper environment variable verification.This test ensures that the
VITE_CDN_DOMAINenvironment variable is correctly set to the local CDN path in the generated JS files, which is crucial for the proper functioning of the local CDN feature.docs/solutions/local-cdn.md (8)
1-10: Well-structured documentation with clear problem statement.The documentation begins with a concise overview of the local CDN solution and clearly outlines the problems it addresses. This helps users understand the purpose and benefits of the feature.
11-39: Clear usage instructions with examples.The documentation provides step-by-step instructions on how to enable and configure the local CDN feature, including environment variable settings and optional configuration in the Vite config file. This makes it easy for users to implement the solution.
41-50: Comprehensive configuration options documentation.The configuration options are clearly presented in a table format, making it easy for users to understand the available parameters, their types, default values, and purposes.
51-87: Detailed importMap configuration explanation.The documentation thoroughly explains the importMap format, URL structure, and the importance of consistent configuration between Vite config and registry settings. The inclusion of examples enhances clarity.
88-126: Clear copy configuration details with examples.The documentation provides comprehensive information about the copy configuration options, including default settings and examples of custom configurations. This helps users understand how to customize file copying for their specific needs.
127-166: Helpful explanation of bundle file handling.The section clearly explains how CDN links in bundle files are processed and provides an important note about the requirement for matching prefixes between bundle.json and the environment variable configuration.
167-202: Well-structured implementation principles.Breaking down the implementation principles into distinct steps helps developers understand the internal workings of the local CDN solution, which is valuable for troubleshooting or extending the functionality.
203-207: Important notes for consideration.The documentation concludes with critical considerations that users should be aware of when implementing the local CDN solution, which helps prevent potential issues.
designer-demo/tests/utils/envHelpers.js (4)
1-27: Well-implemented environment variable management utility.The
ensureEnvVarEnabledfunction effectively:
- Checks if a variable exists
- Adds it if it doesn't exist
- Sets it to true if it exists but is false
- Leaves it unchanged if it already exists and is not false
The function is well-documented with JSDoc comments and has proper error handling.
29-44: Good implementation of CDN domain update utility.The
updateCdnDomainfunction properly handles both cases:
- Adding the CDN domain if it doesn't exist
- Updating it if it already exists
The regex pattern used for matching is appropriate for environment variable files.
46-56: Simple and effective backup function.The
backupEnvFilefunction correctly checks if the file exists before attempting to create a backup, which prevents potential errors.
58-69: Proper cleanup in restore function.The
restoreEnvFilefunction appropriately:
- Checks if the backup file exists
- Copies it back to the original location
- Deletes the backup file to clean up
This ensures clean restoration without leaving unnecessary files.
packages/canvas/DesignCanvas/src/importMap.ts (1)
59-64: Good refactoring of import URLs.The replacement of hardcoded URLs with the centralized
getImportUrlfunction improves maintainability and consistency. This approach makes it easier to update URL patterns and adapt to different CDN providers.designer-demo/tests/localCdnBasic.test.js (1)
1-88: Well-structured test suite for localCDN functionality.This test file is well-organized and follows good testing practices by:
- Properly setting up the test environment in
beforeAll()- Cleaning up after testing in
afterAll()- Using clear, focused test cases with specific assertions
- Validating key aspects of the localCDN functionality
The test coverage is comprehensive, checking for:
- The creation of the local CDN directory
- Proper copying of dependencies
- Environment variable substitution
packages/common/js/importMap/import-map.json (4)
3-3: Good update to use Vue production build.Switching to the production version of Vue (
vue.runtime.esm-browser.prod.js) is appropriate for a production environment as it will be smaller and faster.
16-20: Consistent version range for OpenTiny Vue components.Updating all the OpenTiny Vue components to use the same version range (
~3.20) ensures consistency across the related packages and reduces the risk of incompatibilities.The tilde (
~) version range allows for patch updates while keeping the minor version fixed, which is a good practice for maintaining stability while accepting bug fixes.
3-21:❓ Verification inconclusive
Verify that all imports are compatible with the localCDN setup.
The updated import map is critical for the localCDN functionality being implemented. The changes to file paths and version specifications must be compatible with how the files are stored locally.
Run the following script to verify that all the imported packages specified in the import map are correctly copied to the local CDN directory:
🏁 Script executed:
#!/bin/bash # Check if all required packages are present in the local CDN directory echo "Verifying packages in local CDN directory..." # Build location from the test file TEST_DIR=$(dirname $(find designer-demo -name "localCdnBasic.test.js")) PROJECT_ROOT=$(realpath $(dirname "$TEST_DIR")/..) DIST_DIR="$PROJECT_ROOT/dist" LOCAL_CDN_DIR="$DIST_DIR/local-cdn-static" if [ ! -d "$LOCAL_CDN_DIR" ]; then echo "Error: Local CDN directory not found at $LOCAL_CDN_DIR" exit 1 fi # Check for key packages from import map echo "Checking Vue package..." find "$LOCAL_CDN_DIR" -name "vue.runtime.esm-browser.prod.js" | grep -q . if [ $? -ne 0 ]; then echo "Warning: Vue production runtime not found" fi echo "Checking Vue I18n package..." find "$LOCAL_CDN_DIR" -name "vue-i18n.esm-browser.js" | grep -q . if [ $? -ne 0 ]; then echo "Warning: Vue I18n not found" fi echo "Checking OpenTiny Vue packages..." find "$LOCAL_CDN_DIR" -name "tiny-vue-pc.mjs" | grep -q . if [ $? -ne 0 ]; then echo "Warning: OpenTiny Vue runtime not found" fi # List all packages found echo "Packages found in local CDN directory:" ls -la "$LOCAL_CDN_DIR"Length of output: 649
Action Required: Verify Local CDN Path Resolution & Package Availability
The current verification script fails to determine the project root due to the missing
realpathcommand. As a result, the computed local CDN directory is incorrect (set to/dist/local-cdn-static), and the script cannot verify if the specified packages (e.g., Vue, Vue I18n, OpenTiny Vue) are actually present.Recommendations:
- Update the Script: Replace the usage of
realpathwith a more robust path resolution method (for example, usecd "$(dirname "$TEST_DIR")/.." && pwd) to accurately compute the project root.- Manual Verification: Manually confirm that the local CDN directory exists and includes the required packages matching the import map entries.
- Re-run Verification: After updating the script, re-run the verification process to ensure all CDN imports resolve to the correct files.
5-5:❓ Verification inconclusive
Verify compatibility with updated vue-i18n version.
The version for vue-i18n has been changed from a specific beta version to
^9.9.0, which is a significant update.Please ensure that the application is fully tested with this version update, as moving from a beta version to a newer stable version might introduce compatibility issues.
🏁 Script executed:
#!/bin/bash # Check for usage of vue-i18n specific APIs that might have changed echo "Checking for vue-i18n API usage patterns..." rg -t js -t vue "createI18n|useI18n|i18n\." --no-headingLength of output: 183
Let's try a revised search command that explicitly targets Vue files by extension rather than relying on rg's file type flag. Please run the following shell script:
Once you get the output from this command, we can verify if there are any usages of the vue-i18n APIs that might be impacted by the version update. Please provide the new output if further verification is needed.
🏁 Script executed:
#!/bin/bash echo "Checking for vue-i18n API usage patterns in .js and .vue files..." rg -g '*.{js,vue}' "createI18n|useI18n|i18n\." --no-headingLength of output: 4549
Action Required: Verify Compatibility with Updated vue-i18n Version (^9.9.0)
The version update from a beta version to ^9.9.0 appears to affect several parts of the codebase where vue-i18n APIs (like
createI18n,useI18n, and usage ofi18n.global) are used. The updated search revealed these usage patterns across multiple packages (e.g., in packages/common, packages/vue-generator, packages/design-core, and others).Please perform thorough manual testing of i18n functionalities—such as translation rendering, locale switching, and merging of locale messages—to confirm that no compatibility or regression issues have been introduced by this update.
| VITE_LOCAL_IMPORT_MAPS=false | ||
| VITE_LOCAL_BUNDLE_DEPS=false | ||
| VITE_LOCAL_IMPORT_MAPS=true | ||
| VITE_LOCAL_BUNDLE_DEPS=true |
| filePathInPackage: '/' | ||
| }, | ||
| '@opentiny/vue-runtime': { | ||
| filePathInPackage: '/dist3/' |
There was a problem hiding this comment.
这个配置不能解释 @opentiny/vue映射到vue-runtime ？
我觉得这个dist3是无感的才对， 所以好像不应该存在这个配置文件
There was a problem hiding this comment.
这里默认配置是复制策略的默认配置，这几个都需要复制整个文件夹，所以作为了默认的配置。
| ) | ||
| const cdnFiles = extraBundleCdnLink(bundleFile, originCdnPrefix) | ||
| .map((url) => getCdnPathNpmInfoForSingleFile(url, originCdnPrefix, base, dir, false, bundleTempDir)) | ||
| .filter(Boolean) |
There was a problem hiding this comment.
最好是写语义化一点的？ 这个路径是可能为null是不？ 那如果是null了是如何处理？
|
|
||
| config.define['import.meta.env.VITE_CDN_DOMAIN'] = JSON.stringify(`./${cdnDir}`) | ||
| // 使用本地 CDN 时，强制设置CDN类型为 local | ||
| config.define['import.meta.env.VITE_CDN_TYPE'] = JSON.stringify('local') |
There was a problem hiding this comment.
这里的强制修改env 可能引起麻烦， 理论上不应该回头去覆盖文件变量， 通过vite_xxx_xxx 然后回设置同一个文件的vite_xxx，算是引起一种耦合和数据流问题， 下游不应该重定义上游
| * @param {string} filename - 文件名 | ||
| * @returns {string} - 处理后的内容 | ||
| */ | ||
| function transform(content, filename) { |
There was a problem hiding this comment.
不应该使用通用的名字代表特殊的内容处理，需要重命名函数
There was a problem hiding this comment.
已修改成 replaceJsImportPaths 相对语义化的命名
| // 使用自定义的copyPlugin替代直接调用copy | ||
| copyPlugin(targetFiles) | ||
| ] | ||
| } |
There was a problem hiding this comment.
这个文件似乎有很多不同层次的插件， 逻辑分析的 文件系统操作的，请考虑拆成每个插件一个js文件，把不同层次的业务内容摘出去，比如默认的importmap文件要从最外层使用的地方配置默认文件
There was a problem hiding this comment.
已提取 cdnCopyPlugin 和 createEnvReplacementPlugin 两个 vite 插件到 vite-plugins 文件夹下
| } | ||
|
|
||
| // 使用匹配到的结果 | ||
| const match = npmmirrorMatch || unpkgMatch |
| new RegExp(`^${originCdnPrefix}/?(?<packageName>.+?)@(?<versionDemand>[^/]+)(?<filePathInPackage>.*?)$`) | ||
| ).groups | ||
| ) | ||
| const npmmirrorMatch = url.match( |
| @@ -1,8 +1,8 @@ | |||
| { | |||
| "imports": { | |||
| "vue": "${VITE_CDN_DOMAIN}/vue${versionDelimiter}3.4.23${fileDelimiter}/dist/vue.runtime.esm-browser.js", | |||
| "vue": "${VITE_CDN_DOMAIN}/vue${versionDelimiter}3.4.23${fileDelimiter}/dist/vue.runtime.esm-browser.prod.js", | |||
There was a problem hiding this comment.
当时是画布和预览使用开发模式下的vue方便调试问题（获得更多的warning警告信息）
There was a problem hiding this comment.
已修改成 vue.runtime.esm-browser.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
86-176: Consider a streaming approach for large directories
Reading all file contents into memory can be inefficient for large files or directories. A streaming or chunked copy mechanism might help reduce memory usage and improve performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
designer-demo/package.json(2 hunks)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- designer-demo/package.json
🧰 Additional context used
🧬 Code Definitions (1)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (4)
packages/build/vite-config/src/default-config.js (2)
logger(168-168)config(146-146)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js (13)
content(85-85)content(156-156)result(11-11)version(75-75)version(151-151)fs(7-7)src(77-77)src(152-152)isFolder(76-76)sourceExist(78-78)sourceExist(153-153)stat(81-81)packageNeedToInstall(209-218)packages/build/vite-config/src/localCdnFile/copyBundleDeps.js (4)
result(16-16)fs(13-13)cdnFiles(54-56)packageNeedToInstall(58-58)packages/build/vite-config/src/localCdnFile/copyImportMap.js (1)
packageNeedToInstall(35-37)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (3)
31-31: Rename thetransformfunction to clarify its specialized role
The function name "transform" is too generic; a more precise name would help clarify its intent and usage.
81-81: Reevaluate forcibly setting environment variables
Forcibly modifying environment variables here might introduce data flow and coupling issues.
319-389: Split multiple plugins into separate modules
This file contains several plugin definitions and file-system operations at different abstraction levels. Consider refactoring into smaller modules for better maintainability.
| const compareIsSameVersion = (versionOrigin, versionTarget) => { | ||
| if (versionOrigin === versionTarget) { | ||
| return true | ||
| } | ||
|
|
||
| if (versionOrigin.startsWith('^')) { | ||
| // 如果源版本号是 ^ 开头，则只比较第一个数字是否相同 | ||
| return versionOrigin.slice(1, 2) === versionTarget.slice(0, 1) | ||
| } | ||
|
|
||
| if (versionOrigin.startsWith('~')) { | ||
| // 如果源版本号是 ~ 开头，则只比较前两个数字是否相同 | ||
| return versionOrigin.slice(1, 3) === versionTarget.slice(0, 3) | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify partial matching in compareIsSameVersion
The handling for ^ and ~ only checks the first digit(s) rather than fully applying semver rules. This may lead to incorrect matches or missed breaking changes.
🌐 Web query:
Are partial matches of ^ or ~ sufficient for semver compliance?
💡 Result:
Semantic Versioning (SemVer) compliance for partial matches using ^ (caret) and ~ (tilde) operators depends on their alignment with SemVer rules:
Caret (^) Ranges
- Full syntax:
^1.2.3expands to>=1.2.3 <2.0.0, allowing minor and patch updates but blocking major breaks[2][3]. - Partial syntax:
^1.2⇨>=1.2.0 <2.0.0(equivalent to full major.minor.patch)[2].^1⇨>=1.0.0 <2.0.0(same as^1.0.0)[3].
- Compliance: Valid for SemVer compliance if the toolchain interprets partial versions correctly. However,
^0.2.3is treated as>=0.2.3 <0.3.0due to0.x.xversions’ instability, effectively pinning to the exact0.2.xseries[2][5].
Tilde (~) Ranges
- Full syntax:
~1.2.3⇨>=1.2.3 <1.3.0, allowing only patch updates[2][3]. - Partial syntax:
~1.2⇨>=1.2.0 <1.3.0(auto-fills missing.patchas.0)[2].~1⇨>=1.0.0 <2.0.0(ambiguous; some tools default to minor increments)[3].
- Compliance: Valid when targeting specific minor versions but risky for partial ranges like
~1, which might permit unintended minor changes[3].
Key Considerations
- Zero Major Versions:
0.x.xversions bypass SemVer’s stability guarantees.^0.2.3allows no changes, as0.ximplies instability[2][5]. - Tool Behavior: Not all tools handle partial ranges identically. For example:
npmtreats~1as>=1.0.0 <2.0.0(matching carets)[5].- Rust’s
cargoflags partial ranges as potentially error-prone in unstable contexts[4].
- Build Metadata: Versions differing only in build metadata (e.g.,
1.0.0+build) are treated as equal, but ranges exclude them unless explicitly allowed[5][7].
Recommendations for Compliance
- Use exact ranges (e.g.,
~1.2.3) for precise control. - Avoid
0.x.xranges where possible, as they undermine SemVer’s assumptions. - Validate tool-specific interpretations to avoid unintended upgrades[4][6].
In summary, partial ^/~ ranges are SemVer-compliant if they align with intended versioning policies, but their safety depends on precise syntax and toolchain behavior.
Citations:
- 1: https://semver.org
- 2: http://nodesource.com/blog/semver-a-primer/
- 3: https://portal.ecg.co/guides/alpaca-cli/5.3/semver
- 4: https://rust-lang.github.io/rust-project-goals/2025h1/cargo-semver-checks.html
- 5: https://docs.npmjs.com/cli/v6/using-npm/semver/
- 6: https://pure.tue.nl/ws/files/199634997/Ochoa2022_Article_BreakingBadSemanticVersioningA.pdf
- 7: https://python-semanticversion.readthedocs.io/en/latest/reference.html
- 8: https://jolynch.github.io/posts/semver_considered_harmful/
- 9: https://python-semanticversion.readthedocs.io/en/v1.0.0/reference.html
- 10: The terms MAJOR.MINOR.PATCH are not semantic semver/semver#625
Action: Revise SemVer Handling in compareIsSameVersion
The current implementation only compares the first digit(s) for versions prefixed with ^ or ~. While partial matching can be acceptable if your toolchain interprets them as full ranges (e.g., treating ^1.2.3 as >=1.2.3 <2.0.0), this approach can miss critical semver nuances—especially for versions with major version zero or when only partial version strings are provided. To avoid unexpected mismatches or accidental breaking changes, consider one of the following:
- Use a Dedicated SemVer Library: Adopt a semantic version parser (like the
semvernpm package) to ensure comparisons fully adhere to semver rules. - Document and Align: If you intend to keep the current approach, document its behavior clearly and verify that it aligns with your project's versioning policy.
Location: packages/build/vite-config/src/localCdnFile/localCdnPlugin.js, Lines 235-251
chilingling
force-pushed
the
feat/localCDNPlugin
branch
from
a58e045 to
4338a95
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/canvas/DesignCanvas/src/importMap.ts (1)
4-24:⚠️ Potential issueAdd error handling for missing package configurations.
The
getImportUrlfunction does not handle the case when a package is not found in either the custom import map or the importMapConfig. This could lead to undefined being returned, potentially causing runtime errors.Add error handling for missing packages:
const getImportUrl = (pkgName: string) => { // 自定义的 importMap const customImportMap = getMergeMeta('engine.config')?.importMap const { VITE_CDN_TYPE, VITE_CDN_DOMAIN } = useEnv() const versionDelimiter = VITE_CDN_TYPE === 'npmmirror' ? '/' : '@' const fileDelimiter = VITE_CDN_TYPE === 'npmmirror' ? '/files' : '' if (customImportMap?.imports?.[pkgName]) { return customImportMap.imports[pkgName] .replace('${VITE_CDN_DOMAIN}', VITE_CDN_DOMAIN) .replace('${versionDelimiter}', versionDelimiter) .replace('${fileDelimiter}', fileDelimiter) } if (importMapConfig.imports[pkgName]) { return importMapConfig.imports[pkgName] .replace('${VITE_CDN_DOMAIN}', VITE_CDN_DOMAIN) .replace('${versionDelimiter}', versionDelimiter) .replace('${fileDelimiter}', fileDelimiter) } + + console.warn(`Package ${pkgName} not found in import maps`) + return `${VITE_CDN_DOMAIN}/${pkgName}@latest` }
🧹 Nitpick comments (6)
packages/canvas/DesignCanvas/src/importMap.ts (2)
30-34: Review the TODO comment for cleanup.There's a TODO comment indicating that some code should be deleted. Consider addressing this or provide a clearer comment about when and why this code will be removed.
40-46: Address TODO comment and consider documenting dependency requirements.The TODO comment indicates future deletion. Since this code handles dependencies that aren't supported in the material protocol, consider documenting these requirements or creating an issue to properly address them in the future.
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
214-215: Remove commented-out code.Leaving the old line in comment form can lead to confusion. If it's no longer needed, remove it to keep the codebase clean.
- // const srcPaths = (Array.isArray(src) ? src : [src]).map(item => path.resolve(process.cwd(), item))designer-demo/tests/localCdnBundleDeps.test.js (1)
37-38: Consider testing with consistent CDN domains.You're using
registry.npmmirror.comfor element-plus butunpkg.comfor TinyVue. Line 69 sets VITE_CDN_DOMAIN to 'https://unpkg.com', so only matching URLs will be localized.For more comprehensive testing, consider either:
- Using the same CDN domain for all test components, or
- Adding a separate test case with a different VITE_CDN_DOMAIN value to verify multiple domains can be handled.
docs/solutions/local-cdn.md (2)
192-192: Unclear explanation of JavaScript path replacement.The explanation of path replacement transformation could be more detailed to help users understand exactly what transformations are applied and why they are necessary.
Consider adding an example to clarify the JavaScript path replacement transformation:
- import push from './push' + import push from './push.js'This would make it clearer that the transformation adds file extensions for proper browser imports.
203-207: Consider adding more specific information about size impacts.The note about increased build output size could be more helpful with specific information about typical size increases or strategies to manage them.
Consider adding more specific guidance about size impacts, such as:
- Approximate size increase percentage for typical applications
- Recommendations for which resources to prioritize for local CDN
- Whether gzip/compression helps mitigate the size impact
- Any build flags or options to optimize the size
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.gitignore(1 hunks)designer-demo/env/.env.alpha(1 hunks)designer-demo/env/.env.development(0 hunks)designer-demo/package.json(2 hunks)designer-demo/public/mock/bundle.json(1 hunks)designer-demo/tests/localCdnBasic.test.js(1 hunks)designer-demo/tests/localCdnBundleDeps.test.js(1 hunks)designer-demo/tests/localCdnCustomConfig.test.js(1 hunks)designer-demo/tests/utils/envHelpers.js(1 hunks)designer-demo/vitest.config.js(1 hunks)docs/catalog.json(1 hunks)docs/solutions/local-cdn.md(1 hunks)packages/build/vite-config/package.json(1 hunks)packages/build/vite-config/src/default-config.js(2 hunks)packages/build/vite-config/src/localCdnFile/copyBundleDeps.js(2 hunks)packages/build/vite-config/src/localCdnFile/copyImportMap.js(1 hunks)packages/build/vite-config/src/localCdnFile/index.js(1 hunks)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(1 hunks)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js(2 hunks)packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs(2 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(1 hunks)packages/canvas/DesignCanvas/src/importMap.ts(2 hunks)packages/common/js/importMap/import-map.json(2 hunks)packages/common/js/importMap/index.js(1 hunks)packages/common/package.json(1 hunks)packages/common/vite.config.ts(1 hunks)packages/design-core/package.json(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/design-core/src/preview/src/preview/importMap.js(1 hunks)packages/design-core/vite.config.js(2 hunks)
💤 Files with no reviewable changes (1)
- designer-demo/env/.env.development
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/build/vite-config/src/localCdnFile/index.js
- packages/common/js/importMap/index.js
- packages/build/vite-config/src/localCdnFile/copyImportMap.js
- designer-demo/vitest.config.js
- packages/design-core/package.json
- packages/build/vite-config/package.json
- packages/design-core/src/preview/src/preview/importMap.js
- packages/canvas/DesignCanvas/src/DesignCanvas.vue
- packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs
- packages/common/package.json
- packages/build/vite-config/src/default-config.js
- designer-demo/env/.env.alpha
- packages/build/vite-config/src/localCdnFile/copyBundleDeps.js
- packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js
- packages/design-core/vite.config.js
- docs/catalog.json
- designer-demo/package.json
- .gitignore
- designer-demo/tests/localCdnBasic.test.js
- packages/design-core/src/preview/src/preview/Preview.vue
- designer-demo/tests/utils/envHelpers.js
- packages/common/js/importMap/import-map.json
🧰 Additional context used
🧠 Learnings (1)
packages/canvas/DesignCanvas/src/importMap.ts (1)
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/canvas/DesignCanvas/src/importMap.js:51-51
Timestamp: 2025-04-01T02:05:43.045Z
Learning: The `getImportMapData` function in `packages/canvas/DesignCanvas/src/importMap.js` has default parameter handling that initializes `canvasDeps` with empty arrays for `scripts` and `styles`, making additional null checks unnecessary.
🧬 Code Definitions (3)
designer-demo/tests/localCdnCustomConfig.test.js (3)
designer-demo/tests/localCdnBasic.test.js (5)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(15-15)localCdnDir(16-16)designer-demo/tests/localCdnBundleDeps.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(18-18)designer-demo/tests/utils/envHelpers.js (5)
backupPath(53-53)backupPath(64-64)backupEnvFile(51-56)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
designer-demo/tests/localCdnBundleDeps.test.js (3)
designer-demo/tests/localCdnBasic.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(15-15)designer-demo/tests/localCdnCustomConfig.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(16-16)distDir(18-18)designer-demo/tests/utils/envHelpers.js (4)
backupEnvFile(51-56)updateCdnDomain(35-44)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
packages/canvas/DesignCanvas/src/importMap.ts (2)
packages/design-core/src/preview/src/preview/importMap.js (2)
versionDelimiter(22-22)fileDelimiter(23-23)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
importMapConfig(336-336)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (30)
packages/canvas/DesignCanvas/src/importMap.ts (4)
1-2: Good modularization by importing the importMapConfig from a common package.The code now leverages a centralized import map configuration from the common package, which promotes reusability and consistency across the codebase.
26-26: Improved function signature by removing unnecessary parameter.The removal of the
overrideVersionsparameter simplifies the function signature and reduces complexity. This change is good for maintainability.
36-36: Verify the empty importStyles array.The
importStylesarray is now empty. Confirm if this is intentional and won't affect style loading in the application.
59-60: Good refactoring to use the dynamic import URL generation.Replacing hardcoded URLs with calls to
getImportUrlimproves maintainability and consistency. The code is now more flexible to accommodate different CDN configurations.packages/common/vite.config.ts (2)
20-20: Properly importing new dependency.The addition of the
viteStaticCopyplugin from 'vite-plugin-static-copy' is key to fixing the local import map issue. This ensures the import-map.json file will be available where needed.
28-41:❓ Verification inconclusive
Good refactoring of plugins configuration with clear purpose.
The restructured plugins array with each plugin on its own line improves readability. The added comment at line 32 clearly explains the purpose of the viteStaticCopy plugin - to copy the import-map.json file to the build output for other build plugins to use.
This change directly addresses the PR objective of fixing the local import map functionality by ensuring the import map file is properly copied to the output directory.
🏁 Script executed:
#!/bin/bash # Verify that the source import-map.json file exists and has content fd -t f "importMap/import-map.json" packages/common/js/ | xargs catLength of output: 67
Attention: Verify Import Map File Existence
The refactored Vite plugins configuration looks clear and well-organized, and the inline comment at line 32 effectively explains the plugin's purpose. This change aligns with the PR objective to fix the local import map functionality by copying the import-map.json file to the build output.
However, the shell script check for the existence and content of the import map file produced no output. Please manually verify that the file exists at the expected path (
packages/common/js/importMap/import-map.json) and contains the necessary data.packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (6)
31-37: Use a more descriptive function name.This function handles specific Babel-based import path transformations, yet it is named generically as
transform. Previous feedback advised renaming generic functions to better reflect the specialized logic.
49-51: Ensure placeholders are properly defined.Variables
${versionDelimiter}and${fileDelimiter}are referenced, but they are not declared anywhere in this file. Verify that they are indeed set elsewhere or passed in properly.
77-82: Reconsider forcible environment variable overrides.Forcibly setting
import.meta.env.VITE_CDN_DOMAINandimport.meta.env.VITE_CDN_TYPEmight cause unintended coupling and confusion for downstream code.
235-251: Revise partial SemVer comparisons.Using only the first or first two digits to match
^or~dependencies can cause unmanaged breaking changes or mismatches. Consider a robust SemVer strategy.
339-341: Handle missing import-map.json gracefully.Reading this JSON file without checking if it exists or is valid can lead to runtime errors. Ensure robust error handling before parsing.
319-389: Split this file into smaller, separate plugins.This file defines multiple plugin layers (env replacement, package installation, file copying). Prior feedback suggests extracting each into its own file or layer to reduce complexity.
designer-demo/public/mock/bundle.json (1)
10167-10168: Integrate the new "destructuring" property for module imports.
The addition of"destructuring": truein thenpmconfiguration forTinyGridColumnindicates that the component may now support destructured imports. Please confirm that all consuming modules and the local import map functionality correctly handle this new property. Also, since tests and documentation updates are pending in this PR, ensure that the new behavior is adequately covered in future commits or issues.designer-demo/tests/localCdnBundleDeps.test.js (7)
9-10: Node.js import pattern looks good.The use of
node:prefix for core Node.js modules follows best practices for explicit protocol imports.
29-32: Good test setup for TinyVue component.The test data correctly includes both script and CSS CDN URLs for TinyVue which is necessary for comprehensive testing of the localization feature.
69-72: Good environment setup for test isolation.The code correctly updates the CDN domain and enables the local bundle dependency feature, ensuring proper test isolation.
107-108: Correct assertion for CDN URL replacement.The test properly verifies that TinyVue URLs have been replaced with local paths.
110-114: Good negative test for non-matching CDN domains.The test correctly verifies that element-plus URLs remain unchanged since they don't match the configured CDN domain.
128-132: File existence checks look good.The test correctly verifies the existence of both JS and CSS files for TinyVue in the localized CDN directory.
134-139: Good negative test for excluded packages.The test correctly verifies that element-plus is not copied to the local CDN directory, which is consistent with the expected behavior based on CDN domain filtering.
designer-demo/tests/localCdnCustomConfig.test.js (6)
71-87: Good custom configuration for localCdnConfig.The test correctly adds a custom configuration for Vue to test the local CDN feature with a specific version and file path.
94-96: Ensure proper enabling of environment variables.The code correctly ensures that both VITE_LOCAL_IMPORT_MAPS and VITE_LOCAL_BUNDLE_DEPS are enabled for the test.
126-149: Good verification of custom importMap configuration.The test properly checks that Vue is correctly copied to the local CDN directory and verifies the existence of the expected files.
150-157: Thorough verification of copied files.The test correctly verifies that multiple files were copied to the dist directory, confirming the entire directory was processed as specified in the custom configuration.
159-172: Environment variable replacement test looks good.The second test properly verifies that the VITE_CDN_DOMAIN environment variable is correctly set to the local path in the generated JS files.
23-48:❓ Verification inconclusive
Update registry file function uses risky regex replacement.
The function uses regex to modify the registry.js file. While functional, regex-based file modifications can be brittle if the file structure changes.
🏁 Script executed:
#!/bin/bash # Check the structure of registry.js to ensure the regex pattern is still applicable # Identify the location of the config object grep -n "config: {" $(find . -name "registry.js" | head -n 1) # Check for any existing importMap in registry.js grep -n "importMap" $(find . -name "registry.js" | head -n 1)Length of output: 179
Below is a new script to display the contents of the registry file (around the config object and any importMap) so we can better assess whether the regex replacement in the updateRegistryFile function is safe given the file’s structure.
Please run the following shell script to gather more detailed context:
🏁 Script executed:
#!/bin/bash FILE=$(find . -name "registry.js" | head -n 1) if [ -z "$FILE" ]; then echo "registry.js not found in repository!" else echo "Found registry.js at: $FILE" echo "---------- Content around 'config:' object ----------" grep -n -C3 "config: {" "$FILE" echo "---------- Content around 'importMap' (if any) ----------" grep -n -C3 "importMap" "$FILE" fiLength of output: 585
Action Required: Verify Registry File Structure for Regex Update
While the proposed regex-based change in the
updateRegistryFilefunction may work under expected conditions, our automated checks did not return output confirming the existence or exact structure of theconfig:object indesigner-demo/registry.js. This absence of evidence makes it hard to validate that the regex correctly matches the intended configuration block.
- Ensure that the
registry.jsfile indeed contains aconfig:block formatted as expected.- Confirm there is no existing
importMapthat the script might inadvertently override.- Consider revising the approach to use a more robust parsing method (e.g., AST or JSON parsing, if applicable) to avoid potential brittleness as the file structure evolves.
Please manually verify the file’s content and structure before merging this change.
docs/solutions/local-cdn.md (4)
15-27: Clear instructions for enabling local CDN functionality.The documentation provides clear and concise steps for enabling the local CDN feature through environment variables.
45-50: Well-structured configuration options table.The documentation includes a well-formatted table that clearly presents the available configuration options for the local CDN feature.
101-103: Questionable configuration for @opentiny/vue-runtime.The
/dist3/path for @opentiny/vue-runtime may need further explanation or revision as it was previously questioned in a review.As noted in a previous review comment, this configuration for @opentiny/vue-runtime pointing to
/dist3/seems unusual and might not be transparent to users.
131-132: Important note about CDN domain matching.The documentation correctly emphasizes the importance of matching the CDN domain prefix in the bundle.json file with the VITE_CDN_DOMAIN environment variable.
chilingling
force-pushed
the
feat/localCDNPlugin
branch
from
4338a95 to
55bf74c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
docs/solutions/local-cdn.md (1)
102-102: Check if/dist3/path is necessary for@opentiny/vue-runtimeThe configuration specifies
filePathInPackage: '/dist3/'for@opentiny/vue-runtime. This seems unusual since a previous comment suggested that "dist3 is supposed to be transparent". Consider whether this explicit path configuration is actually necessary.#!/bin/bash # Verify the structure of @opentiny/vue-runtime package to determine if /dist3/ is required # Check if the package is installed if [ -d "node_modules/@opentiny/vue-runtime" ]; then echo "Package exists, listing directory structure:" ls -la node_modules/@opentiny/vue-runtime # Check if dist3 directory exists if [ -d "node_modules/@opentiny/vue-runtime/dist3" ]; then echo "dist3 directory exists, listing content:" ls -la node_modules/@opentiny/vue-runtime/dist3 else echo "dist3 directory does not exist" fi else echo "Package @opentiny/vue-runtime is not installed" fipackages/canvas/DesignCanvas/src/importMap.ts (1)
4-24:⚠️ Potential issueAdd error handling for missing packages
The
getImportUrlfunction does not handle the case when a package is not found in either the custom import map or the importMapConfig. This could lead to undefined being returned, potentially causing runtime errors.Add error handling for missing packages:
const getImportUrl = (pkgName: string) => { // 自定义的 importMap const customImportMap = getMergeMeta('engine.config')?.importMap const { VITE_CDN_TYPE, VITE_CDN_DOMAIN } = useEnv() const versionDelimiter = VITE_CDN_TYPE === 'npmmirror' ? '/' : '@' const fileDelimiter = VITE_CDN_TYPE === 'npmmirror' ? '/files' : '' if (customImportMap?.imports?.[pkgName]) { return customImportMap.imports[pkgName] .replace('${VITE_CDN_DOMAIN}', VITE_CDN_DOMAIN) .replace('${versionDelimiter}', versionDelimiter) .replace('${fileDelimiter}', fileDelimiter) } if (importMapConfig.imports[pkgName]) { return importMapConfig.imports[pkgName] .replace('${VITE_CDN_DOMAIN}', VITE_CDN_DOMAIN) .replace('${versionDelimiter}', versionDelimiter) .replace('${fileDelimiter}', fileDelimiter) } + + console.warn(`Package ${pkgName} not found in import maps`) + return `${VITE_CDN_DOMAIN}/${pkgName}@latest` }
🧹 Nitpick comments (13)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (4)
8-8: Prefer a consistent logger reference over the bare consoleYou're assigning
const logger = consoleat line 8, but in other places (e.g. line 33) you're directly passingconsoletobabelReplaceImportPathWithCertainFileName. For consistency and future flexibility (e.g. switching to a more robust logging library), consider using theloggervariable throughout.-const result = babelReplaceImportPathWithCertainFileName(content, filename, console) +const result = babelReplaceImportPathWithCertainFileName(content, filename, logger)
63-84: Check for potential environment variable overridesThe
createEnvReplacementPluginfunction (lines 68–84) forcefully sets certain environment variables (VITE_CDN_DOMAINandVITE_CDN_TYPEto'local'). If a user wants a different local domain or environment variable configuration, this override may cause confusion. Provide an option or parameter to opt out of these forced overrides.
178-227: Evaluate plugin layering and responsibilitiesThe
copyPluginfunction (lines 186–227) performs multiple tasks: reading config, iterating directories, and copying files. Meanwhile, thecopyFileOrDirectorylogic is quite extensive. Consider moving the copy logic into a separate utility, makingcopyPluginfocus on plugin lifecycle events. This adheres to the single-responsibility principle and can simplify testing.
253-317: Consider alternative bundle detection for installed packagesWithin
getCdnPathNpmInfo, there’s a check forisSameVersionandsourceExist. If the package version differs slightly, the code forcibly references thetempDir. This may lead to re-downloading or ignoring local dependencies that are compatible. If partial matches are acceptable, you can incorporate a logic to handle minor patch differences.designer-demo/tests/localCdnBundleDeps.test.js (3)
49-75: Potential test brittleness due to forced environment modificationsIn
beforeAll, the test forcibly updates.env.alpha, enabling local CDN behavior. If other tests rely on different environment settings, they might break. You could isolate these environment changes (e.g., by using a separate.envfor tests) or mock environment variables without writing to actual files.
97-115: Clarify test expectations for non-localized dependenciesThe first test confirms that TinyVue packages are localized and that Element Plus remains on the remote registry. This is good for verifying selective localization. Consider referencing a comment or test name clarifying that the second dependency is intentionally left remote.
117-139: Extend coverage to negative or edge casesThe second test verifies that certain assets (Element Plus) are not copied to the local directory. You might also consider testing edge cases (e.g., an empty
packagesarray, older versions, or partial version matches) for stronger confidence that the local CDN logic handles all scenarios.designer-demo/tests/localCdnCustomConfig.test.js (1)
64-87: Ensure minimal intrusion to default configThe code forcibly injects
localCdnConfiginto the existingvite.config.js. If users have custom hooks or additional properties inbaseConfig, they might be overridden. Provide an option to merge or selectively override properties.packages/canvas/DesignCanvas/src/importMap.ts (2)
30-35: Consider cleaning up TODO comments and uncommented codeThe code contains a TODO comment and commented-out code for
element-plus. These should either be addressed or removed to improve code clarity.- // TODO: 删除 '@opentiny/vue': getImportUrl('@opentiny/vue'), '@opentiny/vue-icon': getImportUrl('@opentiny/vue-icon'), - // 'element-plus': getImportUrl('element-plus'), '@opentiny/tiny-engine-builtin-component': getImportUrl('@opentiny/tiny-engine-builtin-component')
40-41: Remove unnecessary TODO commentThe
TODO: 删除comment should be addressed or removed for code clarity.-// TODO: 删除designer-demo/tests/localCdnBasic.test.js (3)
58-60: Make file path checks more robustThe test is checking for a specific file path that might change with package versions. Consider making this check more resilient to version changes.
- const indexJsExists = fs.existsSync(path.resolve(localCdnDir, '@vue', devToolsDirs, 'lib/esm/index.js')) + // Check if any index.js file exists in the package directory structure + const packageDir = path.resolve(localCdnDir, '@vue', devToolsDirs) + const indexJsExists = !!execSync(`find ${packageDir} -name "index.js" | head -1`, { encoding: 'utf-8' }).trim()
69-71: Make file path checks more flexibleThe test is checking for a specific Vue runtime file path that might change with Vue versions. Consider implementing a more flexible approach that can accommodate different file structures.
- const vueProdDist = path.resolve(localCdnDir, runtimeDirs, 'dist/vue.runtime.esm-browser.prod.js') - - expect(fs.existsSync(vueProdDist)).toBe(true) + // Check if any Vue runtime file exists + const vueDir = path.resolve(localCdnDir, runtimeDirs, 'dist') + const vueRuntimeExists = fs.readdirSync(vueDir) + .some(file => file.includes('vue.runtime') && file.endsWith('.js')) + + expect(vueRuntimeExists).toBe(true)
74-87: Enhance environment variable replacement testThe current test only checks if
'./local-cdn-static'appears in the file content. Consider adding a more specific check to verify the environment variable is correctly replaced in the expected context.it('应该修改环境变量将VITE_CDN_DOMAIN设置为./local-cdn-static', () => { // 检查构建后生成的JS文件中是否包含 VITE_CDN_DOMAIN: './local-cdn-static' const jsFiles = fs.readdirSync(path.resolve(distDir, 'assets')) .filter(file => file.endsWith('.js') && file.startsWith('preview-')) .map(file => path.resolve(distDir, 'assets', file)) expect(jsFiles.length).toBeGreaterThan(0) // 检查文件内容 const mainJsContent = fs.readFileSync(path.resolve(jsFiles[0]), 'utf-8') // 检查 VITE_CDN_DOMAIN 环境变量是否被替换为本地路径 - expect(mainJsContent).toContain('./local-cdn-static') + // More specifically check if the VITE_CDN_DOMAIN is set to local-cdn-static + const cdnDomainRegex = /VITE_CDN_DOMAIN\s*[:=]\s*['"]\.\/local-cdn-static['"]/ + expect(cdnDomainRegex.test(mainJsContent)).toBe(true) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.gitignore(1 hunks)designer-demo/env/.env.alpha(1 hunks)designer-demo/env/.env.development(0 hunks)designer-demo/package.json(2 hunks)designer-demo/public/mock/bundle.json(1 hunks)designer-demo/tests/localCdnBasic.test.js(1 hunks)designer-demo/tests/localCdnBundleDeps.test.js(1 hunks)designer-demo/tests/localCdnCustomConfig.test.js(1 hunks)designer-demo/tests/utils/envHelpers.js(1 hunks)designer-demo/vitest.config.js(1 hunks)docs/catalog.json(1 hunks)docs/solutions/local-cdn.md(1 hunks)packages/build/vite-config/package.json(1 hunks)packages/build/vite-config/src/default-config.js(2 hunks)packages/build/vite-config/src/localCdnFile/copyBundleDeps.js(2 hunks)packages/build/vite-config/src/localCdnFile/copyImportMap.js(1 hunks)packages/build/vite-config/src/localCdnFile/index.js(1 hunks)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(1 hunks)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js(2 hunks)packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs(2 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(1 hunks)packages/canvas/DesignCanvas/src/importMap.ts(2 hunks)packages/common/js/importMap/import-map.json(2 hunks)packages/common/js/importMap/index.js(1 hunks)packages/common/package.json(1 hunks)packages/common/vite.config.ts(1 hunks)packages/design-core/package.json(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/design-core/src/preview/src/preview/importMap.js(1 hunks)packages/design-core/vite.config.js(2 hunks)
💤 Files with no reviewable changes (1)
- designer-demo/env/.env.development
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/common/js/importMap/index.js
- .gitignore
- packages/build/vite-config/src/localCdnFile/index.js
- packages/build/vite-config/package.json
- packages/design-core/package.json
- packages/design-core/src/preview/src/preview/importMap.js
- packages/build/vite-config/src/localCdnFile/copyImportMap.js
- packages/common/package.json
- packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs
- packages/common/vite.config.ts
- packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js
- packages/build/vite-config/src/default-config.js
- packages/canvas/DesignCanvas/src/DesignCanvas.vue
- designer-demo/tests/utils/envHelpers.js
- docs/catalog.json
- packages/design-core/src/preview/src/preview/Preview.vue
- packages/design-core/vite.config.js
- designer-demo/env/.env.alpha
- packages/build/vite-config/src/localCdnFile/copyBundleDeps.js
- designer-demo/vitest.config.js
- designer-demo/package.json
- packages/common/js/importMap/import-map.json
🧰 Additional context used
🧬 Code Definitions (5)
designer-demo/tests/localCdnCustomConfig.test.js (3)
designer-demo/tests/localCdnBasic.test.js (5)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(15-15)localCdnDir(16-16)designer-demo/tests/localCdnBundleDeps.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(18-18)designer-demo/tests/utils/envHelpers.js (5)
backupPath(53-53)backupPath(64-64)backupEnvFile(51-56)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
designer-demo/tests/localCdnBundleDeps.test.js (3)
designer-demo/tests/localCdnBasic.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(15-15)designer-demo/tests/localCdnCustomConfig.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(16-16)distDir(18-18)designer-demo/tests/utils/envHelpers.js (4)
backupEnvFile(51-56)updateCdnDomain(35-44)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
packages/canvas/DesignCanvas/src/importMap.ts (4)
packages/register/src/common.ts (1)
getMergeMeta(179-181)packages/register/src/hooks.ts (1)
useEnv(90-90)packages/design-core/src/preview/src/preview/importMap.js (2)
versionDelimiter(22-22)fileDelimiter(23-23)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
importMapConfig(336-336)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (5)
packages/build/vite-config/src/default-config.js (2)
logger(168-168)config(146-146)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js (9)
content(85-85)content(156-156)result(11-11)version(75-75)version(151-151)fs(7-7)src(77-77)src(152-152)packageNeedToInstall(209-218)packages/build/vite-config/src/localCdnFile/copyBundleDeps.js (4)
result(16-16)fs(13-13)cdnFiles(54-56)packageNeedToInstall(58-58)packages/build/vite-config/src/localCdnFile/copyImportMap.js (1)
packageNeedToInstall(35-37)packages/build/vite-config/src/vite-plugins/installPackageTemporary.js (1)
installPackageTemporary(5-47)
designer-demo/tests/localCdnBasic.test.js (3)
designer-demo/tests/localCdnCustomConfig.test.js (4)
__dirname(13-13)projectRoot(14-14)distDir(18-18)envAlphaPath(16-16)designer-demo/tests/localCdnBundleDeps.test.js (4)
__dirname(13-13)projectRoot(14-14)distDir(18-18)envAlphaPath(17-17)designer-demo/tests/utils/envHelpers.js (3)
backupEnvFile(51-56)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
designer-demo/public/mock/bundle.json (1)
10167-10168:✅ Verification successful
Verify and Document the New Destructuring Import Setting
The addition of
"destructuring": truewithin thenpmobject forTinyGridColumnis intended to enable more flexible or alternative import methods (e.g., destructured imports) for this component. Please verify that the local import map logic correctly interprets this new property, and ensure that all consuming code and test suites are updated accordingly. Additionally, update the related documentation to inform developers about this new configuration option and its intended behavior.
Below are new shell scripts to help verify that the new
"destructuring": trueproperty is correctly incorporated throughout the codebase. These scripts will search for the property in JSON files as well as scan for references toTinyGridColumnin the consuming code and test suites:Please run these scripts and review the output to ensure that:
- The
"destructuring": trueflag is present where expected (e.g., in the JSON bundle under the npm object forTinyGridColumn).- Code and test files referencing
TinyGridColumnare correctly interpreting this new configuration option.Once you've verified that the new property is handled properly in the local import map logic and in all relevant areas, the review comment can be addressed accordingly.
🏁 Script executed:
#!/bin/bash # Search for the new "destructuring": true property in all files. rg '"destructuring":\s*true' -n . # Additionally, check for usage of "TinyGridColumn" to confirm that consuming code and tests are updated. rg -i 'TinyGridColumn' -n .Length of output: 19339
Action Required: Confirm Destructuring Import Behavior & Documentation Updates
The
destructuring: trueproperty forTinyGridColumnhas been verified in the JSON bundles and appears across multiple test cases and consuming code (e.g., within various componentsMap files and the designer-demo bundle). In addition, documentation (such as indocs/solutions/local-cdn.md) already includes references to this setting.Please ensure that:
- The local import map logic correctly interprets the new
destructuringproperty.- Consuming code (as evidenced in files like those in
/packages/vue-generator/test/) is updated to work with destructured imports.- Documentation clearly explains the purpose and usage of the
destructuring: trueconfiguration.Once you’ve confirmed that the import mapping logic fully supports this behavior during runtime, no further changes should be necessary.
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
319-388: Validate plugin ordering and error handlingThe main export
localCdnPlugin(lines 330–387) returns an array of plugins: environment overrides, package installation, and file copying. If one plugin fails, ensure proper error handling is in place so that subsequent plugins don’t proceed in a broken state. Consider usingapply: 'build'or other advanced plugin ordering features from Vite to ensure everything runs in the expected sequence.Would you like a script or instructions to verify the plugin order or confirm that error handling is robust across all plugin invocations?
designer-demo/tests/localCdnCustomConfig.test.js (1)
159-172:❓ Verification inconclusive
Verify environment variable substitution in final output
The test (lines 159–172) only checks one generated JS file (the first in the
assetsfolder). In multi-entry builds, or if code-splitting is used, environment variables might appear in other files. Consider verifying each.jschunk or searching forVITE_CDN_DOMAINusage across the entiredistfolder.
🏁 Script executed:
#!/bin/bash # Verify environment variable substitution in all built JS assets. fd -t f -e js ' dist ' --exec grep -Hn "VITE_CDN_DOMAIN:" {}Length of output: 62
Manual verification required for consistent environment variable substitution
The current test only checks the first generated JS file in
dist/assets. However, the substitution forVITE_CDN_DOMAIN:"./local-cdn-static"might occur in additional files, especially in multi-entry or code-split builds. The automated verification script (searching for"VITE_CDN_DOMAIN:"across thedistdirectory) did not produce any output, which isn’t conclusive. Please manually verify that all relevant JS chunks include the correct replacement. For example, consider running a recursive search like:grep -rHn 'VITE_CDN_DOMAIN:"./local-cdn-static"' distor updating the test to iterate over all
.jsfiles in the build folder.packages/canvas/DesignCanvas/src/importMap.ts (1)
36-36:✅ Verification successful
Verify the implications of empty importStyles array
The
importStylesarray has been emptied. Ensure this doesn't negatively impact style loading in the application.
🏁 Script executed:
#!/bin/bash # Check for references to importStyles in the codebase echo "Searching for importStyles references:" rg "importStyles" -A 3 -B 3 echo "Checking how styles are currently loaded in the application:" rg "canvasDeps.styles" -A 3 -B 3Length of output: 3717
Action Confirmed: Style Loading Works as Intended
The investigation confirms that the empty
importStylesinpackages/canvas/DesignCanvas/src/importMap.tsis intentional. It serves as the base value which is later merged withblockRequire.importStylesandcanvasDeps.styles—as seen in the computation:
- The merged array created via
[...blockRequire.importStyles, ...canvasDeps.styles]ensures that styles are still loaded correctly.- Usage in
DesignCanvas.vueviainitCanvas(importMap, importStyles)further verifies that style loading relies on the computedimportStyles.No negative impact on style loading has been observed. Please ensure that integration tests continue to verify that all expected styles load correctly in different environments.
| /** | ||
| * 对文件内容进行转换处理 | ||
| * @param {string} content - 文件内容 | ||
| * @param {string} filename - 文件名 | ||
| * @returns {string} - 处理后的内容 | ||
| */ | ||
| function transform(content, filename) { | ||
| if (filename.endsWith('.js')) { | ||
| const result = babelReplaceImportPathWithCertainFileName(content, filename, console) | ||
| return result.code || content | ||
| } | ||
| return content | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle non-JS files or binary assets carefully
The transform function only applies Babel transformations to .js files. However, it still reads all files with utf-8 encoding (see lines 144, 162). If the directory contains binary files (e.g., images or fonts), reading them as text might corrupt them. Consider adding file-type checks and skipping transformation or using a different read/write method for non-text files.
| /** | ||
| * 复制文件或目录到目标路径 | ||
| * @param {string} srcPath - 源文件/目录路径 | ||
| * @param {string[]} destPaths - 目标路径数组 | ||
| * @param {Set} copiedFiles - 已复制文件集合 | ||
| * @param {string} outDir - 输出目录 | ||
| */ | ||
| async function copyFileOrDirectory(srcPath, destPaths, copiedFiles, outDir) { | ||
| // 生成一个唯一标识，避免重复复制相同文件 | ||
| const copyId = `${srcPath}:${destPaths.join(',')}` | ||
|
|
||
| if (copiedFiles.has(copyId)) { | ||
| logger.log(`[vite-cdn-copy-plugin]: Skipping already copied file: ${srcPath}`) | ||
| return | ||
| } | ||
|
|
||
| copiedFiles.add(copyId) | ||
|
|
||
| // 检查源文件是否存在 | ||
| if (!fs.existsSync(srcPath)) { | ||
| logger.warn(`[vite-cdn-copy-plugin]: Source does not exist: ${srcPath}`) | ||
| return | ||
| } | ||
|
|
||
| const isDirectory = fs.statSync(srcPath).isDirectory() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid reading entire directories into memory for all file types
The current approach to copying directories (lines 122–151) reads each file in UTF-8 mode and applies transformations unconditionally. This can be memory-heavy and potentially break for large binary files. Consider an I/O streaming approach or limit content transformation to known text-based files.
| /** | ||
| * 比较两个版本号是否相同 | ||
| * @param {string} versionOrigin - 源版本号, 可能包含 ^ 或 ~ 开头 | ||
| * @param {string} versionTarget - 目标版本号,来源于 package.json 的 version, 不能包含 ^ 或 ~ 开头 | ||
| * @returns {boolean} - 是否相同 | ||
| */ | ||
| const compareIsSameVersion = (versionOrigin, versionTarget) => { | ||
| if (versionOrigin === versionTarget) { | ||
| return true | ||
| } | ||
|
|
||
| if (versionOrigin.startsWith('^')) { | ||
| // 如果源版本号是 ^ 开头，则只比较第一个数字是否相同 | ||
| return versionOrigin.slice(1, 2) === versionTarget.slice(0, 1) | ||
| } | ||
|
|
||
| if (versionOrigin.startsWith('~')) { | ||
| // 如果源版本号是 ~ 开头，则只比较前两个数字是否相同 | ||
| return versionOrigin.slice(1, 3) === versionTarget.slice(0, 3) | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance semantic version comparison
compareIsSameVersion (lines 235–251) only checks partial digits for ^ and ~ prefixes. This might lead to inaccuracies (e.g., ^1.2.3 vs. 1.10.0). Consider using a dedicated SemVer library or implement a more precise comparison to handle major/minor/patch more accurately.
| // 备份原始文件 | ||
| fs.copyFileSync(registryPath, registryPath + '.bak') | ||
|
|
||
| const registryContent = fs.readFileSync(registryPath, 'utf-8') | ||
|
|
||
| // 向 config 对象添加 importMap | ||
| const updatedContent = registryContent.replace( | ||
| /config: {([^}]*)}/, | ||
| `config: {$1, | ||
| importMap: { | ||
| imports: { | ||
| 'vue': "\${VITE_CDN_DOMAIN}/vue\${versionDelimiter}3.4.21\${fileDelimiter}/dist/vue.runtime.esm-browser.js" | ||
| } | ||
| } | ||
| }` | ||
| ) | ||
|
|
||
| fs.writeFileSync(registryPath, updatedContent) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use more robust detection or replacement logic in updateRegistryFile
Here, the registry file is patched with string replacements targeting config: { ... }. This can break if the user modifies the config structure or if the text config: { appears in a different context. Consider using a small parser or AST-based approach to reliably insert the importMap.
| describe('localCDN 功能测试', () => { | ||
| beforeAll(() => { | ||
| // 备份环境变量文件 | ||
| backupEnvFile(envAlphaPath) | ||
|
|
||
| // 确保环境变量正确设置 | ||
| let envContent = fs.readFileSync(envAlphaPath, 'utf-8') | ||
|
|
||
| // 确保关键环境变量已启用 | ||
| envContent = ensureEnvVarEnabled(envContent, 'VITE_LOCAL_IMPORT_MAPS') | ||
| envContent = ensureEnvVarEnabled(envContent, 'VITE_LOCAL_BUNDLE_DEPS') | ||
|
|
||
| // 写回更新后的环境变量 | ||
| fs.writeFileSync(envAlphaPath, envContent) | ||
|
|
||
| // 执行构建 | ||
| execSync('pnpm run build:alpha', { | ||
| cwd: projectRoot, | ||
| stdio: 'inherit' | ||
| }) | ||
| }) | ||
|
|
||
| // 测试结束后恢复原始环境变量 | ||
| afterAll(() => { | ||
| restoreEnvFile(envAlphaPath) | ||
| }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test verification for actual CDN functionality
The tests verify file creation and environment variable replacement, but don't validate if the local CDN actually works when the application runs. Consider adding a test that starts the application and checks if resources are correctly loaded from the local CDN.
You could add a test that:
- Starts a local server for the built application
- Makes an HTTP request to fetch an HTML page
- Parses the HTML to check if scripts point to local CDN paths
- Verifies that these local resources can be loaded
This would ensure end-to-end functionality of the local CDN feature.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
56-72: Enhance semantic version comparison.
Partial matching with^and~is too simplistic and can lead to inaccurate results, for instance matching^1.2with1.10.0. Consider using a dedicated semver library or implementing more precise comparison logic to align with SemVer best practices.
🧹 Nitpick comments (6)
packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js (2)
41-67: Consider formal error handling instead of ignoring.
The functionextractPackageInfois swallowing all errors inside a try/catch block with a simple// ignore. This can make it harder to diagnose certain issues, such as unexpected URL formats or regex failures. Consider at least logging the error or providing fallback handling to aid debugging.
79-80: Add a debug log when no match is found.
WhenextractPackageInforeturnsnull, the function simply returnsnullwithout any notice. A warning or debug log might help track down mismatches or typos in the URL paths, as these often cause confusion.packages/build/vite-config/src/vite-plugins/createEnvReplacementPlugin.js (1)
6-22: Add safety checks for undefined base or cdnDir.
If eitherbaseorcdnDiris undefined, this plugin will produce a malformed environment variable string. Consider adding fallback handling or logging to avoid potential build errors.packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (3)
1-5: Imports look correct; consider using async fast-glob if large directories are expected.
Your import statements are standard, and usingfs-extraprovides robust file operations. However, since you’re copying entire directories, consider switching to the async version offast-globin case of large-scale scenarios to improve performance and prevent blocking the event loop.
20-21: Console-based logging is fine, but consider a configurable logger.
Usingconsoleis acceptable for quick visibility. If you need varying log levels or file/JSON outputs, a dedicated logging library may provide more flexibility.
58-70: Synchronous directory traversal may degrade performance.
Within large projects,fg.sync()can block. Consider usingfg.async()or a streaming approach to avoid potential performance bottlenecks when copying numerous files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/README.md(1 hunks)packages/build/vite-config/src/localCdnFile/copyBundleDeps.js(2 hunks)packages/build/vite-config/src/localCdnFile/copyImportMap.js(1 hunks)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(1 hunks)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js(3 hunks)packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js(1 hunks)packages/build/vite-config/src/vite-plugins/createEnvReplacementPlugin.js(1 hunks)packages/common/js/importMap/import-map.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/build/vite-config/src/localCdnFile/copyImportMap.js
- packages/build/vite-config/src/localCdnFile/copyBundleDeps.js
- packages/common/js/importMap/import-map.json
🧰 Additional context used
🧬 Code Definitions (1)
packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (1)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (3)
logger(8-8)src(96-96)dest(123-123)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (6)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
170-173: Double-check immediate return on empty dependencies.
When no CDN dependencies are found, the plugin chain is effectively bypassed. Verify that this early return does not skip any required fallback logic in other parts of the build process.packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (5)
6-18: Validate Babel transformation approach for large file counts.
ThereplaceJsImportPathsfunction is straightforward, but if many.jsfiles are processed, Babel transformations may become time-consuming. Verify if an incremental or batched strategy better suits your performance requirements.
29-37: Skipping already-copied files is well-handled.
ThecopyIdguard effectively prevents redundant copy operations, which is a neat optimization feature.
40-44: Early return on missing source is appropriate.
Logging a warning and skipping non-existent sources avoids errors downstream. This is a good pattern for robust file operations.
108-110: Error logging doesn’t halt the build; confirm desired behavior.
When a copy fails, you log an error but allow subsequent copies to proceed. This might be acceptable if partial success is desired, but if a complete build is critical, consider surfacing the error back to Vite to fail the build.
114-163: Build-time plugin approach is well-structured; overall logic looks sound.
Your Vite plugin runs the copy inwriteBundle(), which is typically the right hook for build-time operations. Great job structuring it so that configuration and actual copying steps remain clearly separated and maintainable.
| } else { | ||
| // 如果是单个文件 | ||
| logger.log(`[vite-cdn-copy-plugin]: Copying file: ${srcPath} -> ${fullDestPath}`) | ||
|
|
||
| let finalDestPath = path.join(fullDestPath, path.basename(srcPath)) | ||
|
|
||
| // 确保目标文件的目录存在 | ||
| await fs.ensureDir(path.dirname(finalDestPath)) | ||
|
|
||
| // 读取文件内容 | ||
| const content = await fs.readFile(srcPath, 'utf-8') | ||
|
|
||
| // 应用转换 | ||
| const transformedContent = replaceJsImportPaths(content, srcPath) | ||
|
|
||
| // 写入转换后的内容 | ||
| await fs.writeFile(finalDestPath, transformedContent) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-examine single-file copy path logic.
Currently, the final destination is always path.join(fullDestPath, path.basename(srcPath)). If a user configures their dest to be an exact file name (e.g., dest: 'someFile.js'), they might be surprised that the basename of srcPath is still appended. Allowing flexible file renaming or clarifying it in the docs could help avoid confusion.
github-actions
bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
56-72: 🛠️ Refactor suggestionEnhance semantic version comparison for more robust version handling.
The current implementation of
compareIsSameVersionis overly simplistic for handling semantic versioning patterns:
- For
^versions, it only checks the first digit, which fails for versions ≥10 (e.g.,^10.0.0vs10.2.0).- For
~versions, it only checks the first two digits, which doesn't properly implement semver's tilde behavior.Consider using a dedicated semver library or implement a more accurate comparison that properly handles the full semver specification. For example:
-const compareIsSameVersion = (versionOrigin, versionTarget) => { - if (versionOrigin === versionTarget) { - return true - } - - if (versionOrigin.startsWith('^')) { - // 如果源版本号是 ^ 开头，则只比较第一个数字是否相同 - return versionOrigin.slice(1, 2) === versionTarget.slice(0, 1) - } - - if (versionOrigin.startsWith('~')) { - // 如果源版本号是 ~ 开头，则只比较前两个数字是否相同 - return versionOrigin.slice(1, 3) === versionTarget.slice(0, 3) - } - - return false -} +const compareIsSameVersion = (versionOrigin, versionTarget) => { + if (versionOrigin === versionTarget) { + return true + } + + // For caret ranges (^) + if (versionOrigin.startsWith('^')) { + const version = versionOrigin.slice(1) + const [major] = version.split('.') + const [targetMajor] = versionTarget.split('.') + + // For versions with major version 0, allow only exact patches + if (major === '0') { + const [, minor] = version.split('.') + const [, targetMinor] = versionTarget.split('.') + return major === targetMajor && minor === targetMinor + } + + // For regular versions, allow any minor/patch updates in the same major + return major === targetMajor + } + + // For tilde ranges (~) + if (versionOrigin.startsWith('~')) { + const version = versionOrigin.slice(1) + const [major, minor] = version.split('.') + const [targetMajor, targetMinor] = versionTarget.split('.') + + // Allow patch-level changes if a minor is specified + return major === targetMajor && minor === targetMinor + } + + return false +}
🧹 Nitpick comments (2)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (2)
97-107: Add error logging for package.json parsing failures.While you have a try-catch block for parsing package.json, you're silently ignoring any errors. It would be helpful to log these errors to assist with troubleshooting.
try { const pkg = JSON.parse(fs.readFileSync(path.resolve(pkgFilePath))) isSameVersion = compareIsSameVersion(version, pkg.version) } catch (error) { - // ignore + logger.debug(`[local-cdn-plugin]: Failed to parse package.json for ${packageName}:`, error) }
151-204: Main plugin implementation looks good, with opportunities for improvement.The
localCdnPluginfunction effectively manages local CDN dependencies by:
- Combining default and custom import map configurations
- Identifying packages that need installation
- Deduplicating copy operations
- Creating appropriate plugins for installation and copying
Consider adding more detailed error handling and progress logging, especially for the file copy operations that might fail due to permissions or disk space issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/build/vite-config/src/default-config.js(2 hunks)packages/build/vite-config/src/localCdnFile/import-map.js(1 hunks)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(1 hunks)packages/build/vite-config/src/vite-plugins/createDynamicImportMapPlugin.js(1 hunks)packages/common/js/importMap/index.js(1 hunks)packages/common/vite.config.ts(1 hunks)packages/design-core/vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/common/js/importMap/index.js
- packages/design-core/vite.config.js
🧰 Additional context used
🧬 Code Definitions (3)
packages/build/vite-config/src/localCdnFile/import-map.js (1)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
importMapConfig(157-157)
packages/build/vite-config/src/vite-plugins/createDynamicImportMapPlugin.js (2)
packages/build/vite-config/src/localCdnFile/import-map.js (2)
importMapConfig(1-33)importMapConfig(1-33)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
importMapConfig(157-157)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (3)
packages/build/vite-config/src/default-config.js (1)
logger(169-169)packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (2)
logger(20-20)copyPlugin(122-162)packages/build/vite-config/src/localCdnFile/import-map.js (2)
importMapConfig(1-33)importMapConfig(1-33)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (6)
packages/common/vite.config.ts (1)
69-70: External modules configuration looks good.You've added two patterns to the
externalarray:/^@babel.*/for babel-related packages and/^virtual:import-map$/for the virtual import map module. This correctly prevents these modules from being bundled and aligns with the local CDN functionality you're implementing.packages/build/vite-config/src/localCdnFile/import-map.js (1)
1-33: Well-structured module mapping for CDN dependencies.The
importMapConfigobject provides a clear mapping of module names to their CDN URLs with appropriate placeholders for dynamic values. The structure is clean and organized.packages/build/vite-config/src/vite-plugins/createDynamicImportMapPlugin.js (1)
10-36: Clean implementation of dynamic import map plugin.The plugin correctly handles virtual module resolution and conditional content generation based on the
isLocalImportMapflag. When local imports are used, it properly constructs the CDN domain and replaces placeholders with actual values.packages/build/vite-config/src/default-config.js (3)
13-13: Import statements updated to support new local CDN approachThe changes replace
copyPreviewImportMapwithlocalCdnPluginand add a new import forcreateDynamicImportMapPlugin, signaling a shift from a single-function approach to a more comprehensive plugin-based solution for local import maps.Also applies to: 17-17
167-181: Improved local CDN implementation with enhanced plugin supportThe new implementation properly initializes the local CDN plugin when
isLocalImportMapis true, passing necessary configuration parameters. This is a good improvement over the previous approach as it:
- Adds helpful logging for initialization
- Accepts custom configuration via
engineConfig.localCdnConfig- Only adds plugins when they're available (
cdnPlugins && cdnPlugins.length > 0)This addresses the "localImportMap not works" issue mentioned in the PR title by replacing the previous implementation with a more robust solution.
183-189:❓ Verification inconclusive
Added dynamic import map plugin to complement local CDN functionality
The addition of
createDynamicImportMapPluginis a good enhancement that complements the local CDN functionality. It's correctly configured with the base URL, CDN directory, and respects theisLocalImportMapflag.However, I notice that this plugin is added unconditionally, regardless of whether
isLocalImportMapis true or false. Since the plugin receives theisLocalImportMapparameter, it likely handles both cases internally, but it might be worth verifying if this is the intended behavior.
🏁 Script executed:
#!/bin/bash # Let's check the implementation of createDynamicImportMapPlugin to understand how it uses isLocalImportMap rg -A 5 "createDynamicImportMapPlugin" packages/build/vite-config/src/vite-plugins/Length of output: 790
Dynamic Import Map Plugin Behavior Verification
The
createDynamicImportMapPluginfunction is correctly configured to accept anisLocalImportMapparameter (defaulting tofalse), and it is added to the plugins array unconditionally. This appears to be an intentional design, with the expectation that the plugin's internal logic will handle both cases (i.e., whenisLocalImportMapistrueorfalse).
- Verified that the plugin function (in
packages/build/vite-config/src/vite-plugins/createDynamicImportMapPlugin.js) acceptsisLocalImportMapwith a default offalse.- No conditional guard was found in the plugin addition, implying the internal implementation should manage the flag appropriately.
Please double-check that the internal logic of the plugin behaves as expected when
isLocalImportMapisfalse. If the intended behavior is to conditionally add the plugin based on this flag, a guard may need to be added. Otherwise, if the plugin internally differentiates behavior, this implementation is correct.
packages/design-core/vite.config.js
Outdated
| @@ -62,7 +62,7 @@ export default defineConfig({ | |||
| } | |||
| } | |||
| }, | |||
| external: ['vue', 'monaco-editor', 'prettier', /@opentiny\/vue.*/, '@opentiny/tiny-engine-meta-register'] | |||
| external: ['vue', 'monaco-editor', 'prettier', /@opentiny\/vue.*/, /^@opentiny\/tiny-engine*/] | |||
There was a problem hiding this comment.
这个PR里为什么要把@opentiny/tiny-engine*依赖都排除呢
There was a problem hiding this comment.
因为 common 中使用了虚拟模块 virtual:import-map，在 design-core 打包的时候，无法提前得到真正的 import-map 模块内容。所以需要做一个 external。
| @@ -6,6 +6,7 @@ yarn.lock | |||
| pnpm-lock.yaml | |||
| lerna-debug.log | |||
| packages/design-core/bundle-deps | |||
| designer-demo/bundle-deps | |||
| ) | ||
| const cdnFiles = extraBundleCdnLink(bundleFile, originCdnPrefix) | ||
| .map((url) => getCdnPathNpmInfoForSingleFile(url, originCdnPrefix, base, dir, false, bundleTempDir)) | ||
| // 比如 url 前缀跟 originCdnPrefix 不匹配，或者 url 格式不正确 的场景有可能会导致匹配失败 |
There was a problem hiding this comment.
理论上不会出现前缀跟originCdnPrefix不匹配场景，可以再斟酌一下
There was a problem hiding this comment.
用户有可能内部打包的物料和官方物料混合在一起：
{
"packages": [
{
"name": "自定打包的组件库",
"package": "美好出行-xxxxx-component",
"version": "3.20.0",
"destructuring": true,
"script": "https://192.168.1.10/xxxx.mjs",
"css": "https://192.168.1.10/xxxx.css"
},
{
"name": "TinyVue组件库",
"package": "@opentiny/vue",
"version": "3.20.0",
"destructuring": true,
"script": "https://registry.npmmirror.com/@opentiny/vue-runtime/~3.20/files/dist3/tiny-vue-pc.mjs",
"css": "https://registry.npmmirror.com/@opentiny/vue-theme/~3.20/files/index.css"
},
{
"name": "element-plus组件库",
"package": "element-plus",
"version": "2.4.2",
"script": "https://registry.npmmirror.com/element-plus/2.4.2/files/dist/index.full.mjs",
"css": "https://registry.npmmirror.com/element-plus/2.4.2/files/dist/index.css"
}
]
}There was a problem hiding this comment.
copyBundleDeps.js L34已经判断果是要替换的前缀的了？
| @@ -0,0 +1,33 @@ | |||
| export const importMapConfig = { | |||
There was a problem hiding this comment.
这个文件暂时不明白为啥在build vite插件里面 这个应该属于业务内容，应该通过传参进来，与插件要解耦
There was a problem hiding this comment.
背景：
- 画布（@opentiny/tiny-engine-canvas） 需要 import-map
- 页面预览 (preview) 需要 import-map
- 上述两个地方依赖的 import-map 大致重合（vue、vue-i18n、物料后续会逐步解耦出去）
我们需要 import-map 本地化：所以我们拿到将上述两个地方的 import-map 依赖，将依赖提前打包到产物，然后让原来的 import-map 拿到本地化之后的域名
原来的方案：
- 将 import-map.json 放置到 common，让画布和页面预览依赖读取。
- import-map 的域名
${VITE_CDN_DOMAIN}保持字符串。 - 本地化打包之后，改写环境变量
${VITE_CDN_DOMAIN}，在运行时将${VITE_CDN_DOMAIN}字符串替换成环境变量的值。（就是插件包不感知是不是 localCDN，他们只关心环境变量的值）
新方案：
改动 commit: e9b75c4 (#1242)
改动原因：#1242 (comment)
方案改动点：
- 如果不能改环境变量
${VITE_CDN_DOMAIN}的值，那么我们需要将真正的 localCDN 相对路径写入到真正的模块中。
使用虚拟模块的原因
如果不使用虚拟模块，无法在二开工程打包的时候，将真正的 localCDN 相对路径写入到真正的模块中。
使用虚拟模块后
如果开启了 CDN 本地化，则将替换后的内容该写成模块内容，再参与真正的打包。
如果没有开启 CDN 本地化，则将原样的内容写入模块内容，参与真正的打包。
如果有更好的方案，欢迎提出来。
|
|
||
| const logger = console | ||
|
|
||
| // 默认的复制配置，这几个 package 需要复制整个目录，所以需要有默认配置进行复制整个目录 |
There was a problem hiding this comment.
这个配置应该从应用层传参数进来 vite插件不要内置业务会比较好
There was a problem hiding this comment.
当前 tiny-vue 物料还是内置的，需要解耦 tiny-vue 物料之后，再将此部分内置逻辑解耦出来。
| * @param {string} versionTarget - 目标版本号,来源于 package.json 的 version, 不能包含 ^ 或 ~ 开头 | ||
| * @returns {boolean} - 是否相同 | ||
| */ | ||
| const compareIsSameVersion = (versionOrigin, versionTarget) => { |
There was a problem hiding this comment.
此处属于简单实现， 可以抽取单个文件为semver版本号字符串的相等compare，作为utils单独一个文件， 或者更建议看看有无三方库直接使用，把逻辑关注点分离
There was a problem hiding this comment.
这里的相等比较其实不是严格的相等比较。
这里是为了避免重复下载依赖包做的比较，比如我的 cdn 指定版本为：^3.20.0
那么，只要我的 node_module 里面的 package.json 的版本是大版本号相同就可以。
如果指定版本为 ~3.20.0，那么只要我的 node_module 里面的 package.json 的版本是 3.20.x 就可以认为相同。
所以我理解不是严格比较的通用逻辑，后续如果其他插件或者业务也有类似的逻辑，可以抽取成一个通用的 utils。
| return versionOrigin.slice(1, 3) === versionTarget.slice(0, 3) | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
compare函数需要返回 0 1 或者-1
is函数返回true false，
需要修改一下函数名明确
There was a problem hiding this comment.
这里没懂为什么 compare 函数需要返回 0、1 或者 -1，是哪里的规范吗？
| } | ||
| } | ||
|
|
||
| config.plugins.push( |
There was a problem hiding this comment.
已经再包一层插件的话，插件应该一体化， L183-L88应该放到 168的if分支块内，甚至应该放到localCdnPlugin内
There was a problem hiding this comment.
这里不能放置到 168 行的 if 分支条件块内部。理由详见：#1242 (comment)
因为如果不是 localImportMap 了，也需映射虚拟模块。
| return `export default ${newContent}` | ||
| } | ||
|
|
||
| return `export default ${JSON.stringify(importMapConfig)}` |
There was a problem hiding this comment.
这里的两个条件分支的处理层级有点不一样， 待进一步分析
There was a problem hiding this comment.
处理的层级不一样是什么意思？这个插件是决定终态的虚拟模块的内容的。
| const versionDelimiter = VITE_CDN_TYPE === 'npmmirror' ? '/' : '@' | ||
| const fileDelimiter = VITE_CDN_TYPE === 'npmmirror' ? '/files' : '' | ||
|
|
||
| if (customImportMap?.imports?.[pkgName]) { | ||
| return customImportMap.imports[pkgName] | ||
| .replace('${VITE_CDN_DOMAIN}', VITE_CDN_DOMAIN) |
There was a problem hiding this comment.
这里实际分为两种场景：
-
localCDN 的场景：
由于 fix: localImportMap not works #1242 (comment) 的 意见提出，localCDN 的场景会将实际的${VITE_CDN_DOMAIN}在构建态实际写入。使得VITE_CDN_DOMAIN环境变量失效。 -
非 localCDN 的场景：
当前仍然保留了 VITE_CDN_DOMAIN 等环境变量的运行时读取和替换
如果 localCDN 和非 localCDN 的场景都将 import-map 的 CDN 等变量直接在构建态解决的话，我理解就不需要配置到 .env 环境变量了。应该作为构建配置项存在。
| `${VITE_CDN_DOMAIN}/@opentiny/vue-theme${versionDelimiter}${importMapVersions.tinyVue}${fileDelimiter}/index.css`, | ||
| `${VITE_CDN_DOMAIN}/element-plus${versionDelimiter}2.4.2${fileDelimiter}/dist/index.css` | ||
| ] | ||
| importStyles: [] |
There was a problem hiding this comment.
style理论上应该和imports 同个时间清理，应该是一对一绑定的
packages/common/vite.config.ts
Outdated
| @@ -66,7 +66,8 @@ export default defineConfig({ | |||
| /@opentiny\/tiny-engine.*/, | |||
| /@opentiny\/vue.*/, | |||
| /^prettier.*/, | |||
| /^@babel.*/ | |||
| /^@babel.*/, | |||
| /^virtual:import-map$/ | |||
There was a problem hiding this comment.
是否需要分包，可以参考 #1242 (comment) 的背景讨论
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| import { useEnv } from '@opentiny/tiny-engine-meta-register' | |||
| import { getSearchParams } from './http' | |||
| import importMapJSON from './importMap.json' | |||
| import { importMapConfig as importMapJSON } from '@opentiny/tiny-engine-common/js/importMap/index' | |||
There was a problem hiding this comment.
这个路径过于具体（有耦合倾向，需要维持api稳定），common包理论上 只暴露到js目录这一层
| : [] | ||
| ) | ||
|
|
||
| // 添加本地化CDN插件支持 | ||
| if (isLocalImportMap) { |
There was a problem hiding this comment.
如果要改变写法， 需要把isCopyBundleDeps一起改了，保持工整对称
| const logger = console | ||
| logger.log('[local-cdn-plugin]: Initializing local CDN plugin') | ||
|
|
||
| const cdnPlugins = localCdnPlugin({ |
There was a problem hiding this comment.
这个插件似乎只是处理preview预览？ 因为上面的copyBundleDeps是处理画布物料的， 需要考虑合适的名字
rename transform function to replaceJsImportPaths to improve code readability and semantics. The new name more clearly expresses the actual functionality of the function: replacing import paths in JS files.
提取独立的插件 cdnCopyPlugin、createEnvReplacementPlugin 到 vite-plugin 文件夹中。
chilingling
force-pushed
the
feat/localCDNPlugin
branch
from
83e92c8 to
c1894bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
56-72: 💡 Verification agent🧩 Analysis chain
Enhance semantic version comparison logic
The current implementation of
compareIsSameVersiondoesn't fully comply with semver rules:
- For caret ranges (
^), it only checks the first digit, which fails with multi-digit major versions like^10.2.3- For tilde ranges (
~), it only compares the first two digits, which doesn't properly handle minor versions with multiple digits like~1.10.0This simplified approach could lead to incorrect matches or miss breaking changes.
Consider using a dedicated semver library or implementing a more complete comparison:
-const compareIsSameVersion = (versionOrigin, versionTarget) => { - if (versionOrigin === versionTarget) { - return true - } - - if (versionOrigin.startsWith('^')) { - // 如果源版本号是 ^ 开头，则只比较第一个数字是否相同 - return versionOrigin.slice(1, 2) === versionTarget.slice(0, 1) - } - - if (versionOrigin.startsWith('~')) { - // 如果源版本号是 ~ 开头，则只比较前两个数字是否相同 - return versionOrigin.slice(1, 3) === versionTarget.slice(0, 3) - } - - return false -} +const compareIsSameVersion = (versionOrigin, versionTarget) => { + if (versionOrigin === versionTarget) { + return true + } + + // Extract semver parts + let cleanVersion = versionOrigin + if (versionOrigin.startsWith('^') || versionOrigin.startsWith('~')) { + cleanVersion = versionOrigin.slice(1) + } + + const [originMajor, originMinor] = cleanVersion.split('.').map(Number) + const [targetMajor, targetMinor] = versionTarget.split('.').map(Number) + + if (versionOrigin.startsWith('^')) { + // For caret ranges, major version must match + return originMajor === targetMajor + } + + if (versionOrigin.startsWith('~')) { + // For tilde ranges, major and minor must match + return originMajor === targetMajor && originMinor === targetMinor + } + + return false +}Alternatively, you could use the semver npm package:
+import semver from 'semver' + const compareIsSameVersion = (versionOrigin, versionTarget) => { if (versionOrigin === versionTarget) { return true } - if (versionOrigin.startsWith('^')) { - // 如果源版本号是 ^ 开头，则只比较第一个数字是否相同 - return versionOrigin.slice(1, 2) === versionTarget.slice(0, 1) - } - - if (versionOrigin.startsWith('~')) { - // 如果源版本号是 ~ 开头，则只比较前两个数字是否相同 - return versionOrigin.slice(1, 3) === versionTarget.slice(0, 3) - } + try { + return semver.satisfies(versionTarget, versionOrigin) + } catch (e) { + logger.warn(`[local-cdn-plugin]: Invalid semver comparison between ${versionOrigin} and ${versionTarget}`) + } return false }
🌐 Web query:
What is the proper way to compare SemVer versions with caret (^) and tilde (~) ranges in JavaScript?💡 Result:
Comparing SemVer Versions with Caret (^) and Tilde (~) Ranges in JavaScript
To properly compare Semantic Versioning (SemVer) versions in JavaScript—especially in the context of caret (^) and tilde (~) constraints—it is essential to understand both the versioning rules and the available tooling.
Understanding Caret (^) and Tilde (~) Ranges
- Tilde (~) Range:
Permits updating to all patch versions as long as the major and minor versions remain the same.
Example:~1.2.3matches>=1.2.3 <1.3.0, allowing updates like1.2.4,1.2.9, etc., but not1.3.0[6][7][9].- Caret (^) Range:
Allows updating to all newer patch and minor versions, as long as the major version is unchanged (i.e., no breaking changes).
Example:^1.2.3matches>=1.2.3 <2.0.0, so it will upgrade to any1.x.xversion, like1.4.0or1.9.9, but not2.0.0[6][7][9].
- For versions
<1.0.0, the caret is more restrictive: for^0.1.2, it behaves like~0.1.2, matching>=0.1.2 <0.2.0[6].
Proper Way to Compare SemVer Versions in JavaScript
1. Use a SemVer Library
The most robust approach is to use a library that understands version ranges and performs checks according to the SemVer specification.
npm's
semverpackage:
This is the industry standard for version comparison, validation, and range satisfaction.
Example:const semver = require('semver'); // Check if a version satisfies a caret or tilde range semver.satisfies('1.2.5', '^1.2.3'); // true semver.satisfies('1.3.0', '~1.2.3'); // false
satisfies(version, range)verifies if a given version matches the specified range, handling all the complexity of^and~[6].Other lightweight libraries:
semver-compare: Compares two versions and tells you which is higher, but does not handle range logic such as caret or tilde[2][5].compare-versions: Can compare two versions, sort lists, and check for equality, but to interpret^and~, you must parse the constraint logic yourself[1][5].2. How to Compare with Caret and Tilde
If you want to check if a version (e.g.,
1.2.4) is acceptable for a range (e.g.,^1.2.3or~1.2.3):
With
semver:semver.satisfies('1.2.4', '^1.2.3'); // true semver.satisfies('1.2.4', '~1.2.3'); // true semver.satisfies('1.3.0', '^1.2.3'); // true semver.satisfies('2.0.0', '^1.2.3'); // falseFor sorting or direct comparison (not range checks):
// semver-compare or compare-versions const compareVersions = require('compare-versions'); compareVersions.compare('1.2.4', '1.2.3', '>'); // true compareVersions.compare('1.2.4', '1.2.3', '='); // falseBut to check range compliance, prefer
semver.satisfies()[6][5].
Summary Table
Task Recommended Tool / Method Example Usage Check if version fits ^/~rangesemver.satisfies(version, range)semver.satisfies('1.4.2', '^1.2.3') // trueCompare two versions (which is newer) semver.compare,semver-comparesemver.compare('1.2.3', '1.2.4') // -1Sort list of versions compare-versions,semver.sortversions.sort(compareVersions)
Key Points
- Use
semver.satisfies()to evaluate if a version matches a caret or tilde range in JavaScript[6][7].- Tilde (
~) is patch-flexible; caret (^) is patch- and minor-flexible (major version must match)[6][7][9].- Lightweight tools (like
semver-compare) can directly compare versions, but don't handle range expressions like^and~[2][5].For all caret and tilde comparison logic, the npm
semverlibrary is the standard and correct choice in JavaScript projects.Citations:
- 1: https://www.npmjs.com/package/compare-versions
- 2: https://www.npmjs.com/package/semver-compare
- 3: https://gist.github.com/iwill/a83038623ba4fef6abb9efca87ae9ccb
- 4: https://semver.org
- 5: https://npm-compare.com/compare-version,compare-versions,node-version,semver,semver-compare
- 6: http://nodesource.com/blog/semver-tilde-and-caret/
- 7: https://www.geeksforgeeks.org/difference-between-tilde-and-caret-in-package-json/
- 8: https://github.com/jceb/semver-compare-cli
- 9: https://sentry.io/answers/node-difference-between-tilde-caret/
- 10: https://portal.ecg.co/guides/alpaca-cli/5.3/semver
🏁 Script executed:
#!/bin/bash set -e # Check for semver in package.json dependencies across the repo echo "Searching for semver in package.json files..." rg -n '"semver"\s*:' --glob "package.json" # Search for semver imports or requires in the codebase echo -e "\nSearching for semver imports or requires..." rg -n -e "import semver" -e "require('semver')" . # Display the current implementation of compareIsSameVersion echo -e "\nShowing compareIsSameVersion implementation..." sed -n '1,200p' packages/build/vite-config/src/localCdnFile/localCdnPlugin.js || trueLength of output: 152
🏁 Script executed:
#!/bin/bash set -e # Locate localCdnPlugin.js in the repository echo "Locating localCdnPlugin.js..." rg -l "compareIsSameVersion" --glob "*.js" # If found, display the snippet around compareIsSameVersion echo -e "\nShowing compareIsSameVersion implementation (±5 lines)..." rg -n -C5 "compareIsSameVersion" --glob "*.js" || true # Search all package.json files for a semver dependency echo -e "\nSearching for semver in package.json files..." rg -n '"semver"\s*:' --glob "**/package.json"Length of output: 2598
Enhance version comparison to handle full SemVer ranges
The current
compareIsSameVersionin
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(lines 56–72)
uses simple string slicing which fails on multi-digit majors/minors (e.g.^10.2.3,~1.10.0). This can cause mismatches or overlook allowed patch/minor updates.Please update as follows:
File:
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(56–72)Option A: Use the industry-standard
semverpackage
- Add to your
package.json(e.g.devDependencies):
"semver": "^7.5.6"- Replace the function with:
+ import semver from 'semver' const compareIsSameVersion = (versionOrigin, versionTarget) => { if (versionOrigin === versionTarget) { return true } + try { + // semver.satisfies handles both ^ and ~ correctly + return semver.satisfies(versionTarget, versionOrigin) + } catch (e) { + console.warn(`[local-cdn-plugin] Invalid semver range "${versionOrigin}"`) + } return false }Option B: Improve manual parsing without external deps
const compareIsSameVersion = (versionOrigin, versionTarget) => { if (versionOrigin === versionTarget) { return true } + // strip range marker and parse numbers + const clean = versionOrigin.replace(/^[~^]/, '') + const [oMajor, oMinor] = clean.split('.').map(Number) + const [tMajor, tMinor] = versionTarget.split('.').map(Number) if (versionOrigin.startsWith('^')) { // major must match + return oMajor === tMajor } if (versionOrigin.startsWith('~')) { // major & minor must match + return oMajor === tMajor && oMinor === tMinor } return false }Either approach will correctly handle multi-digit SemVer ranges and prevent subtle mismatches.
designer-demo/tests/localCdnCustomConfig.test.js (1)
30-48: 🛠️ Refactor suggestionUse more robust detection or replacement logic in
updateRegistryFileThe current regex-based approach to modify the registry file is fragile and could break if:
- The config structure changes
- The file contains multiple objects with similar patterns
- The config object contains nested objects with braces
Consider using a more robust approach such as an AST-based modification.
A simple improvement would be to use a more specific regex pattern:
- const updatedContent = registryContent.replace( - /config: {([^}]*)}/, - `config: {$1, - importMap: { - imports: { - 'vue': "\${VITE_CDN_DOMAIN}/vue\${versionDelimiter}3.4.21\${fileDelimiter}/dist/vue.runtime.esm-browser.js" - } - } - }` - ) + // Look specifically for a config object with id: 'engine.config' + const updatedContent = registryContent.replace( + /config:\s*{[^{]*id:\s*['"]engine\.config['"][^}]*}/, + (match) => { + // Check if importMap already exists + if (match.includes('importMap:')) { + return match; + } + // Add importMap before the closing brace + return match.replace( + /}$/, + `, + importMap: { + imports: { + 'vue': "\${VITE_CDN_DOMAIN}/vue\${versionDelimiter}3.4.21\${fileDelimiter}/dist/vue.runtime.esm-browser.js" + } + } + }` + ); + } + )For a more robust solution, consider using a JavaScript parser like Acorn or Babel to parse, modify, and regenerate the file.
packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (1)
92-93: Single-file copy always appends basename – reconsider UX
The plugin still forcespath.basename(srcPath)onto the destination, preventing legitimate renames (e.g.,{ src: 'foo.js', dest: 'bar.js' }). This was raised in a previous review.
🧹 Nitpick comments (9)
packages/build/vite-config/src/default-config.js (1)
167-181: Consider adding explicit error handling for plugin initializationWhile checking if
cdnPluginsexists and has items is good, there's no explicit warning ifcdnPluginsis empty whenisLocalImportMapis true. This might make debugging difficult if plugin initialization silently fails.if (isLocalImportMap) { const logger = console logger.log('[local-cdn-plugin]: Initializing local CDN plugin') const cdnPlugins = localCdnPlugin({ localCdnConfig: engineConfig.localCdnConfig, base: getBaseUrlFromCli(config.base), cdnDir: 'local-cdn-static' }) if (cdnPlugins && cdnPlugins.length > 0) { config.plugins.push(...cdnPlugins) + } else { + logger.warn('[local-cdn-plugin]: No plugins were returned from localCdnPlugin, local CDN may not work correctly') } }packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (4)
100-107: Improve error handling in package.json parsingThe current code silently ignores any errors when reading or parsing package.json. While this prevents the process from failing, it makes debugging difficult if there are issues with package.json files.
if (fs.existsSync(pkgFilePath)) { try { const pkg = JSON.parse(fs.readFileSync(path.resolve(pkgFilePath))) isSameVersion = compareIsSameVersion(version, pkg.version) } catch (error) { - // ignore + logger.debug(`[local-cdn-plugin]: Error reading or parsing package.json for ${packageName}: ${error.message}`) } }
34-48: The extractInfo function could fail silently in some edge casesWhen extractInfo fails to parse a string, it logs an error but doesn't return a value, which could lead to undefined items in the parsed arrays later. Consider returning a default object or throwing an error to better handle these cases.
function extractInfo(str) { try { let [packageName, versionAndPath] = str.split('${versionDelimiter}') packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '') const [version, filePath] = versionAndPath.split('${fileDelimiter}') return { packageName, version, filePathInPackage: filePath || '/' } } catch (error) { logger.error(`[local-cdn-plugin]: Failed to extract info from ${str} 提取 importMap 信息失败`, error) + // Return a recognizable error object so we can filter it out later + return { error: true, originalString: str } } }Then update where it's used:
- const parsedDefaultImportMapConfig = Object.values(defaultImportMapConfig.imports).map((item) => extractInfo(item)) - const parsedImportMapConfig = Object.values(importMapConfig.imports).map((item) => extractInfo(item)) + const parsedDefaultImportMapConfig = Object.values(defaultImportMapConfig.imports) + .map((item) => extractInfo(item)) + .filter(item => !item.error) + const parsedImportMapConfig = Object.values(importMapConfig.imports) + .map((item) => extractInfo(item)) + .filter(item => !item.error)
10-24: Consider moving the default copy configuration to a separate fileHardcoding the default copy configuration in the plugin makes it less maintainable and harder to customize. Consider extracting it to a separate configuration file.
-// 默认的复制配置，这几个 package 需要复制整个目录，所以需要有默认配置进行复制整个目录 -const defaultCopyConfig = { - '@opentiny/vue-theme': { - filePathInPackage: '/' - }, - '@opentiny/vue-renderless': { - filePathInPackage: '/' - }, - '@opentiny/vue-runtime': { - filePathInPackage: '/dist3/' - }, - '@vue/devtools-api': { - filePathInPackage: '/' - } -} +import { defaultCopyConfig } from './default-copy-config.js'And create a new file
default-copy-config.js:/** * Default copy configuration for packages that need their entire directory copied */ export const defaultCopyConfig = { '@opentiny/vue-theme': { filePathInPackage: '/' }, '@opentiny/vue-renderless': { filePathInPackage: '/' }, '@opentiny/vue-runtime': { filePathInPackage: '/dist3/' }, '@vue/devtools-api': { filePathInPackage: '/' } }
162-164: Filter out packages that may have undefined or error valuesIf any import map string fails to parse in
extractInfo, it might return undefined or an error object, which could cause errors later in the process chain. Ensure that such items are filtered out.const overriddenImportMap = parsedDefaultImportMapConfig.filter((item) => { - return !parsedImportMapConfig.find((parsedItem) => parsedItem.packageName === item.packageName) + // First ensure item is valid + if (!item || !item.packageName) return false + // Then check if it's not overridden by a user-provided import + return !parsedImportMapConfig.find((parsedItem) => + parsedItem && parsedItem.packageName === item.packageName) })designer-demo/tests/localCdnCustomConfig.test.js (1)
144-148: Make file checks more robust with path normalizationWhen checking for file existence, consider normalizing paths to handle platform-specific differences in path separators.
// Check if vue.global.prod.js file exists const vueJsExists = vueDirs.some(dir => { - return fs.existsSync(path.resolve(dir, 'dist', 'vue.runtime.esm-browser.js')) + return fs.existsSync(path.normalize(path.resolve(dir, 'dist', 'vue.runtime.esm-browser.js'))) })docs/solutions/local-cdn.md (1)
73-87: Provide a more complete example of registry configurationWhile you explain the need to synchronize importMap configuration with the registry, a complete code example showing how to access and share the importMap between Vite config and registry would be helpful.
**重要说明**：如果您在 Vite 配置中传递了 `localCdnConfig.importMap`，还需要在 registry 注册表的 config 中传入同样的配置，以确保应用在运行时能正确读取自定义的 importMap 配置。例如： ```javascript +// 在 vite.config.js 中 +const localCdnConfig = { + importMap: { + imports: { + 'vue': "${VITE_CDN_DOMAIN}/vue${versionDelimiter}3.4.21${fileDelimiter}/dist/vue.runtime.esm-browser.js" + } + }, + copy: { ... } +}; + +const baseConfig = useTinyEngineBaseConfig({ + localCdnConfig, + // ...otherConfig +}) + +// 在其他地方导出 localCdnConfig 以便在 registry.js 中使用 +export { localCdnConfig }; + +// 在 registry.js 中 +import { localCdnConfig } from './vite.config.js'; + // 在注册表配置中 { config: { id: 'engine.config', importMap: localCdnConfig.importMap, // ... 其他配置 } }packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (2)
20-20: Leverage Vite’s logger for consistent output
Using the rawconsolebypasses Vite’s coloured, level-controlled logging.
Consider:-import { createLogger } from 'vite' -const logger = createLogger() +import { createLogger } from 'vite' +const logger = createLogger()This keeps plugin output aligned with the rest of the build toolchain and honours
--silent/logLevelflags.
46-47: Avoid blocking sync I/O inside async function
fs.statSyncblocks the event loop during potentially large directory scans. Usingawait fs.stat(...)(async) keeps the build responsive, especially when many targets are configured.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.gitignore(1 hunks)designer-demo/env/.env.alpha(1 hunks)designer-demo/env/.env.development(0 hunks)designer-demo/package.json(2 hunks)designer-demo/public/mock/bundle.json(1 hunks)designer-demo/tests/localCdnBasic.test.js(1 hunks)designer-demo/tests/localCdnBundleDeps.test.js(1 hunks)designer-demo/tests/localCdnCustomConfig.test.js(1 hunks)designer-demo/tests/utils/envHelpers.js(1 hunks)designer-demo/vitest.config.js(1 hunks)docs/README.md(1 hunks)docs/catalog.json(1 hunks)docs/solutions/local-cdn.md(1 hunks)packages/build/vite-config/package.json(1 hunks)packages/build/vite-config/src/default-config.js(2 hunks)packages/build/vite-config/src/localCdnFile/copyBundleDeps.js(3 hunks)packages/build/vite-config/src/localCdnFile/copyImportMap.js(1 hunks)packages/build/vite-config/src/localCdnFile/import-map.js(1 hunks)packages/build/vite-config/src/localCdnFile/index.js(1 hunks)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(1 hunks)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js(3 hunks)packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs(2 hunks)packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js(1 hunks)packages/build/vite-config/src/vite-plugins/createDynamicImportMapPlugin.js(1 hunks)packages/canvas/DesignCanvas/src/DesignCanvas.vue(1 hunks)packages/canvas/DesignCanvas/src/importMap.ts(2 hunks)packages/common/js/importMap/index.js(1 hunks)packages/common/package.json(1 hunks)packages/common/vite.config.ts(1 hunks)packages/design-core/package.json(1 hunks)packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/design-core/src/preview/src/preview/importMap.js(1 hunks)packages/design-core/src/preview/src/preview/importMap.json(0 hunks)packages/design-core/vite.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- designer-demo/env/.env.development
- packages/design-core/src/preview/src/preview/importMap.json
✅ Files skipped from review due to trivial changes (2)
- designer-demo/env/.env.alpha
- designer-demo/package.json
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/canvas/DesignCanvas/src/DesignCanvas.vue
- packages/common/js/importMap/index.js
- .gitignore
- packages/build/vite-config/src/localCdnFile/index.js
- designer-demo/vitest.config.js
- packages/common/vite.config.ts
- docs/README.md
- packages/build/vite-config/package.json
- packages/common/package.json
- packages/design-core/package.json
- docs/catalog.json
- packages/build/vite-config/src/localCdnFile/copyImportMap.js
- packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs
- packages/build/vite-config/src/localCdnFile/import-map.js
- packages/design-core/vite.config.js
- packages/canvas/DesignCanvas/src/importMap.ts
- designer-demo/tests/localCdnBundleDeps.test.js
- packages/build/vite-config/src/localCdnFile/copyBundleDeps.js
- packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js
- designer-demo/tests/localCdnBasic.test.js
- packages/build/vite-config/src/vite-plugins/createDynamicImportMapPlugin.js
- packages/design-core/src/preview/src/preview/importMap.js
- packages/design-core/src/preview/src/preview/Preview.vue
- designer-demo/public/mock/bundle.json
- designer-demo/tests/utils/envHelpers.js
🧰 Additional context used
🧬 Code Graph Analysis (2)
designer-demo/tests/localCdnCustomConfig.test.js (3)
designer-demo/tests/localCdnBasic.test.js (5)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(15-15)localCdnDir(16-16)designer-demo/tests/localCdnBundleDeps.test.js (4)
__dirname(13-13)projectRoot(14-14)envAlphaPath(17-17)distDir(18-18)designer-demo/tests/utils/envHelpers.js (5)
backupPath(53-53)backupPath(64-64)backupEnvFile(51-56)ensureEnvVarEnabled(12-27)restoreEnvFile(63-69)
packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (1)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (2)
logger(8-8)src(96-96)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (10)
packages/build/vite-config/src/default-config.js (2)
167-191: The integration of the local CDN plugin looks good.The plugin architecture is well-designed, conditionally initializing the local CDN plugin when
isLocalImportMapis true, and always adding the dynamic import map plugin. This approach allows for flexible CDN dependency handling based on environment configuration.
183-189: The createDynamicImportMapPlugin is always included regardless of whether local import maps are enabled.This is a good approach as it ensures the import map handling is always available, with its behavior controlled by the
isLocalImportMapparameter. This separation of concerns makes the code more maintainable.designer-demo/tests/localCdnCustomConfig.test.js (2)
126-157: The test correctly validates custom importMap configurationThe test effectively verifies that:
- Vue directories are created in the local CDN directory
- The dist directory exists within the Vue directory
- The expected Vue runtime file exists
- Multiple files are copied as expected
This comprehensive check ensures the custom configuration is properly applied.
159-172: Good validation of environment variable replacementThe test correctly verifies that the CDN domain is replaced with the local path in the built JavaScript files, which is a critical aspect of the local CDN functionality.
docs/solutions/local-cdn.md (5)
1-10: Excellent overview of the local CDN solutionThe documentation clearly explains the purpose and benefits of the local CDN solution, focusing on application reliability, offline/intranet usage, and performance improvements.
90-125: Clear explanation of copy configurationThe documentation provides a clear explanation of the copy configuration options, including the default configuration and examples of custom configurations. This is particularly useful for users who need to customize the copy behavior for specific packages.
131-164: Good explanation of bundle handling with clear examplesThe documentation explains how the local CDN handles bundle files, including the important note about the CDN domain prefix matching. The example with both TinyVue and Element Plus clearly illustrates which packages will be processed.
167-202: Comprehensive explanation of implementation principlesThe documentation provides a clear explanation of how the local CDN solution works, breaking it down into logical steps. This gives users a good understanding of what happens under the hood.
203-207: Important notes for usersThe notes section provides critical information about ensuring package versions are defined, acknowledging the increased build size, and handling special CDN URL formats. This helps users avoid common pitfalls when using the local CDN solution.
packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (1)
124-131: ResetcopiedFilesbetween builds for watch mode
When Vite runs in--watch/ serve mode, a second build in the same process will reuse the staleSet, causing legitimate copies to be skipped. Clear the set inbuildStartor eachwriteBundle.
| // 执行构建 | ||
| execSync('pnpm run build:alpha', { | ||
| cwd: projectRoot, | ||
| stdio: 'inherit' | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for build command execution
The current implementation runs the build command synchronously and will throw an error if the command fails, potentially leaving the test environment in an inconsistent state. Consider adding a try-catch block to ensure cleanup happens even if the build fails.
- // Execute build
- execSync('pnpm run build:alpha', {
- cwd: projectRoot,
- stdio: 'inherit'
- })
+ // Execute build
+ try {
+ execSync('pnpm run build:alpha', {
+ cwd: projectRoot,
+ stdio: 'inherit'
+ })
+ } catch (error) {
+ console.error('Build failed:', error.message)
+ // Clean up even if the build fails
+ restoreEnvFile(envAlphaPath)
+ restoreRegistryFile()
+ if (fs.existsSync(viteConfigPath + '.bak')) {
+ fs.copyFileSync(viteConfigPath + '.bak', viteConfigPath)
+ fs.unlinkSync(viteConfigPath + '.bak')
+ }
+ throw error
+ }Committable suggestion skipped: line range outside the PR's diff.
| const fullDestPath = path.resolve(outDir, destPath) | ||
|
|
||
| try { | ||
| // 确保目标目录存在 | ||
| await fs.ensureDir(path.dirname(fullDestPath)) |
There was a problem hiding this comment.
Potential path traversal outside of outDir
path.resolve(outDir, destPath) will happily resolve destPath strings containing ../ and copy files outside the intended output directory (e.g. dest: '../../etc').
Add a guard:
const fullDestPath = path.resolve(outDir, destPath)
+if (!fullDestPath.startsWith(path.resolve(outDir))) {
+ logger.error('[vite-cdn-copy-plugin]: Refusing to write outside outDir', fullDestPath)
+ continue
+}This mitigates an easy foot-gun and a security risk when targets come from user config.
Also applies to: 92-93
| const files = fg.sync(`${absoluteSrcPath}/**/*`, { onlyFiles: true }) | ||
|
|
||
| // 处理每个文件 | ||
| for (const file of files) { | ||
| const relativePath = path.relative(absoluteSrcPath, file) | ||
| const destFilePath = path.join(fullDestPath, relativePath) | ||
|
|
||
| // 确保目标文件的目录存在 | ||
| await fs.ensureDir(path.dirname(destFilePath)) | ||
|
|
||
| // 读取文件内容 | ||
| const content = await fs.readFile(file, 'utf-8') | ||
|
|
||
| // 应用转换 | ||
| const transformedContent = replaceJsImportPaths(content, file) | ||
|
|
||
| // 写入转换后的内容 | ||
| await fs.writeFile(destFilePath, transformedContent) | ||
| } |
There was a problem hiding this comment.
Binary files corrupted by utf-8 read/write
The directory branch assumes every file is text (fs.readFile(..., 'utf-8')). Images, fonts, and wasm blobs will be mangled.
Replace with a binary-safe flow:
-const content = await fs.readFile(file, 'utf-8')
-const transformedContent = replaceJsImportPaths(content, file)
-await fs.writeFile(destFilePath, transformedContent)
+if (JS_EXT_RE.test(file)) {
+ const content = await fs.readFile(file, 'utf8')
+ const transformed = replaceJsImportPaths(content, file)
+ await fs.writeFile(destFilePath, transformed)
+} else {
+ await fs.copyFile(file, destFilePath)
+}Do the same for the single-file branch below to guarantee asset integrity.
Committable suggestion skipped: line range outside the PR's diff.
| function replaceJsImportPaths(content, filename) { | ||
| if (filename.endsWith('.js')) { | ||
| const result = babelReplaceImportPathWithCertainFileName(content, filename, console) | ||
| return result.code || content | ||
| } | ||
| return content | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle additional script extensions & avoid needless string operations
The helper only transforms files ending in .js, but Vite users can author modules with .mjs, .cjs, .jsx, .ts, etc.
Proposed improvement:
-function replaceJsImportPaths(content, filename) {
- if (filename.endsWith('.js')) {
+const JS_EXT_RE = /\.(?:m?js|cjs|jsx?|tsx?)$/i
+
+function replaceJsImportPaths(content, filename) {
+ if (JS_EXT_RE.test(filename)) {Expanding the regex future-proofs the plugin and prevents silent skips for legitimate script assets.
📝 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.
| function replaceJsImportPaths(content, filename) { | |
| if (filename.endsWith('.js')) { | |
| const result = babelReplaceImportPathWithCertainFileName(content, filename, console) | |
| return result.code || content | |
| } | |
| return content | |
| } | |
| const JS_EXT_RE = /\.(?:m?js|cjs|jsx?|tsx?)$/i | |
| function replaceJsImportPaths(content, filename) { | |
| if (JS_EXT_RE.test(filename)) { | |
| const result = babelReplaceImportPathWithCertainFileName(content, filename, console) | |
| return result.code || content | |
| } | |
| return content | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
55-71: 🛠️ Refactor suggestionEnhance semantic version comparison
The current implementation only checks the first digit(s) for versions prefixed with
^or~. This might lead to inaccurate version comparisons, especially for more complex version patterns.Consider using a dedicated SemVer library (like
semvernpm package) to ensure proper version comparison according to SemVer rules, or implement a more comprehensive comparison function:- const compareIsSameVersion = (versionOrigin, versionTarget) => { - if (versionOrigin === versionTarget) { - return true - } - - if (versionOrigin.startsWith('^')) { - // 如果源版本号是 ^ 开头，则只比较第一个数字是否相同 - return versionOrigin.slice(1, 2) === versionTarget.slice(0, 1) - } - - if (versionOrigin.startsWith('~')) { - // 如果源版本号是 ~ 开头，则只比较前两个数字是否相同 - return versionOrigin.slice(1, 3) === versionTarget.slice(0, 3) - } - - return false - } + const compareIsSameVersion = (versionOrigin, versionTarget) => { + // Import or install the semver package first + const semver = require('semver'); + + if (versionOrigin === versionTarget) { + return true; + } + + // Use semver.satisfies to check if the target version satisfies the origin version range + return semver.satisfies(versionTarget, versionOrigin); + }
🧹 Nitpick comments (4)
packages/design-core/src/preview/src/preview/srcFiles.js (3)
29-32: Consider using useEnv() consistently for environment variablesWhile the logic for determining delimiters is correct, this code directly accesses
import.meta.envinstead of using the importeduseEnv()function, which is used elsewhere in the file (line 67) and in other files likeimportMap.js.-const versionDelimiter = - import.meta.env.VITE_CDN_TYPE === 'npmmirror' && !import.meta.env.VITE_LOCAL_CDN_PATH ? '/' : '@' -const fileDelimiter = - import.meta.env.VITE_CDN_TYPE === 'npmmirror' && !import.meta.env.VITE_LOCAL_CDN_PATH ? '/files' : '' +const { VITE_CDN_TYPE, VITE_LOCAL_CDN_PATH } = useEnv() +const versionDelimiter = VITE_CDN_TYPE === 'npmmirror' && !VITE_LOCAL_CDN_PATH ? '/' : '@' +const fileDelimiter = VITE_CDN_TYPE === 'npmmirror' && !VITE_LOCAL_CDN_PATH ? '/files' : ''
38-38: Consider using useEnv() for consistent environment variable accessSimilar to the previous comment, consider using the
useEnv()function for accessing environment variables instead of directly usingimport.meta.env.- .replaceAll('${VITE_CDN_DOMAIN}', import.meta.env.VITE_LOCAL_CDN_PATH || import.meta.env.VITE_CDN_DOMAIN) + .replaceAll('${VITE_CDN_DOMAIN}', useEnv().VITE_LOCAL_CDN_PATH || useEnv().VITE_CDN_DOMAIN)
67-67: Consider consistent string replacement methodsWhile the logic here is correct, this code uses
replacewith a regex pattern, whereas line 38 usesreplaceAllwith a string literal. Consider using a consistent approach throughout the codebase.- fileContent: appJS.replace(/VITE_CDN_DOMAIN/g, useEnv().VITE_LOCAL_CDN_PATH || useEnv().VITE_CDN_DOMAIN) + fileContent: appJS.replaceAll('${VITE_CDN_DOMAIN}', useEnv().VITE_LOCAL_CDN_PATH || useEnv().VITE_CDN_DOMAIN)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js (1)
9-9: Consider using English comments for consistencyThe comment is in a language other than English (looks like Chinese). For international collaboration and maintenance, it would be better to use English comments throughout the codebase.
-// 默认的复制配置，这几个 package 需要复制整个目录，所以需要有默认配置进行复制整个目录 +// Default copy configuration - these packages need their entire directories copied
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
designer-demo/env/.env.alpha(1 hunks)designer-demo/env/.env.localCDN.example(1 hunks)packages/build/vite-config/src/default-config.js(3 hunks)packages/build/vite-config/src/localCdnFile/localCdnPlugin.js(1 hunks)packages/canvas/DesignCanvas/src/importMap.ts(2 hunks)packages/common/js/importMap/import-map.json(2 hunks)packages/common/js/importMap/index.js(1 hunks)packages/common/vite.config.ts(1 hunks)packages/design-core/src/preview/src/preview/importMap.js(1 hunks)packages/design-core/src/preview/src/preview/srcFiles.js(3 hunks)packages/design-core/vite.config.js(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/common/js/importMap/import-map.json
- designer-demo/env/.env.localCDN.example
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/common/js/importMap/index.js
- packages/common/vite.config.ts
- designer-demo/env/.env.alpha
- packages/design-core/vite.config.js
- packages/canvas/DesignCanvas/src/importMap.ts
- packages/build/vite-config/src/default-config.js
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/design-core/src/preview/src/preview/srcFiles.js (1)
packages/design-core/src/preview/src/preview/importMap.js (3)
versionDelimiter(23-23)fileDelimiter(24-24)useEnv(22-22)
packages/design-core/src/preview/src/preview/importMap.js (3)
packages/build/vite-config/src/localCdnFile/copyPreviewImportMap.js (1)
importMap(17-17)packages/design-core/src/preview/src/preview/srcFiles.js (2)
versionDelimiter(29-30)fileDelimiter(31-32)packages/common/js/environments.js (4)
VITE_CDN_TYPE(19-19)VITE_CDN_TYPE(19-19)VITE_CDN_DOMAIN(18-18)VITE_CDN_DOMAIN(18-18)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/design-core/src/preview/src/preview/importMap.js (3)
15-15: Good change - centralized import map configurationThe change from importing a local JSON file to importing from a centralized module enhances maintainability and consistency across the codebase.
22-24: Improved environment variables handlingGood refactoring to destructure environment variables once from
useEnv()and properly handle conditional delimiters based on both CDN type and local CDN path configuration.
27-27: Proper local CDN path prioritizationCorrectly prioritizing
VITE_LOCAL_CDN_PATHoverVITE_CDN_DOMAINsupports the local CDN solution's goal of offline or intranet deployment scenarios.packages/design-core/src/preview/src/preview/srcFiles.js (1)
13-13: Good addition of useEnv importImporting the
useEnvutility allows for consistent environment variable access across the codebase.
| function extractInfo(str) { | ||
| try { | ||
| let [packageName, versionAndPath] = str.split('${versionDelimiter}') | ||
| packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '') | ||
| const [version, filePath] = versionAndPath.split('${fileDelimiter}') | ||
|
|
||
| return { | ||
| packageName, | ||
| version, | ||
| filePathInPackage: filePath || '/' | ||
| } | ||
| } catch (error) { | ||
| logger.error(`[local-cdn-plugin]: Failed to extract info from ${str} 提取 importMap 信息失败`, error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in extractInfo function
The function logs errors but doesn't return any value when an error occurs. This could lead to undefined behavior when the result is used later in the code.
function extractInfo(str) {
try {
let [packageName, versionAndPath] = str.split('${versionDelimiter}')
packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '')
const [version, filePath] = versionAndPath.split('${fileDelimiter}')
return {
packageName,
version,
filePathInPackage: filePath || '/'
}
} catch (error) {
logger.error(`[local-cdn-plugin]: Failed to extract info from ${str} 提取 importMap 信息失败`, error)
+ return null
}
}You should also add validation for null values when using the results of this function.
📝 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.
| function extractInfo(str) { | |
| try { | |
| let [packageName, versionAndPath] = str.split('${versionDelimiter}') | |
| packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '') | |
| const [version, filePath] = versionAndPath.split('${fileDelimiter}') | |
| return { | |
| packageName, | |
| version, | |
| filePathInPackage: filePath || '/' | |
| } | |
| } catch (error) { | |
| logger.error(`[local-cdn-plugin]: Failed to extract info from ${str} 提取 importMap 信息失败`, error) | |
| } | |
| } | |
| function extractInfo(str) { | |
| try { | |
| let [packageName, versionAndPath] = str.split('${versionDelimiter}') | |
| packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '') | |
| const [version, filePath] = versionAndPath.split('${fileDelimiter}') | |
| return { | |
| packageName, | |
| version, | |
| filePathInPackage: filePath || '/' | |
| } | |
| } catch (error) { | |
| logger.error(`[local-cdn-plugin]: Failed to extract info from ${str} 提取 importMap 信息失败`, error) | |
| return null | |
| } | |
| } |
| const defaultImportMapConfig = JSON.parse( | ||
| fs.readFileSync(path.resolve(process.cwd(), './node_modules/@opentiny/tiny-engine/dist/import-map.json'), 'utf-8') | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file reading operation
Reading the default import map file lacks error handling. If the file doesn't exist or has invalid JSON, it will throw an unhandled exception.
- const defaultImportMapConfig = JSON.parse(
- fs.readFileSync(path.resolve(process.cwd(), './node_modules/@opentiny/tiny-engine/dist/import-map.json'), 'utf-8')
- )
+ let defaultImportMapConfig = { imports: {} };
+ try {
+ const importMapPath = path.resolve(process.cwd(), './node_modules/@opentiny/tiny-engine/dist/import-map.json');
+ if (fs.existsSync(importMapPath)) {
+ defaultImportMapConfig = JSON.parse(
+ fs.readFileSync(importMapPath, 'utf-8')
+ );
+ } else {
+ logger.warn('[local-cdn-plugin]: Default import-map.json not found, using empty import map');
+ }
+ } catch (error) {
+ logger.error('[local-cdn-plugin]: Failed to read or parse default import-map.json', error);
+ }Committable suggestion skipped: line range outside the PR's diff.
| if (fs.existsSync(pkgFilePath)) { | ||
| try { | ||
| const pkg = JSON.parse(fs.readFileSync(path.resolve(pkgFilePath))) | ||
| isSameVersion = compareIsSameVersion(version, pkg.version) | ||
| } catch (error) { | ||
| // ignore | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling when reading package.json
The current code silently ignores errors when reading or parsing package.json. This could hide important issues that might affect version comparison accuracy.
if (fs.existsSync(pkgFilePath)) {
try {
const pkg = JSON.parse(fs.readFileSync(path.resolve(pkgFilePath)))
isSameVersion = compareIsSameVersion(version, pkg.version)
} catch (error) {
- // ignore
+ logger.warn(`[local-cdn-plugin]: Failed to parse package.json for ${packageName}`, error)
+ // Default to false in case of error
+ isSameVersion = false
}
}📝 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.
| if (fs.existsSync(pkgFilePath)) { | |
| try { | |
| const pkg = JSON.parse(fs.readFileSync(path.resolve(pkgFilePath))) | |
| isSameVersion = compareIsSameVersion(version, pkg.version) | |
| } catch (error) { | |
| // ignore | |
| } | |
| } | |
| if (fs.existsSync(pkgFilePath)) { | |
| try { | |
| const pkg = JSON.parse(fs.readFileSync(path.resolve(pkgFilePath))) | |
| isSameVersion = compareIsSameVersion(version, pkg.version) | |
| } catch (error) { | |
| logger.warn(`[local-cdn-plugin]: Failed to parse package.json for ${packageName}`, error) | |
| // Default to false in case of error | |
| isSameVersion = false | |
| } | |
| } |
| * @param {Object} options - 配置选项 | ||
| * @param {Object} options.localCdnConfig - 本地CDN配置 | ||
| * @param {Object} options.localCdnConfig.importMap - 导入映射配置，定义需要本地化的CDN依赖 | ||
| * @param {Object} options.localCdnConfig.copy - 自定义复制配置，可以覆盖特定包的默认配置 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate importMap structure
The code assumes a specific structure for the importMap configuration, but doesn't validate it before use. Adding validation would improve robustness.
export function localCdnPlugin({
localCdnConfig = { importMap: { imports: {} }, copy: {} },
base = './',
cdnDir = 'local-cdn-static', // 构建目录中的CDN文件夹名称
bundleTempDir = 'bundle-deps/local-cdn' // 临时存放下载的包的目录
}) {
+ // Validate importMap structure
+ if (!localCdnConfig || typeof localCdnConfig !== 'object') {
+ logger.warn('[local-cdn-plugin]: Invalid localCdnConfig, using defaults')
+ localCdnConfig = { importMap: { imports: {} }, copy: {} }
+ }
+
const importMapConfig = localCdnConfig.importMap || { imports: {} }
+ if (!importMapConfig.imports || typeof importMapConfig.imports !== 'object') {
+ logger.warn('[local-cdn-plugin]: Invalid importMap.imports, using empty object')
+ importMapConfig.imports = {}
+ }📝 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.
| * @param {Object} options.localCdnConfig.copy - 自定义复制配置，可以覆盖特定包的默认配置 | |
| export function localCdnPlugin({ | |
| localCdnConfig = { importMap: { imports: {} }, copy: {} }, | |
| base = './', | |
| cdnDir = 'local-cdn-static', // 构建目录中的CDN文件夹名称 | |
| bundleTempDir = 'bundle-deps/local-cdn' // 临时存放下载的包的目录 | |
| }) { | |
| // Validate importMap structure | |
| if (!localCdnConfig || typeof localCdnConfig !== 'object') { | |
| logger.warn('[local-cdn-plugin]: Invalid localCdnConfig, using defaults') | |
| localCdnConfig = { importMap: { imports: {} }, copy: {} } | |
| } | |
| const importMapConfig = localCdnConfig.importMap || { imports: {} } | |
| if (!importMapConfig.imports || typeof importMapConfig.imports !== 'object') { | |
| logger.warn('[local-cdn-plugin]: Invalid importMap.imports, using empty object') | |
| importMapConfig.imports = {} | |
| } | |
| // …rest of plugin implementation… | |
| } |
| const parsedDefaultImportMapConfig = Object.values(defaultImportMapConfig.imports).map((item) => extractInfo(item)) | ||
| const parsedImportMapConfig = Object.values(importMapConfig.imports).map((item) => extractInfo(item)) | ||
| const overriddenImportMap = parsedDefaultImportMapConfig.filter((item) => { | ||
| return !parsedImportMapConfig.find((parsedItem) => parsedItem.packageName === item.packageName) | ||
| }) | ||
| const combinedImportMapConfig = [...overriddenImportMap, ...parsedImportMapConfig] |
There was a problem hiding this comment.
Handle potential null values from extractInfo
The extractInfo function can return undefined when an error occurs, but the code doesn't handle these cases when mapping over the import map values.
- const parsedDefaultImportMapConfig = Object.values(defaultImportMapConfig.imports).map((item) => extractInfo(item))
- const parsedImportMapConfig = Object.values(importMapConfig.imports).map((item) => extractInfo(item))
+ const parsedDefaultImportMapConfig = Object.values(defaultImportMapConfig.imports || {})
+ .map((item) => extractInfo(item))
+ .filter(Boolean) // Filter out null/undefined values
+ const parsedImportMapConfig = Object.values(importMapConfig.imports || {})
+ .map((item) => extractInfo(item))
+ .filter(Boolean) // Filter out null/undefined values
const overriddenImportMap = parsedDefaultImportMapConfig.filter((item) => {
return !parsedImportMapConfig.find((parsedItem) => parsedItem.packageName === item.packageName)
})📝 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.
| const parsedDefaultImportMapConfig = Object.values(defaultImportMapConfig.imports).map((item) => extractInfo(item)) | |
| const parsedImportMapConfig = Object.values(importMapConfig.imports).map((item) => extractInfo(item)) | |
| const overriddenImportMap = parsedDefaultImportMapConfig.filter((item) => { | |
| return !parsedImportMapConfig.find((parsedItem) => parsedItem.packageName === item.packageName) | |
| }) | |
| const combinedImportMapConfig = [...overriddenImportMap, ...parsedImportMapConfig] | |
| const parsedDefaultImportMapConfig = Object.values(defaultImportMapConfig.imports || {}) | |
| .map((item) => extractInfo(item)) | |
| .filter(Boolean) // Filter out null/undefined values | |
| const parsedImportMapConfig = Object.values(importMapConfig.imports || {}) | |
| .map((item) => extractInfo(item)) | |
| .filter(Boolean) // Filter out null/undefined values | |
| const overriddenImportMap = parsedDefaultImportMapConfig.filter((item) => { | |
| return !parsedImportMapConfig.find((parsedItem) => parsedItem.packageName === item.packageName) | |
| }) | |
| const combinedImportMapConfig = [...overriddenImportMap, ...parsedImportMapConfig] |
| .reduce((acc, cur) => { | ||
| // 同个包避免多个版本只保留一个版本 | ||
| if (!acc.some(({ packageName }) => cur.packageName === packageName)) { | ||
| acc.push(cur) | ||
| } | ||
| return acc | ||
| }, []) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider preserving version differences in package installations
The current deduplication logic only keeps one version per package name. This could be problematic if different parts of the application require different versions of the same package.
🏁 Script executed:
#!/bin/bash
# Check if there are any duplicated package names with different versions in the import maps
# First, extract all package entries from the combined config
echo "Checking for multiple versions of the same package in import maps..."
# Use ast-grep to find places where the same package name might have different versions
ast-grep --pattern 'const [$_] = combinedImportMapConfig.map'
# Search for potential version conflicts
rg -A 5 'packageName.*version' packages/build/vite-config/src/localCdnFileLength of output: 7358
Preserve multiple versions in dedup logic
The current .reduce only keys off packageName, so any additional versions of the same package are dropped. To allow coexisting versions, include version in the uniqueness check:
- File: packages/build/vite-config/src/localCdnFile/localCdnPlugin.js
Lines: ~183–189
Diff:
- .reduce((acc, cur) => {
- // 同个包避免多个版本只保留一个版本
- if (!acc.some(({ packageName }) => cur.packageName === packageName)) {
- acc.push(cur)
- }
- return acc
- }, [])
+ .reduce((acc, cur) => {
+ // 同包不同版本也需保留
+ if (!acc.some(({ packageName, version }) =>
+ cur.packageName === packageName &&
+ cur.version === version
+ )) {
+ acc.push(cur)
+ }
+ return acc
+ }, [])This ensures entries with the same name but different versions are both included.
📝 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.
| .reduce((acc, cur) => { | |
| // 同个包避免多个版本只保留一个版本 | |
| if (!acc.some(({ packageName }) => cur.packageName === packageName)) { | |
| acc.push(cur) | |
| } | |
| return acc | |
| }, []) | |
| .reduce((acc, cur) => { | |
| // 同包不同版本也需保留 | |
| if (!acc.some(({ packageName, version }) => | |
| cur.packageName === packageName && | |
| cur.version === version | |
| )) { | |
| acc.push(cur) | |
| } | |
| return acc | |
| }, []) |
| let [packageName, versionAndPath] = str.split('${versionDelimiter}') | ||
| packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '') | ||
| const [version, filePath] = versionAndPath.split('${fileDelimiter}') |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the variable delimiters usage
The code uses ${versionDelimiter} and ${fileDelimiter} as string literals for splitting, but it's not clear where these values are defined or set. This could lead to confusion and potential errors.
🏁 Script executed:
#!/bin/bash
# Check where versionDelimiter and fileDelimiter are defined or used
echo "Searching for versionDelimiter and fileDelimiter definitions or usages..."
rg -A 2 -B 2 "(versionDelimiter|fileDelimiter)" --type js --type tsLength of output: 6484
Clarify placeholder usage in extractInfo
The extractInfo function in
packages/build/vite-config/src/localCdnFile/localCdnPlugin.js
splits on the literal strings ${versionDelimiter} and ${fileDelimiter} to pull apart the CDN path. Those placeholders are actually defined (and replaced at runtime) in your import-map generators:
- packages/canvas/DesignCanvas/src/importMap.ts
•const versionDelimiter = … ? '/' : '@'
•const fileDelimiter = … ? '/files' : '' - packages/design-core/src/preview/src/preview/importMap.js
Please add a brief comment above extractInfo so future readers know where these placeholders originate and why we split on the string "${versionDelimiter}" rather than a hard-coded @ or /.
• File: packages/build/vite-config/src/localCdnFile/localCdnPlugin.js
• Location: immediately above function extractInfo(str) {
Suggested diff:
+/**
+ * The strings `${versionDelimiter}` and `${fileDelimiter}` are
+ * injected by our import-map generators (see
+ * packages/canvas/DesignCanvas/src/importMap.ts and
+ * preview/importMap.js). We split on these placeholders here
+ * to extract packageName, version, and path.
+ */
function extractInfo(str) {
try {
let [packageName, versionAndPath] = str.split('${versionDelimiter}')
packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '')
const [version, filePath] = versionAndPath.split('${fileDelimiter}')📝 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.
| let [packageName, versionAndPath] = str.split('${versionDelimiter}') | |
| packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '') | |
| const [version, filePath] = versionAndPath.split('${fileDelimiter}') | |
| /** | |
| * The strings `${versionDelimiter}` and `${fileDelimiter}` are | |
| * injected by our import-map generators (see | |
| * packages/canvas/DesignCanvas/src/importMap.ts and | |
| * preview/importMap.js). We split on these placeholders here | |
| * to extract packageName, version, and path. | |
| */ | |
| function extractInfo(str) { | |
| try { | |
| let [packageName, versionAndPath] = str.split('${versionDelimiter}') | |
| packageName = packageName.replace(/^\$\{VITE_CDN_DOMAIN\}\//, '') | |
| const [version, filePath] = versionAndPath.split('${fileDelimiter}') | |
| // …rest of implementation… | |
| } catch (e) { | |
| // handle error | |
| } | |
| } |
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores