-
Notifications
You must be signed in to change notification settings - Fork 46
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(kleros-sdk): move-viem-to-peer-dependencies #1769
Conversation
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Code Climate has analyzed commit 46ed066 and detected 0 issues on this pull request. View more on Code Climate. |
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
🧹 Outside diff range and nitpick comments (1)
kleros-sdk/package.json (1)
51-52
: Document peer dependency changesMoving viem to peer dependencies is a breaking change that affects package consumers. Consider:
- Adding a note in the changelog about this change
- Providing migration instructions for consumers
- Updating the package's major version number following semver
Would you like me to help draft the changelog entry and migration guide?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (2)
kleros-sdk/package.json
(1 hunks)web/package.json
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/package.json
🔇 Additional comments (2)
kleros-sdk/package.json (2)
42-42
: Verify viem version update implications
The viem version has been updated from ^2.21.48 to ^2.21.53. While this is a patch version update and should be backward compatible, it's good practice to:
- Review the changelog for any potential breaking changes
- Verify compatibility with dependent packages
Let's check for recent releases and breaking changes:
Also applies to: 52-52
✅ Verification successful
Version update is safe to proceed
The version bump from ^2.21.48 to ^2.21.53 consists of only patch changes:
- Updates to chain configurations (new chains, multicall addresses)
- API URL updates
- Bug fixes for transaction receipt handling
- No breaking changes or deprecations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch recent releases and their descriptions
gh api graphql -f query='
{
repository(owner: "wagmi-dev", name: "viem") {
releases(first: 5, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
tagName
description
}
}
}
}' | jq -r '.data.repository.releases.nodes[] | "Version: \(.tagName)\nChanges:\n\(.description)\n"'
Length of output: 6625
42-42
: Verify complete migration of viem to peer dependencies
The PR title suggests moving viem to peer dependencies, but it's currently declared in both devDependencies
and peerDependencies
. This might lead to dependency resolution issues. Consider:
- Keep it only in
peerDependencies
if it's required by consumers - Keep it in both
devDependencies
andpeerDependencies
if it's also needed for development/testing
Let's verify the usage of viem in the package:
Also applies to: 52-52
✅ Verification successful
Keep viem in both devDependencies and peerDependencies
The codebase extensively uses viem throughout the source files for types, utilities, and core functionality. Having viem in both devDependencies
and peerDependencies
is the correct approach because:
- It's needed during development for TypeScript compilation and testing (
devDependencies
) - It's a runtime dependency that should be provided by the consuming application (
peerDependencies
)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if viem is used in tests or build scripts
rg -l "from ['|\"]viem['\"]" "kleros-sdk/src" "kleros-sdk/test" || echo "No direct viem imports found"
Length of output: 371
Script:
#!/bin/bash
# Check how viem is used in the codebase
rg "from ['|\"]viem['\"]" "kleros-sdk/src" -A 2 -B 2
Length of output: 2186
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
lgtm
PR-Codex overview
This PR updates the version of the
viem
package across multiple files, ensuring consistency in dependency management.Detailed summary
viem
version from^2.21.48
to^2.21.53
in:web/package.json
kleros-sdk/package.json
yarn.lock
viem
as apeerDependency
inkleros-sdk/package.json
.yarn.lock
to reflect the new version ofviem
.Summary by CodeRabbit
New Features
@kleros/kleros-sdk
package to version2.1.8
.@kleros/kleros-v2-web
project to version0.2.0
.Bug Fixes
viem
package version to^2.21.53
in bothkleros-sdk
andkleros-v2-web
projects to ensure compatibility and stability.