-
Notifications
You must be signed in to change notification settings - Fork 7
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
chore: Remove a a bunch of old dependencies #1528
Conversation
WalkthroughThis pull request involves configuration updates across multiple files in the project. The changes primarily focus on modifying dependencies in Changes
Suggested reviewers
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
package.json (1)
Line range hint
1-256
: Add ts-node as a devDependency to fix the TypeScript CI pipeline.The TypeScript CI pipeline is failing due to a missing ts-node dependency, which is required for parsing TypeScript configuration files.
Add the following to devDependencies:
"devDependencies": { + "ts-node": "^10.9.2", // ... other devDependencies }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lock
is excluded by!**/*.lock
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (4)
config.ts
(1 hunks)jest.config.ts
(1 hunks)jest.setup.ts
(0 hunks)package.json
(3 hunks)
💤 Files with no reviewable changes (1)
- jest.setup.ts
🧰 Additional context used
🪛 GitHub Actions: Typescript CI
jest.config.ts
[error] Failed to parse TypeScript config file due to missing 'ts-node' package dependency. Install 'ts-node' to resolve this issue.
config.ts
[error] Failed to parse TypeScript config file due to missing 'ts-node' package dependency. Install 'ts-node' to resolve this issue.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
package.json (1)
188-195
: LGTM! Correctly moved Babel dependencies to devDependencies.Moving Babel-related packages to devDependencies is the right approach as these are build-time dependencies, not runtime dependencies.
config.ts (1)
4-4
: LGTM! Correctly migrated from wagmi to viem chains.The change aligns with the removal of the wagmi dependency, and the chain configurations remain functionally equivalent.
Let's verify that all wagmi imports have been migrated:
✅ Verification successful
Verified: All wagmi chain imports have been successfully migrated
The codebase shows consistent use of:
viem/chains
for base configurationthirdweb/chains
for wallet and ethereum-specific functionalityNo remaining wagmi imports were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no remaining wagmi chain imports rg "from ['\"]wagmi/chains['\"]"Length of output: 38
Script:
#!/bin/bash # Search for any wagmi imports echo "=== Checking for any wagmi imports ===" rg "from ['\"]wagmi['/]" # Search for chain imports to verify they're using viem echo -e "\n=== Checking all chain imports ===" rg "from ['\"].*chains['\"]"Length of output: 700
🧰 Tools
🪛 GitHub Actions: Typescript CI
[error] Failed to parse TypeScript config file due to missing 'ts-node' package dependency. Install 'ts-node' to resolve this issue.
@@ -87,7 +87,7 @@ const config: Config = { | |||
* Won't transform: "lodash", "moment" | |||
*/ | |||
transformIgnorePatterns: [ | |||
"node_modules/(?!((jest-)?react-native|@react-native(-community)?|expo(nent)?|@expo(nent)?/.*|@expo-google-fonts/.*|react-navigation|@react-navigation/.*|@unimodules/.*|unimodules|sentry-expo|native-base|react-native-svg|rn-fetch-blob|@xmtp|uuid|wagmi))", | |||
"node_modules/(?!((jest-)?react-native|@react-native(-community)?|expo(nent)?|@expo(nent)?/.*|@expo-google-fonts/.*|react-navigation|@react-navigation/.*|@unimodules/.*|unimodules|sentry-expo|native-base|react-native-svg|rn-fetch-blob|@xmtp|uuid))", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Keep react-navigation patterns in transformIgnorePatterns
The react-navigation packages are actively used in the codebase with multiple dependencies in package.json and 40+ imports across various files. Removing these patterns could break test execution. While @expo-google-fonts can be safely removed, please keep the following patterns:
- @react-navigation/.*
- react-navigation
🔗 Analysis chain
Verify the impact of removing packages from transformIgnorePatterns.
The removal of @expo-google-fonts/., react-navigation, and @react-navigation/. from transformIgnorePatterns needs verification to ensure it doesn't affect test execution.
Let's verify if these packages are still in use and require transformation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for usage of removed packages
echo "Checking for @expo-google-fonts usage..."
rg "from ['\"]\@expo-google-fonts/"
echo "Checking for react-navigation usage..."
rg "from ['\"]\@?react-navigation/"
Length of output: 467
Script:
#!/bin/bash
# Check package.json for dependencies
echo "Checking package.json for dependencies..."
cat package.json | jq '.dependencies, .devDependencies' 2>/dev/null | grep -E "@expo-google-fonts|react-navigation"
echo -e "\nChecking for @expo-google-fonts imports..."
rg "from .*@expo-google-fonts"
echo -e "\nChecking for react-navigation imports..."
rg "from .*react-navigation"
echo -e "\nChecking for additional jest config files..."
fd -e js -e json -e ts "jest.config"
Length of output: 6315
🧰 Tools
🪛 GitHub Actions: Typescript CI
[error] Failed to parse TypeScript config file due to missing 'ts-node' package dependency. Install 'ts-node' to resolve this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
Line range hint
1-256
: Consider using strict versions for critical dependencies.Several critical dependencies use caret (^) versioning which may lead to unexpected updates. Consider using fixed versions for critical dependencies like:
- ethers
- @xmtp/xmtp-js
- react-native
- expo
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (1)
package.json
(3 hunks)
🔇 Additional comments (3)
package.json (3)
188-195
: Correct move of Babel dependencies to devDependencies.Moving Babel-related packages to
devDependencies
is the right approach since these are build-time dependencies not required at runtime.
86-86
: Verify module resolver configuration.The addition of
babel-plugin-module-resolver
suggests path aliasing is being implemented. This is good for maintainability, but requires proper configuration.Run this script to check for the Babel configuration:
✅ Verification successful
Module resolver configuration is properly implemented ✅
The babel-plugin-module-resolver is correctly configured in babel.config.js and serves specific module aliasing needs for compatibility and bundling purposes, rather than path aliasing for maintainability as initially assumed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for babel.config.js and verify module-resolver configuration # Look for babel config files echo "Checking for Babel configuration files..." fd -t f "babel\.config\.(js|json)" # Check module-resolver configuration in babel config echo "Checking module-resolver configuration..." rg -A 10 "module-resolver"Length of output: 2514
203-203
: Verify compatibility with updated dev dependencies.The following version updates need verification:
- @types/jest: ^29.5.14
- jest: ~29.7.0
- jest-expo: ~52.0.3
- react-native-svg-transformer: ^1.5.0
Run this script to check for potential compatibility issues:
Also applies to: 222-223, 233-233
✅ Verification successful
Dependencies are compatible with each other
The updated dependencies maintain version compatibility:
- jest-expo ~52.0.3 matches expo ^52.0.0
- @types/jest ^29.5.14 matches jest ~29.7.0
- react-native-svg-transformer ^1.5.0 is compatible with the current setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential compatibility issues in package.json # Check if jest-expo version matches the expo version echo "Checking jest-expo compatibility..." rg '"expo":.*"52\.0\.0"' package.json rg '"jest-expo":.*"52\.0\.3"' package.json # Check React Native version compatibility echo "Checking React Native compatibility..." rg '"react-native":.*"0\.76\.5"' package.jsonLength of output: 330
Script:
#!/bin/bash # Get all relevant dependency versions echo "Checking dependency versions..." rg -A1 '"dependencies": \{' package.json rg -A1 '"devDependencies": \{' package.json rg '@types/jest|jest":|jest-expo|react-native-svg-transformer|expo":' package.json # Check expo SDK version from app.json/app.config.js echo -e "\nChecking Expo SDK version..." fd -e json -e js -x grep -l "sdkVersion" {}Length of output: 686
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
Going to hold this until after the Preview build Friday morning |
13f09aa
to
4019c0e
Compare
Should be fine to merge once we get a consistent build |
Removed a large bit of dependencies
TS Node is required when using jest.config.ts
4019c0e
to
226c46c
Compare
Removed a large bit of dependencies
Summary by CodeRabbit
Configuration Updates
wagmi/chains
toviem/chains
Dependency Management
react-native-device-info
babel-plugin-module-resolver
react-native-svg-transformer
and@types/jest
versionsjest-expo
versionTesting
react-native-device-info