Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [grid,alert,form]use catalog to globally upgrade vite and vitest; and add mobile build config #2680

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

zzcr
Copy link
Member

@zzcr zzcr commented Dec 19, 2024

PR

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our Commit Message Guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new TypeScript configuration files for mobile package to enhance compilation settings.
    • Added a Vite configuration file to streamline the build process for modern web projects.
  • Updates

    • Updated build script for mobile package to utilize Vite.
    • Enhanced development environment with new dependencies for TypeScript and Vite integration.
  • Bug Fixes

    • Corrected formatting of the vue3 dependency in package declarations.
  • Dependency Changes

    • Updated version specifications for several development dependencies across multiple packages.

Copy link

coderabbitai bot commented Dec 19, 2024

Walkthrough

This pull request involves multiple configuration and dependency updates across various packages in the project. The changes primarily focus on updating development dependencies, modifying build configurations, and introducing new TypeScript and Vite configurations for the mobile package. The modifications include updating package.json files, introducing new configuration files like tsconfig.json and vite.config.ts, and making minor adjustments to existing code, such as changing a property declaration in the SHA256 hashing utility.

Changes

File Change Summary
internals/cli/package.json Updated vite dependency version from catalog reference to ^4.3.8
internals/unplugin-virtual-template/tsup.config.ts Disabled TypeScript declaration file generation by setting dts to false
package.json Corrected formatting of vue3 dependency in pnpm.overrides
packages/mobile/package.json Changed build script to use Vite, added new development dependencies
packages/mobile/tsconfig.json Added new TypeScript configuration with strict compilation options
packages/mobile/tsconfig.node.json Added new Node-specific TypeScript configuration
packages/mobile/vite.config.ts Added Vite configuration for mobile package build process
packages/utils/package.json Updated typescript and vite dependencies to catalog references
packages/utils/src/crypt/sha256.ts Changed _data property declaration to use declare
pnpm-workspace.yaml Updated versions for vitest and vite

Possibly related PRs

Suggested labels

enhancement

Poem

🐰 Hop, hop, through configs we go,
Vite and TypeScript in a coding flow
Dependencies dance, versions align
Build tools sparkle, everything's fine!
A rabbit's update, smooth and bright 🚀


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the enhancement New feature or request (功能增强) label Dec 19, 2024
@zzcr zzcr changed the title feat: use catalog to globally upgrade vite and vitest; and add mobile build config feat: [grid,alert,form]use catalog to globally upgrade vite and vitest; and add mobile build config Dec 19, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
packages/mobile/package.json (1)

28-28: Consider enhancing build script configuration

The build script has been simplified to "vite build" but might benefit from additional configuration options for different environments or build modes.

Consider expanding the build script:

-    "build": "vite build"
+    "build": "vite build",
+    "build:dev": "vite build --mode development",
+    "build:prod": "vite build --mode production"
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1bc3a1e and f549494.

📒 Files selected for processing (10)
  • internals/cli/package.json (1 hunks)
  • internals/unplugin-virtual-template/tsup.config.ts (1 hunks)
  • package.json (1 hunks)
  • packages/mobile/package.json (2 hunks)
  • packages/mobile/tsconfig.json (1 hunks)
  • packages/mobile/tsconfig.node.json (1 hunks)
  • packages/mobile/vite.config.ts (1 hunks)
  • packages/utils/package.json (1 hunks)
  • packages/utils/src/crypt/sha256.ts (1 hunks)
  • pnpm-workspace.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (3)
  • packages/mobile/tsconfig.node.json
  • packages/mobile/tsconfig.json
  • package.json
🔇 Additional comments (7)
internals/unplugin-virtual-template/tsup.config.ts (1)

7-7: Verify impact of disabled TypeScript declarations

Disabling dts generation means TypeScript declaration files (*.d.ts) won't be generated. This could impact TypeScript users of this package.

Let's check the impact:

Please clarify:

  1. Why was type declaration generation disabled?
  2. How will TypeScript support be maintained?
packages/utils/package.json (1)

24-26: Verify vite-plugin-dts compatibility with Vite 6

