feat: external all dependencies of core package #1236
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update primarily expands the list of external dependencies across multiple Vite configuration files in various packages. The changes ensure that a broader set of modules—such as those under Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant initCanvas
participant Env
participant Script
App->>initCanvas: Call with importMap, importStyleUrls
initCanvas->>Env: Check development mode
alt Development mode
initCanvas->>Script: Inject script tag with src attribute
else Non-development mode
initCanvas->>Script: Inject inline script with $ replaced by $$
end
initCanvas-->>App: Return constructed HTML string
Poem
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/externalAllDepsFromCore
branch
from
3624474 to
da90453
Compare
chilingling
force-pushed
the
feat/externalAllDepsFromCore
branch
from
da90453 to
d986aa0
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: 1
🧹 Nitpick comments (1)
packages/layout/vite.config.ts (1)
35-35: Consider consistent externalization pattern across packagesWhile this change is good, I notice that different packages are externalizing different dependencies. Consider standardizing a common set of core dependencies that should be externalized across all packages to ensure consistency.
For example, if '@vueuse/core' is being externalized here, should it also be externalized in other packages where it might be used?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/canvas/init-canvas/init-canvas.ts(1 hunks)packages/canvas/scripts/vite-plugin-separate-build.ts(2 hunks)packages/canvas/vite.config.ts(1 hunks)packages/common/vite.config.ts(1 hunks)packages/design-core/vite.config.js(1 hunks)packages/layout/vite.config.ts(1 hunks)packages/plugins/block/vite.config.ts(1 hunks)packages/plugins/datasource/vite.config.js(1 hunks)packages/plugins/i18n/vite.config.ts(1 hunks)packages/plugins/schema/vite.config.ts(1 hunks)packages/plugins/tutorial/vite.config.ts(1 hunks)packages/register/vite.config.ts(1 hunks)packages/settings/props/vite.config.ts(1 hunks)packages/settings/styles/vite.config.ts(1 hunks)packages/toolbars/lang/vite.config.ts(1 hunks)packages/toolbars/view-setting/vite.config.ts(1 hunks)packages/utils/vite.config.ts(1 hunks)packages/vue-generator/vite.config.js(1 hunks)packages/webcomponent/vite.config.js(1 hunks)
🔇 Additional comments (20)
packages/settings/props/vite.config.ts (1)
35-35: Good externalization of @vueuse/core dependency.Adding
'@vueuse/core'to the externals list is consistent with the PR's objective to externalize all core package dependencies. This ensures the package won't bundle @vueuse/core, reducing the final bundle size and preventing duplicate code when multiple packages use the same dependency.packages/plugins/schema/vite.config.ts (1)
35-35: Good externalization of @vueuse/core dependency.Adding
'@vueuse/core'to the externals list for this package maintains consistency with the other package configurations and aligns with the PR's goal of externalizing all core dependencies.packages/toolbars/view-setting/vite.config.ts (1)
32-32: Good externalization of @vueuse/core dependency.Adding
'@vueuse/core'to the externals list is consistent with the other packages and properly supports the PR's objective to externalize all core package dependencies.packages/utils/vite.config.ts (1)
28-28: Good addition of jszip as external dependencyAdding 'jszip' to the external dependencies list is consistent with the PR objective to externalize all core package dependencies. This ensures that jszip isn't bundled with the utils package, which should lead to smaller bundle sizes.
This change is part of a breaking change as noted in the PR objectives, so consumers of this package will now need to provide jszip themselves.
packages/register/vite.config.ts (1)
29-29: Correctly externalizing lodash-esAdding 'lodash-es' to the external dependencies is appropriate and aligns with the PR objective. Lodash is a commonly used utility library that's better served as an external dependency to avoid duplication across packages.
Note that this change is part of a breaking change as mentioned in the PR objectives, so consumers will now need to provide lodash-es themselves.
packages/plugins/tutorial/vite.config.ts (1)
35-35: Good addition of marked as external dependencyAdding 'marked' to the external dependencies list is consistent with the PR's goal to externalize all core package dependencies. This markdown parser library is commonly used and makes sense as an external dependency.
This is part of the breaking change mentioned in the PR objectives, so consumers will now need to ensure the 'marked' package is available.
packages/layout/vite.config.ts (1)
35-35: Correctly externalizing @vueuse/core and vue-draggable-nextAdding '@vueuse/core' and 'vue-draggable-next' to the external dependencies list is consistent with the PR objective. These are commonly used Vue ecosystem libraries that are sensible choices for externalization.
As part of the breaking change noted in the PR, consumers will need to provide these dependencies themselves going forward.
packages/plugins/i18n/vite.config.ts (1)
35-35: LGTM! New external dependency is properly added.Adding 'vue-clipboard3' to the external dependencies list is consistent with the PR objective of externalizing all dependencies of the core package. This will prevent bundling this dependency, reducing the package size and avoiding potential duplicate dependencies in the final application.
Just ensure that 'vue-clipboard3' is properly documented as a peer dependency in the package.json file.
packages/plugins/block/vite.config.ts (1)
35-35: LGTM! External dependencies properly expanded.Adding '@vue/shared', 'html2canvas', and 'pinyin-pro' to the external dependencies list is aligned with the PR objective of externalizing core package dependencies. This approach reduces bundle size and gives consumers more flexibility in managing dependency versions.
Ensure these newly externalized dependencies are documented as peer dependencies in the package.json file.
packages/plugins/datasource/vite.config.js (1)
35-43: LGTM! External dependencies reformatted and expanded.The reformatted and expanded external dependencies list with additions of '@vue/shared', 'axios', 'vue-clipboard3', and 'xlsx' improves readability and aligns with the PR objective. Externalizing these dependencies provides better separation of concerns and more flexibility for consumers of this package.
Remember to update the package.json to include these dependencies as peer dependencies with appropriate version constraints.
packages/vue-generator/vite.config.js (1)
47-55: LGTM! Comprehensive external dependencies configuration.The expanded external dependencies list now properly includes Vue packages and namespace patterns. Using regex patterns for scoped packages (
/@vue\/.*/and/@opentiny\/tiny-engine.*/) is a good approach that avoids listing individual packages under these namespaces.This change will significantly impact how this package is consumed, so ensure that:
- These externalized dependencies are documented as peer dependencies in package.json
- Documentation is updated to inform consumers about these new external requirements
packages/toolbars/lang/vite.config.ts (1)
35-35: External dependency '@vueuse/core' added correctly.Adding '@vueuse/core' to the external dependencies list aligns with the PR objective of externalizing all dependencies of the core package. This ensures '@vueuse/core' is not bundled with the output, reducing bundle size and avoiding potential duplicate dependencies.
packages/settings/styles/vite.config.ts (1)
35-35: External dependencies '@vueuse/core' and 'postcss' added correctly.Adding '@vueuse/core' and 'postcss' to the external dependencies list ensures these modules won't be bundled with the output. This change is consistent with the PR objective of externalizing all dependencies of the core package.
packages/common/vite.config.ts (1)
69-76: External dependencies expansion looks good.The additions to the external dependencies list are comprehensive and well-organized:
- Added regex patterns for '@babel.', '@eslint.', '@vueuse.', and '@VUE.' packages
- Added specific modules: 'axios', 'css-tree', 'eslint-linter-browserify', and 'vue-draggable-next'
This implementation correctly fulfills the PR objective of externalizing all dependencies.
packages/canvas/vite.config.ts (1)
54-66: External dependencies structured clearly and comprehensively.The external dependencies array has been well-structured with:
- Core Vue-related packages ('vue', '@vueuse/core', 'vue-i18n')
- Organization-specific packages via regex patterns (OpenTiny packages)
- Tooling-related packages ('@babel.', '@VUE.')
- Specific utility packages ('diff-match-patch', 'jsondiffpatch', 'postcss', 'postcss-selector-parser')
This comprehensive externalization aligns with the PR objective and follows a consistent pattern with other package configurations.
packages/design-core/vite.config.js (1)
82-98: Expanded external dependencies list aligns with PR objectiveThe updated
externalarray in rollup options now includes a comprehensive list of modules to be treated as external during bundling. This change properly externalizes all dependencies of the core package as intended in the PR objectives.The externalization strategy uses both specific module names and regex patterns, which is an effective approach to cover related packages under common namespaces.
packages/canvas/scripts/vite-plugin-separate-build.ts (2)
8-8: Type annotation added for improved type safetyAdding the string type annotation to the
cleanUrlparameter enhances code readability and type safety.
91-92: Simplified code export formatThe previous implementation that encoded bundled code as a base64 data URI has been replaced with a direct JSON-stringified export. This simplification:
- Makes debugging easier since the code is no longer base64-encoded
- Coordinates with the changes in the canvas initialization that conditionally injects scripts
- Aligns with the overall externalization strategy in the PR
This change streamlines how bundled code is included in the build output.
packages/canvas/init-canvas/init-canvas.ts (2)
5-5: Improved code structure with local variableUsing a local variable
resbefore returning improves code structure and readability without changing functionality.Also applies to: 20-21
12-18: Environment-specific script loading strategyThe implementation now uses different script embedding strategies based on environment:
- Development: External script tag with src attribute
- Production: Inline script with properly escaped content
This approach optimizes for both development (easier debugging) and production (fewer HTTP requests) while properly handling escaping issues by doubling
$characters in the inline script.This change works well with the simplified export format in the vite-plugin-separate-build.ts file.
| @@ -32,7 +32,7 @@ export default defineConfig({ | |||
| }, | |||
| rollupOptions: { | |||
| // 确保外部化处理那些你不想打包进库的依赖 | |||
| external: ['vue', 'vue-i18n'], | |||
| external: ['vue', 'vue-i18n', '@vue/shared'], | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good externalization of @vue/shared dependency.
Adding '@vue/shared' to the externals list is appropriate for this package. However, I notice that you've updated the globals mapping for 'vue' and 'vue-i18n' but not for '@vue/shared'.
Consider whether you need to add '@vue/shared' to the globals object for UMD builds:
🏁 Script executed:
#!/bin/bash
# Check if @vue/shared is used in UMD builds and needs a global variable mapping
# Search for imports of @vue/shared in the webcomponent package
rg -A 2 "from ['\"]\@vue/shared['\"]" packages/webcomponent/Length of output: 167
I’ll check the Vite config and scan for any actual imports of @vue/shared before deciding if a UMD global is needed.
🏁 Script executed:
#!/bin/bash
set -e
echo "1) Locate Vite config for webcomponent package"
rg -l "vite.config.js"
echo -e "\n2) Show externals & globals in vite.config.js"
sed -n '1,200p' packages/webcomponent/vite.config.js | sed -n '30,60p'
echo -e "\n3) Find any usage of @vue/shared in source files"
rg -n "@vue/shared" packages/webcomponentLength of output: 1407
Add missing UMD global for @vue/shared
The webcomponent bundle imports utilities from @vue/shared (e.g. in src/defineCustomElement.js), and you’ve externalized it in Rollup, but you haven’t provided a corresponding global in the UMD build. This will lead to a runtime “undefined” when consumers use the UMD script.
• File: packages/webcomponent/vite.config.js
• Location: under rollupOptions.output.globals
Suggested diff:
output: {
globals: {
vue: 'Vue',
'vue-i18n': 'VueI18n',
+ '@vue/shared': 'VueShared'
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| external: ['vue', 'vue-i18n', '@vue/shared'], | |
| // packages/webcomponent/vite.config.js | |
| export default defineConfig({ | |
| build: { | |
| rollupOptions: { | |
| external: ['vue', 'vue-i18n', '@vue/shared'], | |
| output: { | |
| globals: { | |
| vue: 'Vue', | |
| 'vue-i18n': 'VueI18n', | |
| '@vue/shared': 'VueShared' | |
| } | |
| } | |
| } | |
| } | |
| }) |
| import.meta.env.MODE === 'development' | ||
| ? `<script type="module" src="${canvasScript}"></script>` | ||
| : // 将 $ 替换为 $$，然后在上一层的 $$ 再转义回来 $，避免被转义 | ||
| `<script type="module">${canvasScript.replace(/\$/g, '$$$$')}</script>` |
There was a problem hiding this comment.
String.prototype.replace 会将字符串里的 $1、$2、... $n、$$、$&、$\``、$'、 $` 等转义。
| 模式 | 插入值 |
|---|---|
$$ |
插入一个 "$"。 |
$& |
插入匹配的子字符串。 |
| `$`` | 插入匹配子字符串之前的字符串片段。 |
$' |
插入匹配子字符串之后的字符串片段。 |
$n |
插入名称为 Name 的命名捕获组。 |
但是我们不希望源码里的 $x 被当做特殊的替换模式。所以我们将源码里的所有 $ 符号替换成 $$。即：
canvasScript.replace(/\$/g, '$$$$')然后在上一层的替换中，将 $$ 转换回来正常的 $ 符号，从而避免被当前特殊模式替换。
// 这一层 replace 将 `$$` 转换为原来的 `$`
.replace(
'<!--%MAIN_SCRIPT%-->',
import.meta.env.MODE === 'development'
? `<script type="module" src="${canvasScript}"></script>`
// 这一层将 `$` 转换为 `$$`
: `<script type="module">${canvasScript.replace(/\$/g, '$$$$')}</script>`
)
| @@ -32,7 +32,7 @@ export default defineConfig({ | |||
| }, | |||
chilingling
force-pushed
the
feat/externalAllDepsFromCore
branch
from
d986aa0 to
50e44f1
Compare
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
Refactor
Chores