fix: localImportMap not works#1242
Conversation
WalkthroughThis update introduces a comprehensive solution for localizing CDN resources in the TinyEngine platform. The changes span build tooling, environment configuration, documentation, and testing. Key enhancements include a new Vite plugin for copying and transforming CDN dependencies into a local directory during build, dynamic import map handling, and updates to environment files and test infrastructure. Documentation is expanded with a new guide on the local CDN solution. Supporting utilities and configuration changes ensure that both runtime and build-time dependencies can be served from local assets, improving reliability in restricted or enterprise environments. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant BuildSystem (Vite)
participant importMapLocalPlugin
participant NodeModules
participant LocalCDNDir
Developer->>BuildSystem (Vite): Starts build with local CDN env vars
BuildSystem (Vite)->>importMapLocalPlugin: Initialize with importMapLocalConfig
importMapLocalPlugin->>NodeModules: Check/install required packages
importMapLocalPlugin->>NodeModules: Locate/copy required files
importMapLocalPlugin->>LocalCDNDir: Copy files, rewrite import paths
BuildSystem (Vite)->>LocalCDNDir: Include local CDN assets in output
Developer->>BuildSystem (Vite): Build completes, local CDN ready
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:Details
✅ 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:Details
✅ 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:Details
❓ 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:Details
❓ 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:Details
❓ 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:Details
❓ 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.
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.
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:Details
❓ 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:Details
❓ 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:Details
✅ 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:Details
❓ 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:Details
✅ 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.
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.
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:Details
❓ 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.
chilingling
force-pushed
the
feat/localCDNPlugin
branch
from
03b23f4 to
3cf5017
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/build/vite-config/src/localCdnFile/importMapLocalPlugin.js (2)
119-124:⚠️ Potential issueDestination path uses original version instead of potentially overridden version.
The destination path construction uses
originVersioneven though the version might have been overridden by the copy configuration (lines 83-92). This could lead to copied files being placed under a different version folder than expected.- const destPackageDir = `${cdnDir}/${packageName}@${originVersion}` + const destPackageDir = `${cdnDir}/${packageName}@${version}`
183-186: 🛠️ Refactor suggestionAdd error handling for missing default import map.
The code reads the default import map without error handling, which could break the build if the file doesn't exist.
- 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 { + defaultImportMapConfig = JSON.parse( + fs.readFileSync( + path.resolve(process.cwd(), './node_modules/@opentiny/tiny-engine/dist/import-map.json'), + 'utf-8' + ) + ) + } catch (error) { + logger.warn('[import-map-local-plugin]: Default import map not found, using empty config', error) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.gitignore(1 hunks)designer-demo/env/.env.alpha(0 hunks)designer-demo/env/.env.development(0 hunks)designer-demo/env/.env.localCDN.example(1 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/import-map-local.md(1 hunks)packages/build/vite-config/package.json(2 hunks)packages/build/vite-config/src/default-config.js(3 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/importMapLocalPlugin.js(1 hunks)packages/build/vite-config/src/localCdnFile/index.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/canvas/DesignCanvas/src/DesignCanvas.vue(1 hunks)packages/canvas/DesignCanvas/src/importMap.ts(2 hunks)packages/common/js/import-map.json(2 hunks)packages/common/js/importMap.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/importMap.js(1 hunks)packages/design-core/src/preview/src/preview/srcFiles.js(3 hunks)packages/design-core/vite.config.js(3 hunks)
💤 Files with no reviewable changes (2)
- designer-demo/env/.env.development
- designer-demo/env/.env.alpha
✅ Files skipped from review due to trivial changes (2)
- designer-demo/package.json
- packages/build/vite-config/src/localCdnFile/replaceImportPath.mjs
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/common/js/importMap.js
- .gitignore
- packages/build/vite-config/src/localCdnFile/copyImportMap.js
- packages/build/vite-config/src/localCdnFile/index.js
- packages/common/vite.config.ts
- packages/common/package.json
- packages/build/vite-config/package.json
- docs/catalog.json
- docs/README.md
- packages/common/js/import-map.json
- designer-demo/vitest.config.js
- packages/canvas/DesignCanvas/src/DesignCanvas.vue
- designer-demo/env/.env.localCDN.example
- packages/design-core/src/preview/src/preview/importMap.js
- packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js
- designer-demo/tests/localCdnCustomConfig.test.js
- packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js
- packages/design-core/src/preview/src/preview/srcFiles.js
- packages/build/vite-config/src/default-config.js
- designer-demo/public/mock/bundle.json
- packages/canvas/DesignCanvas/src/importMap.ts
- packages/design-core/package.json
- designer-demo/tests/utils/envHelpers.js
- designer-demo/tests/localCdnBasic.test.js
- packages/design-core/vite.config.js
- designer-demo/tests/localCdnBundleDeps.test.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/build/vite-config/src/localCdnFile/importMapLocalPlugin.js (3)
packages/build/vite-config/src/vite-plugins/cdnCopyPlugin.js (2)
logger(18-18)copyPlugin(114-154)packages/build/vite-config/src/localCdnFile/locateCdnNpmInfo.js (1)
dedupeCopyFiles(198-209)packages/build/vite-config/src/vite-plugins/installPackageTemporary.js (1)
installPackageTemporary(5-47)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (11)
packages/build/vite-config/src/localCdnFile/copyBundleDeps.js (3)
15-42: Added support for packages array alongside components array.The function now uses a Set to ensure uniqueness of extracted URLs and adds support for the new
packagesarray structure while maintaining compatibility with the existingcomponentsarray. This change handles both data structures correctly.
58-68: Extended replaceBundleCdnLink to handle packages array.This addition properly extends the CDN link replacement functionality to the new packages array structure, maintaining parallel logic with the existing components array handling.
80-83: Added filtering for invalid CDN file entries.The addition of
filter(Boolean)ensures that null values returned fromgetCdnPathNpmInfoForSingleFilewon't cause issues in downstream processing. The explanatory comment correctly describes scenarios that might lead to matching failures.packages/build/vite-config/src/localCdnFile/importMapLocalPlugin.js (3)
1-24: Well-structured plugin with clear dependencies and default configuration.The plugin correctly imports necessary dependencies and defines default copy configurations for packages that require special handling.
213-222: Efficient package deduplication logic.The deduplication logic for packages that need installation is well-implemented, ensuring that only one version of each package is included in the installation list.
232-237: Clear plugin composition and execution.The plugin composition is clean, first installing needed packages temporarily and then copying files to the target location using the custom copy plugin.
docs/solutions/import-map-local.md (5)
1-43: Comprehensive documentation with clear problem statement and solution.The documentation begins with a well-defined problem statement and solution overview, explaining why this local CDN approach is needed and the benefits it provides. The environment variable configuration section is detailed and clear.
57-103: Detailed configuration options with import map format explanation.The documentation thoroughly explains the configuration options, import map format, and URL structure with placeholders. The warning about registering the custom import map in the runtime registry is crucial and well-explained.
104-142: Clear copy configuration explanation with examples.The documentation explains the copy configuration options clearly, including the default configurations and how to override them. The examples provided are helpful for understanding how to customize the copying behavior.
143-181: Important note on bundle files and CDN domain matching.The documentation correctly explains how the bundle file CDN links are processed and the requirement that the URL prefixes must match the configured CDN domain. The example helps clarify this important constraint.
182-219: Well-explained implementation principles and considerations.The implementation principles section provides a good technical overview of how the solution works internally. The final notes section appropriately mentions the trade-off between increased build size and improved reliability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
docs/solutions/import-map-local.md (15)
1-2: Consistent title formatting
The heading本地化 import-map CDN方案mixes English and Chinese without spacing. Consider normalizing it, for example:
本地化 Import Map CDN 方案Import Map CDN 本地化方案
5-7: Clarify redundant wording
The phrase “依赖 vue、vue-i18n 以及物料等依赖” repeats “依赖”. You might rephrase to avoid duplication, e.g.:
“依赖 vue、vue-i18n 以及各类物料模块”
8-11: Unify bullet-list style
The two options are presented inline. For readability, indent them as a Markdown list:当前可选方案： - 搭建私网 unpkg - 使用本文档介绍的 import-map CDN 本地化方案
26-37: Polish environment-variable code block
- Remove the blank line after the comment
# .env.alpha、.env.production、.env.development- Specify the code block language as
bashfor syntax highlighting
41-49: Expand example import for clarity
The snippet usesuseTinyEngineBaseConfigbut doesn’t show its import. Consider adding at the top of the code block:import { useTinyEngineBaseConfig } from 'packages/build/vite-config';This helps readers understand where that helper comes from.
55-60: Align table styling and code formatting
- Add code fences around default object literals for readability.
- Ensure column widths are consistent (e.g., longer descriptions may wrap).
67-74: Annotate JSON code block
Specify the language for syntax highlighting:{ "imports": { "vue": "${VITE_CDN_DOMAIN}/vue${versionDelimiter}3.2.37${fileDelimiter}/dist/vue.global.prod.js", ... } }
85-94: Wrap registry snippet in proper code fence
Add ajavascriptcode fence and semicolons to improve readability:// 在注册表配置中 { config: { id: 'engine.config', importMap: importMapLocalConfig.importMap, // ... 其他配置 } };
104-117: Specify code fence for default copy config
Use ajavascriptfence and align indentation:{ '@opentiny/vue-theme': { filePathInPackage: '/' }, '@opentiny/vue-renderless': { filePathInPackage: '/' }, // ... }
126-135: Clarify custom copy example
Wrap in ajavascriptfence and note thatversiononly affects downloaded version, not URL path:{ 'vue': { filePathInPackage: '/dist/' }, 'element-plus': { filePathInPackage: '/dist/index.css', version: '2.2.0' } }
147-156: Bundle.json snippet formatting
- Mark the code block as
json- Ensure the snippet only shows the relevant part or include ellipsis to avoid overwhelming detail.
170-173: Annotate.env.alphaexample
Wrap inbashfences and include a preceding comment explaining whyVITE_CDN_DOMAINis needed here:# .env.alpha VITE_CDN_DOMAIN=https://unpkg.com
181-187: Consistent list markers
In the implementation steps, some lists use hyphens, others use bullets. Convert all to hyphens for consistency:- 解析 CDN URL 获取包名、版本和文件路径 - 合并用户配置和默认配置 - 生成文件复制任务列表
195-200: Uniform bullet formatting
Under “复制并转换文件” and “处理Bundle文件” sections, ensure sub-items are indented and prefixed consistently (either all hyphens or all asterisks).
210-213: Add punctuation for completeness
The second注意事项 item lacks a period at the end. For consistency, append “。” or “.”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
designer-demo/env/.env.localCDN.example(1 hunks)designer-demo/tests/localCdnBasic.test.js(1 hunks)designer-demo/tests/localCdnCustomConfig.test.js(1 hunks)docs/solutions/import-map-local.md(1 hunks)packages/canvas/DesignCanvas/src/importMap.ts(2 hunks)packages/design-core/src/preview/src/preview/http.js(0 hunks)packages/design-core/src/preview/src/preview/importMap.js(1 hunks)packages/design-core/src/preview/src/preview/srcFiles.js(4 hunks)packages/design-core/src/preview/src/preview/usePreviewData.ts(1 hunks)packages/design-core/vite.config.js(3 hunks)
💤 Files with no reviewable changes (1)
- packages/design-core/src/preview/src/preview/http.js
🚧 Files skipped from review as they are similar to previous changes (7)
- designer-demo/env/.env.localCDN.example
- packages/design-core/vite.config.js
- designer-demo/tests/localCdnBasic.test.js
- packages/design-core/src/preview/src/preview/srcFiles.js
- packages/design-core/src/preview/src/preview/importMap.js
- designer-demo/tests/localCdnCustomConfig.test.js
- packages/canvas/DesignCanvas/src/importMap.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/design-core/src/preview/src/preview/usePreviewData.ts (4)
7-7: Simplified HTTP importsThe imports from './http' have been consolidated into a single line, removing the previously imported
fetchImportMapfunction which is no longer used in this file. This aligns with the broader changes to how import maps are handled.
245-245: Import map initializationThis line uses the simplified
getImportMapfunction to process scripts from URL parameters. The removal of conditional behavior ensures consistent handling of import maps regardless of environment variables.
317-321: Import map state managementThis section handles updating the import map state when changes are detected. The code is unchanged but works with the simplified
getImportMapfunction, maintaining proper reactivity while benefiting from the more consistent import map handling approach.
148-150:Details
✅ Verification successful
Simplified import map handling logic
The
getImportMapfunction has been simplified to always return the result ofgetInitImportMap, removing previous conditional logic that likely checked environment variables (e.g.,VITE_LOCAL_BUNDLE_DEPS) and calledfetchImportMapwhen needed.This change is central to fixing the local import map functionality issue mentioned in the PR, as it standardizes how import maps are obtained regardless of environment.
🏁 Script executed:
#!/bin/bash # Verify that no code depends on the previous conditional behavior of getImportMap rg -g "*.{js,ts,vue}" "VITE_LOCAL_BUNDLE_DEPS" --context 3Length of output: 7684
🏁 Script executed:
#!/bin/bash # Show context of getImportMap in usePreviewData.ts rg -n "getImportMap" -g "packages/design-core/src/preview/src/preview/usePreviewData.ts" -C 5Length of output: 3291
🏁 Script executed:
#!/bin/bash # Display the implementation of getInitImportMap in importMap.js rg -n "getInitImportMap" -g "packages/design-core/src/preview/src/preview/importMap.js" -C 10Length of output: 91
🏁 Script executed:
#!/bin/bash # Locate the definition of getImportMap in the preview importMap file rg -n "export (?:async )?function getImportMap" -g "packages/design-core/src/preview/src/preview/importMap.js" -C 5Length of output: 115
🏁 Script executed:
#!/bin/bash # Display the contents of importMap.js to locate getInitImportMap sed -n '1,200p' packages/design-core/src/preview/src/preview/importMap.jsLength of output: 1714
Import map handling logic simplified and verified
- In usePreviewData.ts,
getImportMapnow cleanly delegates to thegetImportMapimplementation in importMap.js.- The importMap.js implementation still applies all environment‐based placeholders (
VITE_LOCAL_BUNDLE_DEPS,VITE_CDN_TYPE,VITE_CDN_DOMAIN, etc.) viareplacePlaceholder.- A global search for
fetchImportMapreturned no references, confirming there’s no residual conditional fallback in the preview code.All dependent code remains functional under the new unified import‐map resolution.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/updateTemplate.mjs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
docs/solutions/import-map-local.md (13)
1-2: Consider unifying terminology across the document
Some places still refer to “import-map” while the title uses “CDN 依赖本地化方案”. To avoid confusion, consistently use “CDN 依赖本地化” throughout.
5-17: Refine the overview section for clarity
The overview introduces both import-map and CDN 依赖本地化概念 but could be streamlined. Consider merging similar bullets and clarifying the relationship between import-map and 本地化方案.
29-37: Explain default values for environment variables
It would help to briefly describe typical values or use cases forVITE_LOCAL_IMPORT_PATH(e.g., default folder structure) and clarify the effect if these variables are omitted.
41-51: Link to default plugin configuration
Consider adding a hyperlink or file reference to the defaultimportMapLocalConfigimplementation (e.g.,default-config.js) so readers can inspect fallback behavior.
55-62: Improve table readability
The config options table is clear, but aligning cell separators (adding spaces) can improve visual parsing in markdown-rendered docs.
68-76: Example importMap format is clear
Good use of a JSON code block to illustrate the importMap structure. Consider adding a note about any differences if users paste this into a JavaScript config file.
78-89: Consolidate URL format explanations
The explanations forunpkgandnpmmirrorare similar. You could combine them into a side-by-side table or single comparison to reduce duplication.
114-124: Clarify registry config context
Explain where this registry config snippet should be placed (e.g., in your application’s meta config or registry UI) to prevent misplacement.
150-153: Bullet formatting incopyoptions
Indent the dashes under the option header (or convert to a sub-table) for consistency with earlier sections.
167-175: Expand on bundle processing caveat
You note that only matching prefixes are processed; consider adding an example of a non-matching URL to illustrate the behavior.
176-195: Condense longbundle.jsonexample
The JSON block is lengthy—consider trimming to one package entry with an ellipsis or collapsing irrelevant fields to keep the document concise.
209-231: Link plugin implementation for deeper dive
In the implementation principle section, include links to theimportMapLocalPluginsource or API docs for readers who want more technical detail.
239-243: Add caution about output size impact
You mention build size increase; consider adding approximate metrics or real-world ratios to set better expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
designer-demo/env/.env.development(0 hunks)designer-demo/env/.env.localCDN.example(1 hunks)designer-demo/env/.env.production(0 hunks)designer-demo/tests/localCdnBasic.test.js(1 hunks)designer-demo/tests/localCdnCustomConfig.test.js(1 hunks)docs/solutions/import-map-local.md(1 hunks)packages/build/vite-config/src/default-config.js(3 hunks)packages/canvas/DesignCanvas/src/importMap.ts(2 hunks)packages/design-core/src/preview/src/preview/importMap.js(1 hunks)packages/design-core/src/preview/src/preview/srcFiles.js(4 hunks)packages/design-core/vite.config.js(3 hunks)packages/engine-cli/template/designer/env/.env.alpha(0 hunks)packages/engine-cli/template/designer/env/.env.development(0 hunks)packages/engine-cli/template/designer/env/.env.production(0 hunks)
💤 Files with no reviewable changes (5)
- packages/engine-cli/template/designer/env/.env.alpha
- packages/engine-cli/template/designer/env/.env.production
- packages/engine-cli/template/designer/env/.env.development
- designer-demo/env/.env.production
- designer-demo/env/.env.development
✅ Files skipped from review due to trivial changes (1)
- designer-demo/tests/localCdnBasic.test.js
🚧 Files skipped from review as they are similar to previous changes (7)
- designer-demo/env/.env.localCDN.example
- packages/design-core/src/preview/src/preview/srcFiles.js
- packages/build/vite-config/src/default-config.js
- packages/design-core/vite.config.js
- designer-demo/tests/localCdnCustomConfig.test.js
- packages/design-core/src/preview/src/preview/importMap.js
- packages/canvas/DesignCanvas/src/importMap.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (6)
docs/solutions/import-map-local.md (6)
24-37: Add.env.developmentto the environment files list
You mention.env.alphaand.env.production, but developers might also use.env.developmentor other variants. Please verify and update the list to avoid confusion.
92-100: Mirror CDN example is accurate
The npmmirror URL breakdown is correct and informative.
101-109: Custom semver example is helpful
Demonstrating the^3.5version range adds practical value.
131-148: Defaultcopyconfig looks good
The default copy configuration for various packages is comprehensive and matches implementation defaults.
155-165: Good customcopyconfiguration example
This snippet clearly demonstrates overriding default copy settings.
200-203: Environment snippet is straightforward
Showing the.env.alphaexample withVITE_CDN_DOMAINis clear.
4 participants
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
Bug Fixes
Documentation
Tests
Chores