While moving to catalog versions for typescript and vite is good for standardization, please verify that [email protected] is compatible with Vite 6.

Let's check compatibility:

Consider:

  1. Updating vite-plugin-dts to a newer version that officially supports Vite 6
  2. Adding vite-plugin-dts to the catalog for consistent versioning
✅ Verification successful

vite-plugin-dts is compatible with Vite 6

The verification shows that vite-plugin-dts has no strict version constraints for Vite in its peer dependencies (accepts any version with *). The package version 4.3.0 should work fine with Vite 6. Additionally, there are other packages in the monorepo already using vite-plugin-dts v4.3.0 successfully.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check vite-plugin-dts version constraints
echo "Checking vite-plugin-dts peer dependencies..."
curl -s https://registry.npmjs.org/vite-plugin-dts/4.3.0 | jq '.peerDependencies'

# Look for any existing issues with this combination
echo "Checking for potential version conflicts..."
rg -g "package.json" "\"vite-plugin-dts\""

Length of output: 582


Script:

#!/bin/bash
# Let's check the latest version of vite-plugin-dts and its compatibility
echo "Checking latest vite-plugin-dts version and its peer dependencies..."
curl -s https://registry.npmjs.org/vite-plugin-dts/latest | jq '{version, peerDependencies}'

# Let's also check what version of Vite is being used
echo "Checking Vite version in catalog..."
cat package.json | jq '.catalog.vite'

Length of output: 458

packages/mobile/vite.config.ts (2)

11-13: Verify entry point existence and exports

The build configuration specifies './index.ts' as the entry point. Let's ensure this file exists and exports all necessary components.

✅ Verification successful

Entry point verified and exports confirmed

The entry point ./index.ts exists and properly exports all necessary components. The file contains:

  • A version export
  • All UI components with both their original names and prefixed variants (e.g., Button and TinyButton)
  • Proper imports from the components directory
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if index.ts exists and examine its exports
if [ -f "packages/mobile/index.ts" ]; then
    echo "Entry point exists. Checking exports:"
    rg -A 5 "export \{.*\}" "packages/mobile/index.ts"
else
    echo "Warning: Entry point 'packages/mobile/index.ts' not found"
fi

Length of output: 173


Script:

#!/bin/bash
# Let's check the content of index.ts and any export statements with different patterns
echo "Checking index.ts content:"
cat packages/mobile/index.ts

echo -e "\nSearching for export statements with different patterns:"
rg "^export" packages/mobile/index.ts

echo -e "\nChecking if there might be a .js variant:"
[ -f "packages/mobile/index.js" ] && echo "index.js exists" || echo "No index.js found"

echo -e "\nListing files in packages/mobile to verify entry point location:"
ls -la packages/mobile/

Length of output: 5677


16-16: Review external dependencies pattern

The current external dependencies pattern /@better-scroll/, /@opentiny/ might be too broad. Consider being more specific to avoid accidentally excluding required dependencies.

Let's verify the impact:

✅ Verification successful

Based on the search results, I can see that @better-scroll and @opentiny dependencies are used across multiple files in the codebase. Let me analyze the specific usage patterns to determine if the external pattern is appropriate.

This will help us understand the specific import patterns and determine if the external configuration needs to be more granular.


Let me modify the search to work without file type restrictions since the Vue file type isn't recognized.

This should show us the actual import statements being used for these dependencies.


Based on the analysis of the import patterns, I can now provide a definitive response about the external dependencies configuration:

The external dependencies pattern is appropriate and should be kept as is

The current external pattern /@better-scroll/, /@opentiny/ is correctly configured because:

  1. @better-scroll imports:

    • Used consistently in wheel and action-sheet components
    • Only specific modules are imported (core and wheel)
    • Used in both mobile and vue packages
  2. @opentiny imports:

    • Used extensively throughout the codebase
    • Includes various submodules (vue-common, vue-renderless, vue-theme, etc.)
    • Forms the core framework of the application

