feat: [select]reduce common package size #2577
Conversation
WalkthroughThis pull request introduces several modifications primarily focused on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
zzcr
changed the title
![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
🧹 Outside diff range and nitpick comments (7)
packages/vue-common/src/index-dev.ts (2)
4-4: Translate Chinese comment to EnglishThe Chinese comment "动态注入twMerge，保证本地联调" should be translated to English for better maintainability.
-// 动态注入twMerge，保证本地联调 +// Dynamically inject twMerge for local development and testing
1-5: Document development vs production behavior differencesThe separation between development and production behavior needs to be clearly documented to prevent confusion.
Consider:
- Adding a comment block explaining why this dev-specific file exists
- Documenting how twMerge behaves differently in dev vs prod
- Adding a warning if this file is accidentally imported in production
+/** + * Development-only entry point for @opentiny/vue-common + * + * This file includes development-specific configurations and utilities, + * particularly the dynamic injection of twMerge for local testing. + * DO NOT USE THIS FILE IN PRODUCTION. + */ import { customDesignConfig } from './index'packages/vue-saas-common/src/index.ts (1)
8-9: Document the design configurationThe comment "自定义主题交互规范" should be in English for consistency and clarity. Also, consider documenting the purpose of adding
twMergeto the configuration.Consider updating the comment:
-// 自定义主题交互规范 +// Custom theme interaction specifications +// twMerge is used for merging Tailwind CSS classes in a safe waypackages/vue-common/src/index.ts (4)
162-168: Add JSDoc documentation for the CustomDesignConfig interfaceThe new
twMergeproperty would benefit from documentation explaining its purpose and usage. Consider adding TypeScript documentation:interface CustomDesignConfig { designConfig: null | DesignConfig + /** Function to merge Tailwind CSS classes. If not configured, returns empty string */ twMerge: (str: string) => string }
206-207: Document the shorthand methodsConsider adding comments to explain the shorthand methods:
- gcls: (key) => getElementCssClass(classes, key), - m: mergeClass + /** Get element CSS class by key */ + gcls: (key) => getElementCssClass(classes, key), + /** Merge CSS classes with optional Tailwind class merging */ + m: mergeClass
271-273: Consider extracting default classes to constantsThe hardcoded Tailwind classes could be moved to constants for better maintainability:
+const DEFAULT_SVG_CLASSES = 'h-4 w-4 inline-block' + if (isMobileFirst) { - className = mergeClass('h-4 w-4 inline-block', customClass || '', mergeProps.class || '') + className = mergeClass(DEFAULT_SVG_CLASSES, customClass || '', mergeProps.class || '') }
Line range hint
162-273: Well-architected changes for package size reductionThe changes effectively make the Tailwind integration optional while maintaining a clean API. This aligns well with the PR's goal of reducing the common package size. The implementation:
- Makes Tailwind integration optional through
customDesignConfig- Provides consistent API through the
mergeClassfunction- Maintains backward compatibility
- Supports mobile-first responsive design
Consider documenting the following in the README:
- How to configure
customDesignConfig.twMergefor Tailwind integration- Performance implications of enabling/disabling Tailwind integration
- Migration guide for existing users
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
internals/cli/src/config/vite.ts(1 hunks)packages/vue-common/package.json(1 hunks)packages/vue-common/src/index-dev.ts(1 hunks)packages/vue-common/src/index.ts(3 hunks)packages/vue-saas-common/package.json(1 hunks)packages/vue-saas-common/src/index.ts(1 hunks)
🔇 Additional comments (8)
packages/vue-common/src/index-dev.ts (2)
5-5: Verify twMerge availability in production builds
Since twMerge is dynamically injected only in development, we need to ensure it's properly handled in production.
Let's check how twMerge is used across the codebase:
✅ Verification successful
Let me analyze the usage of twMerge across the codebase by checking its implementation and how it's used in production builds.
Based on the code analysis, I can now provide the final response:
twMerge implementation is properly handled in both development and production
The implementation shows a well-designed pattern where:
- In production (
index.ts),customDesignConfig.twMergedefaults to a no-op function that returns an empty string - In development (
index-dev.ts),twMergefrom 'tailwind-merge' is dynamically injected - The
mergeClassutility function safely uses this configuration in both environments
This pattern ensures that:
- Development builds have full Tailwind class merging capabilities
- Production builds have a safe fallback that won't break functionality
- The implementation is properly encapsulated within the design system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify twMerge usage and availability
# Find all files that use twMerge or customDesignConfig.twMerge
echo "Files using twMerge:"
rg -l "twMerge|customDesignConfig\.twMerge"
# Check if there's a production fallback
echo "Production entry point usage:"
rg -l "from.*?@opentiny/vue-common"
Length of output: 53790
Script:
#!/bin/bash
# Check the implementation and usage of twMerge in both dev and production files
# Check index.ts and index-dev.ts content
echo "Content of index.ts:"
cat packages/vue-common/src/index.ts
echo -e "\nContent of index-dev.ts:"
cat packages/vue-common/src/index-dev.ts
# Check if twMerge is used in production code
echo -e "\nUsage of twMerge in production code:"
rg "twMerge" -A 3 -B 3 packages/vue-common/src/index.ts
Length of output: 12724
1-3: Verify the impact on bundle size
While separating the dev entry point could help reduce the production bundle size, we should verify this optimization.
Let's analyze the bundle size impact:
✅ Verification successful
Based on the analysis of the code, I can now generate the final response:
Optimization is correctly implemented for bundle size reduction
The separation of tailwind-merge into a development-only entry point is a valid optimization:
-
Production build (
index.ts):- Defines
customDesignConfig.twMergeas an empty function() => '' - Keeps the interface but with minimal implementation
- Defines
-
Development build (
index-dev.ts):- Imports and injects the actual
twMergefunction only during development - Helps with local testing while keeping production bundle lean
- Imports and injects the actual
This approach effectively reduces the production bundle size by excluding the tailwind-merge dependency from production builds while maintaining development functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the size of index.ts vs index-dev.ts and their dependencies
# Get size of both files and their direct imports
echo "Size of index-dev.ts and its imports:"
find packages/vue-common/src -name "index-dev.ts" -o -name "index.ts" | xargs wc -l
# Check if tailwind-merge is listed as a dependency or devDependency
find . -name "package.json" -exec grep -l "tailwind-merge" {} \;
Length of output: 692
Script:
#!/bin/bash
# Let's analyze the tailwind-merge dependency configuration and usage
# Check package.json files to see if tailwind-merge is in dependencies or devDependencies
echo "=== Checking tailwind-merge dependency type ==="
for pkg in $(find . -name "package.json"); do
echo "=== $pkg ==="
cat "$pkg" | grep -A 2 -B 2 "tailwind-merge"
done
# Check where tailwind-merge is actually imported/used
echo -e "\n=== Files importing tailwind-merge ==="
rg "import.*tailwind-merge" -l
# Check if customDesignConfig is used in production builds
echo -e "\n=== Usage of customDesignConfig ==="
rg "customDesignConfig" -l
Length of output: 156424
Script:
#!/bin/bash
# Let's analyze how tailwind-merge and customDesignConfig are used in the codebase
# Check the implementation and usage of customDesignConfig
echo "=== customDesignConfig implementation ==="
rg -A 5 "export.*customDesignConfig"
# Check how tailwind-merge is used in production vs development
echo -e "\n=== tailwind-merge usage in index-dev.ts ==="
cat packages/vue-common/src/index-dev.ts
echo -e "\n=== tailwind-merge usage in main index.ts ==="
cat packages/vue-common/src/index.ts
Length of output: 13577
packages/vue-saas-common/package.json (1)
32-33: Review impact on package size
Adding new dependencies seems contrary to the PR's objective of reducing the common package size:
tailwind-mergeadds ~8KB to the bundle- Additional workspace dependency
@opentiny/vue-design-saascould further increase size
Could you explain how these additions align with the goal of reducing package size? Consider:
- Is
tailwind-mergeessential, or could we implement a simpler solution? - Can the functionality from
@opentiny/vue-design-saasbe optimized or partially imported?
packages/vue-common/package.json (2)
27-27: Verify the development entry point configuration
The main entry point has been changed to src/index-dev.ts. This naming suggests it's intended for development purposes, which might not be appropriate for the main entry point in a production package.
Let's verify the entry point configuration and its impact:
31-35: Analyze package size reduction impact
The removal of @opentiny/vue-theme-mobile aligns with the PR's objective to reduce package size. However, we should verify if the addition of tailwind-merge doesn't offset these savings.
Let's analyze the package size impact:
internals/cli/src/config/vite.ts (1)
29-29: LGTM! Verify production builds.
The change to use index-dev as the entry point aligns with the PR objective to reduce package size by potentially splitting development and production code. However, we should verify that this doesn't affect production builds.
Let's verify the production configuration:
✅ Verification successful
The development-only entry point is safe and working as intended
The change is verified to be correct and safe because:
index-dev.tsis a thin wrapper around the mainindex.tsthat only adds development-specifictailwind-mergefunctionality- It exports everything from the main
index.tsusingexport * from './index' - The only additional code is for injecting
twMergefor local development purposes - The main package entry point in
package.jsonpoints tolib/index.js, which means production builds will use the compiled version without the dev-specific code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for corresponding production entry point and build configuration
# Check if there's a corresponding production entry point
echo "Checking for production entry point..."
fd "index(\.|-)prod" "packages/vue-common/src"
# Check if the package.json has different entry points for production
echo "Checking package.json configuration..."
fd "package.json" "packages/vue-common" --exec cat {} | grep -A 5 '"main":'
# Look for build configuration that might handle dev/prod differently
echo "Checking build configuration..."
rg -A 5 "vue-common.*((prod)|(build))" "packages" "internals"
Length of output: 965
Script:
#!/bin/bash
# Let's check the content of index-dev.ts and related files to understand the dev/prod split
echo "Checking index-dev.ts content..."
fd "index-dev.ts" "packages/vue-common/src" --exec cat {}
echo "\nChecking for other index files..."
fd "^index" "packages/vue-common/src" --exec ls -la {}
echo "\nChecking for build configuration in vite config..."
rg -A 5 "vue-common" "internals/cli/src/config"
echo "\nChecking for any mode-specific configurations..."
rg -A 5 "process.env.(NODE_ENV|MODE)" "packages/vue-common"
Length of output: 2416
packages/vue-common/src/index.ts (2)
171-171: LGTM: Clean implementation of mergeClass
The implementation elegantly combines stringifyCssClass with the configurable twMerge function, making the Tailwind class merging functionality optional.
186-187: Verify the usage of newly added utilities
The addition of useBreakpoint and mergeClass to the utils object makes them available in the component context. Let's verify their usage across components.
✅ Verification successful
Both utilities are actively used across the codebase
The verification shows that both utilities are properly implemented and actively used:
useBreakpointis used in multiple components including grid, cascader, upload, select, and dialog-box for responsive behaviormergeClassis utilized in various components like tooltip, grid, and form-item for class name management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of useBreakpoint and mergeClass in components
echo "Searching for useBreakpoint usage:"
rg "useBreakpoint" --type ts --type vue
echo -e "\nSearching for mergeClass usage:"
rg "mergeClass" --type ts --type vue
Length of output: 290
Script:
#!/bin/bash
# Search for usage of useBreakpoint and mergeClass with correct file extensions
echo "Searching for useBreakpoint usage:"
rg "useBreakpoint" -t typescript -t vue-template
echo -e "\nSearching for mergeClass usage:"
rg "mergeClass" -t typescript -t vue-template
# Also search in the implementation files to verify they exist
echo -e "\nChecking implementation files:"
fd -e ts -e vue "useBreakpoint|mergeClass" packages/
Length of output: 514
Script:
#!/bin/bash
# Search with simpler ripgrep options and check the implementation
echo "Searching for useBreakpoint usage:"
rg "useBreakpoint"
echo -e "\nSearching for mergeClass usage:"
rg "mergeClass"
echo -e "\nChecking mergeClassnames implementation:"
cat packages/vue/src/query-builder/src/utils/mergeClassnames.ts
Length of output: 10384
| import designSaasConfig from '@opentiny/vue-design-saas' | ||
|
|
||
| export * from '@opentiny/vue-common' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review re-export strategy
Re-exporting everything from @opentiny/vue-common might include unnecessary exports. Consider being more selective about what's re-exported to reduce the final bundle size.
Consider replacing the wildcard export with specific exports:
-export * from '@opentiny/vue-common'
+export {
+ // List only the necessary exports
+ customDesignConfig,
+ // ... other required exports
+} from '@opentiny/vue-common'Committable suggestion skipped: line range outside the PR's diff.
| @@ -1,6 +1,9 @@ | |||
| import { customDesignConfig } from '@opentiny/vue-common' | |||
|
|
|||
| import { twMerge } from 'tailwind-merge' | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide a final response:
Reconsider the tailwind-merge implementation strategy
The analysis reveals several concerns:
tailwind-mergeis not tree-shakeable and adds ~7KB gzipped to the bundle- It's being used across multiple framework implementations (Vue, React, OpenInula) for a common
mergeClassutility - There are existing class merging utilities in the codebase (clsx, classNames) that could be leveraged
Recommendations:
- Move the class merging logic to a shared internal utility using existing lighter alternatives like
clsx - If
tailwind-mergespecific features are required, implement it as an optional peer dependency - Consider creating a minimal implementation that only includes the merge rules you actually need
🔗 Analysis chain
Consider impact on package size when adding tailwind-merge
Adding tailwind-merge as a dependency seems to contradict the PR's objective of reducing the common package size. While this utility is useful for merging Tailwind CSS classes, we should verify its impact on the final bundle size.
Let's analyze the impact:
Consider these alternatives to minimize bundle size:
- Lazy load tailwind-merge only when needed
- Create a minimal version with only required merge rules
- Consider moving this to an optional peer dependency if not widely used
Also applies to: 9-9
Let me gather more information about how twMerge is actually used in these files to better understand its necessity and implementation.
This will help us understand:
- How twMerge is being used across different packages
- If there are any existing class merging utilities that could be used instead
- Whether the implementation is consistent across different frameworks (Vue, React, OpenInula)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the size of tailwind-merge package
curl -s https://bundlephobia.com/api/size?package=tailwind-merge | jq '{
size: .size,
gzip: .gzip,
treeshaking: .hasJSModule,
dependencies: .dependencyCount
}'
# Check if twMerge is actually used in the codebase
rg -l "twMerge" --type ts
Length of output: 468
Script:
#!/bin/bash
# Check the actual usage of twMerge in the files
for file in $(rg -l "twMerge" --type ts); do
echo "=== $file ==="
rg "twMerge" -B 2 -A 2 "$file"
done
# Check if there are any existing class merging utilities
rg -l "classNames|clsx|cls|mergeClasses" --type ts
Length of output: 4759
| @@ -29,7 +29,8 @@ | |||
| "type": "module", | |||
| "dependencies": { | |||
| "@opentiny/vue-common": "workspace:~", | |||
| "@opentiny/vue-design-saas": "workspace:~" | |||
| "@opentiny/vue-design-saas": "workspace:~", | |||
| "tailwind-merge": "^1.8.0" | |||
There was a problem hiding this comment.
💡 Codebase verification
Update tailwind-merge to the latest version 2.5.5
The current version (1.8.0) is significantly outdated compared to the latest version (2.5.5). While no security vulnerabilities were found, it's recommended to update to benefit from the latest improvements and bug fixes.
- Update the dependency in
packages/vue-saas-common/package.json:"tailwind-merge": "^2.5.5"
🔗 Analysis chain
Verify tailwind-merge version and security
The package uses tailwind-merge@^1.8.0. Let's ensure this version is secure and up-to-date.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for latest version and security advisories
curl -s https://registry.npmjs.org/tailwind-merge/latest | jq '.version'
# Check for known vulnerabilities
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "tailwind-merge") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 428
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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
@opentiny/vue-commonto enhance local development capabilities.twMergefunctionality for improved Tailwind CSS utility class management.twMergein thecustomDesignConfigfor bothvue-commonandvue-saas-commonpackages.Bug Fixes
mergeClassfunction to ensure proper integration with the newtwMergemethod.Chores
tailwind-mergefor enhanced styling capabilities.