The pattern is neither too broad nor restrictive because:

  • It correctly excludes these dependencies from the bundle while allowing them to be resolved at runtime
  • Both patterns match the actual import paths used in the codebase
  • The patterns are specific to the vendor prefixes and won't accidentally exclude other dependencies
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for all @better-scroll and @opentiny imports in the codebase
echo "Checking @better-scroll imports:"
rg -l '@better-scroll' 

echo "\nChecking @opentiny imports:"
rg -l '@opentiny'

Length of output: 65670


Script:

#!/bin/bash
# Check actual import patterns for these dependencies
echo "Checking @better-scroll import patterns:"
rg -A 2 '@better-scroll' --type ts --type vue

echo "\nChecking @opentiny import patterns:"
rg -A 2 '@opentiny' --type ts --type vue

Length of output: 318


Script:

#!/bin/bash
# Check actual import patterns for these dependencies
echo "Checking @better-scroll import patterns:"
rg -A 2 'from.*@better-scroll|import.*@better-scroll'

echo "\nChecking @opentiny import patterns:"
rg -A 2 'from.*@opentiny|import.*@opentiny'

Length of output: 66043

internals/cli/package.json (1)

26-26: ⚠️ Potential issue

Address version inconsistency between Vite and its plugins

The Vite version (^4.3.8) is significantly older than the Vue plugins specified in the mobile package (@vitejs/plugin-vue@^5.2.1). This version mismatch could lead to compatibility issues.

Consider upgrading Vite to match the plugin requirements:

-    "vite": "^4.3.8",
+    "vite": "^5.0.0",

Let's check for any other Vite version references:

packages/utils/src/crypt/sha256.ts (2)

39-39: Verify no runtime behavior changes in SHA256 implementation

Although this is a TypeScript type-level change, please verify that the SHA256 hash outputs remain consistent after the upgrade.

Let's search for any additional changes to SHA256 implementation:

✅ Verification successful

No runtime behavior changes detected in SHA256 implementation

The only change is adding the declare keyword to the _data property in the SHA256Algo class, which is a TypeScript-only modification that gets removed during compilation. The core SHA256 algorithm implementation, including initialization vectors, round constants, and all computational logic remains completely unchanged. The property is still used in the same way within _doFinalize() method.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any other changes to SHA256 files
git diff --name-only HEAD~1 | grep -i sha256

# Search for usage patterns that might be affected
rg "new SHA256|SHA256\(" 

Length of output: 412


Script:

#!/bin/bash
# Let's examine the actual changes in the SHA256 implementation
git diff HEAD~1 packages/utils/src/crypt/sha256.ts

# Let's also check the full implementation to understand the context
cat packages/utils/src/crypt/sha256.ts

# Check the actual usage in the crypto index file
cat packages/utils/src/crypt/index.ts

Length of output: 4487


39-39: 🛠️ Refactor suggestion

Verify parent class initialization of _data property

While changing to declare is syntactically correct if the property is initialized in the parent class, using any type for cryptographic data reduces type safety. Consider using more specific types like WordArray or Uint8Array.

Let's verify the parent class initialization:

Consider replacing any with a more specific type:

- declare _data: any
+ declare _data: WordArray
✅ Verification successful

Property declaration aligns with parent class implementation

The declare _data: any matches the parent implementation in BufferedBlockAlgorithm class where _data is initialized as a WordArray in the reset() method and is typed as any. While the type could be more specific, this maintains consistency with the parent class implementation.

Key findings:

  • _data is properly initialized in parent class's reset() method: this._data = new WordArray()
  • The property is used throughout the codebase with WordArray operations
  • The any type matches the parent class declaration
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Hasher class implementation and _data initialization
ast-grep --pattern 'class Hasher {
  $$$
  _data
  $$$
}'

Length of output: 59


Script:

#!/bin/bash
# Let's try a different approach to find the Hasher class and _data initialization
# First, let's find the file containing Hasher class
fd "hasher" --type f

# Then let's look for _data usage in crypto-related files
rg "_data" --type ts -C 3 packages/utils/src/crypt/

Length of output: 2856

Comment on lines +9 to +10
'vitest': '^2.1.8'
'vite': '^6.0.0'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Version conflicts detected - Migration needed in internal packages

The verification revealed version conflicts that need attention:

  • Found packages still using Vite 4.x:
    • internals/unplugin-tiny-vue/example/package.json
    • internals/cli/package.json

Additionally, multiple Vite and Vitest configuration files were found that may need updates:

  • 15 Vite config files across various packages
  • 3 Vitest config files in Vue examples

Please update the remaining Vite v4 dependencies and review all configuration files for compatibility with the new versions.

🔗 Analysis chain

Major version upgrades require careful verification

The changes include significant version jumps:

  • Vite: 4.3.8 → 6.0.0 (Major upgrade)
  • Vitest: 0.31.0 → 2.1.8 (Major upgrade)

These updates likely include breaking changes that need careful review.

Let's verify the package compatibility:

Please review:

  1. Vite Migration Guide from v4 to v6
  2. Vitest Migration Guide

Consider documenting the migration steps in the PR description to help with future upgrades.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if any package.json files still reference old versions
echo "Checking for potential version conflicts..."
rg -g "package.json" "\"vite\": \"(\^4|\~4)"
rg -g "package.json" "\"vitest\": \"(\^0|\~0)"

# Check for Vite/Vitest config files that might need updates
echo "Checking for config files that might need migration..."
fd "vite.config" 
fd "vitest.config"

Length of output: 1097

@@ -36,6 +35,12 @@
"@opentiny/vue-locale": "workspace:~"
},
"devDependencies": {
"vue": "^3.5.0"
"vue": "^3.5.0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect Vue version specification

The specified Vue version ^3.5.0 is incorrect as Vue 3.5.0 doesn't exist yet. The latest Vue 3.x version is lower than this.

Consider updating to a valid version:

-    "vue": "^3.5.0",
+    "vue": "^3.4.0",
📝 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.

Suggested change
"vue": "^3.5.0",
"vue": "^3.4.0",

@@ -36,7 +36,7 @@ while (mPrime < 64) {

export class SHA256Algo extends Hasher {
_hash: any
_data: any
declare _data: any
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Missing test coverage for critical cryptographic functionality

The SHA256 implementation lacks dedicated test coverage, which is concerning for cryptographic code. The codebase shows:

  • A complete SHA256 implementation in packages/utils/src/crypt/sha256.ts
  • Usage in packages/utils/src/crypt/index.ts with both WebCrypto and fallback paths
  • No corresponding test files found in the utils package

Key areas requiring test coverage:

  • SHA256Algo class implementation
  • Hash generation with different input types
  • WebCrypto API fallback logic
  • Edge cases and error handling
🔗 Analysis chain

Verify test coverage for SHA256 implementation

Given this is cryptographic code, ensure that the TypeScript upgrade hasn't impacted test coverage or existing test cases.

Let's check for test coverage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for SHA256 test files
fd -e test.ts -e spec.ts sha256

# Search for SHA256 test cases
rg -l "describe.*SHA256|test.*SHA256|it.*SHA256"

Length of output: 84


Script:

#!/bin/bash
# Let's try a broader search for test files related to SHA256
fd test packages/utils/src/crypt

# Search for any SHA256-related tests with more flexible patterns
rg -i "sha256" --type ts -A 5 packages/utils/src/crypt

# Check the actual SHA256 implementation file
cat packages/utils/src/crypt/sha256.ts

Length of output: 4914


Script:

#!/bin/bash
# Let's check for any test files in the parent directories
fd test packages/utils

# Look for any imports or usage of SHA256 in test files
rg -l "SHA256|sha256" $(fd -e test.ts -e spec.ts packages/utils)

# Check if there are any test utilities or setup files
fd -e ts "test-utils|test-setup|jest.setup" packages/utils

Length of output: 812


Script:

#!/bin/bash
# Let's try with correct fd syntax
fd . packages/utils --type f --extension test.ts --extension spec.ts

# Check for any test configuration files
fd jest.config packages/utils

# Look for SHA256 usage in the codebase to understand test coverage importance
rg -p "SHA256|sha256" packages/utils/src -C 3

Length of output: 1080

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request (功能增强)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants