Update Node SDK #715
Update Node SDK #715
Conversation
🦋 Changeset detectedLatest commit: 31ca82d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe pull request introduces several updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 (
|
Deploying xmtp-js-docs with
|
| Latest commit: |
31ca82d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a442f404.xmtp-js.pages.dev |
| Branch Preview URL: | https://rygine-node-sdk-update.xmtp-js.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
sdks/node-sdk/test/helpers.ts (1)
1-1: Consider generating unique encryption keys per testWhile using
getRandomValuesfor key generation is secure, having a single shared encryption key at module level could potentially lead to test interference or make it harder to test key-specific scenarios. Consider moving the key generation into thecreateClientfunction to ensure each test instance has its own unique key.-const testEncryptionKey = getRandomValues(new Uint8Array(32)); export const createClient = async (user: User, options?: ClientOptions) => { + const testEncryptionKey = getRandomValues(new Uint8Array(32)); const opts = { ...options, env: options?.env ?? "local", }; return Client.create(user.account.address, testEncryptionKey, {Also applies to: 17-17
sdks/node-sdk/test/Client.test.ts (3)
38-40: LGTM! Consider adding more test cases.The updated assertion correctly validates the new return type of
canMessageusingObject.fromEntries(). This aligns with the PR objective of updating the method's return type to match the browser SDK.Consider adding more test cases to cover:
- Multiple addresses with mixed registration status
- Invalid/malformed addresses
- Empty address array
- Case sensitivity handling
Example:
it("should handle multiple addresses with mixed registration status", async () => { const user1 = createUser(); const user2 = createUser(); const client = await createRegisteredClient(user1); const canMessage = await client.canMessage([ user1.account.address, user2.account.address ]); expect(Object.fromEntries(canMessage)).toEqual({ [user1.account.address.toLowerCase()]: true, [user2.account.address.toLowerCase()]: false }); });
Line range hint
1-1: Add test coverage for new features.The PR introduces two significant features that require test coverage:
- Encryption key requirement:
- Add tests to verify proper encryption key handling
- Include negative test cases for invalid/missing keys
- Smart contract wallet support:
- Add test cases using smart contract wallet addresses
- Verify integration with existing wallet association tests
Would you like me to help generate the test cases for these features?
Line range hint
1-275: Enhance test organization and coverage.The test suite is well-structured but could benefit from these improvements:
- Group related tests using nested
describeblocks- Split longer tests into smaller, focused cases
- Add explicit error case coverage
Example restructuring:
describe("Client", () => { describe("initialization", () => { it("should create an unregistered client", async () => { // existing create client test }); it("should fail with invalid encryption key", async () => { // new test }); }); describe("registration", () => { // existing registration tests }); describe("messaging capabilities", () => { // existing and new canMessage tests }); describe("wallet management", () => { describe("adding wallets", () => { // split from existing add wallet test }); describe("revoking wallets", () => { // split from existing revoke wallet test }); describe("smart contract wallets", () => { // new tests }); }); });sdks/node-sdk/src/Client.ts (3)
76-80: Document default values for logging optionsIncluding the default values in the JSDoc comments for
structuredLoggingandloggingLevelwould enhance clarity for users regarding the expected behavior when these options are omitted.
101-105: Add documentation for thecreatemethod's parametersNow that the
createmethod requires anencryptionKeyparameter, adding JSDoc comments for the method and its parameters would improve usability and help developers understand how to use it effectively.
207-215: Add documentation for theaddScwSignaturemethodAdding JSDoc comments for the
addScwSignaturemethod would provide clarity on its purpose, parameters, and usage, benefiting other developers interfacing with this API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
sdks/node-sdk/package.json(1 hunks)sdks/node-sdk/src/Client.ts(6 hunks)sdks/node-sdk/src/index.ts(1 hunks)sdks/node-sdk/test/Client.test.ts(1 hunks)sdks/node-sdk/test/helpers.ts(3 hunks)
🔇 Additional comments (5)
sdks/node-sdk/src/index.ts (2)
13-25: LGTM! Explicit type exports improve clarity and maintainability.
The change from wildcard exports to explicit type exports is a good practice as it:
- Makes dependencies more transparent
- Enables better tree-shaking
- Improves IDE support and type documentation
26-43: Verify version compatibility with node-bindings.
The explicit exports look good and support the new features. Let's verify the version compatibility to ensure all exported entities are available in the required version.
✅ Verification successful
Based on the results, I can now generate the final response:
Version compatibility with node-bindings is confirmed
The package requires ^0.0.18 of @xmtp/node-bindings and all exported types in the index.ts are actively used throughout the codebase, as evidenced by the import statements found in multiple files. The types being exported are consistent with their usage in the codebase, indicating proper version compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all exported entities exist in the specified version of node-bindings
# Check package.json for the required version
echo "Required version:"
fd -t f package.json -x grep -l "@xmtp/node-bindings" {} \; -exec grep -A 1 "@xmtp/node-bindings" {} \;
# Verify exports in node_modules (if available) or npm registry
echo -e "\nVerifying exports in @xmtp/node-bindings:"
curl -s https://registry.npmjs.org/@xmtp/node-bindings/latest | jq '.exports'
# Look for any potential breaking changes in the changelog
echo -e "\nChecking for breaking changes:"
fd -t f "CHANGELOG.md" -x grep -A 5 "0.0.18" {} \;
Length of output: 854
Script:
#!/bin/bash
# Let's try a different approach to verify the exports
# First, check the exact version requirement in package.json
echo "Checking package.json for node-bindings version:"
fd package.json -x cat {} \; | jq '.dependencies["@xmtp/node-bindings"]'
# Now let's check the actual type definitions in the source code
echo -e "\nChecking imported types from node-bindings:"
rg -A 1 "from ['\"]@xmtp/node-bindings['\"]" --type ts
# Let's also check if there are any type errors in the codebase
echo -e "\nChecking for potential type-related imports:"
ast-grep --pattern 'import { $$$_ } from "@xmtp/node-bindings"'
Length of output: 4143
sdks/node-sdk/package.json (1)
Line range hint 82-84: Verify Node.js version compatibility
The package requires Node.js >= 20. Please ensure that the new features and changes in @xmtp/[email protected] are compatible with this version requirement.
✅ Verification successful
Let me generate another verification script to check the node-bindings package's requirements and features:
Based on the verification results:
- The package requires Node.js >= 20 as specified in the package.json
- No Node.js 20+ specific features (WeakRef, Array.fromAsync, Promise.withResolvers) are used in the codebase
- The only modern JavaScript feature found is
import.metawhich is supported since Node.js 16 - The dependency
@xmtp/[email protected]is used as a regular dependency
Node.js version compatibility verified
The codebase appears to be compatible with Node.js 20 requirement. No Node.js 20-specific features are used, and the existing code uses features well-supported in Node.js 20.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Node.js version requirements in dependencies
# Check package.json in node-bindings for engine requirements
rg -l "node-bindings" | xargs rg "engines|node"
# Check for usage of Node.js 20+ specific features
ast-grep --pattern 'import.meta' # ES2020 feature
Length of output: 48466
Script:
#!/bin/bash
# Check node-bindings package.json for engine requirements and dependencies
rg -g "package.json" "\"@xmtp/node-bindings\"" -B5 -A5
# Look for any Node.js 20+ specific features in the codebase
ast-grep --pattern 'WeakRef' # Node.js 20 feature
ast-grep --pattern 'Array.fromAsync' # Node.js 20 feature
ast-grep --pattern 'Promise.withResolvers' # Node.js 20 feature
Length of output: 797
sdks/node-sdk/src/Client.ts (2)
115-118: Logging options initialized correctly
The logOptions object is properly initialized with default values, ensuring logging behaves as expected when options are not specified.
196-197: Updated return type of canMessage method to Map
Changing the return type of canMessage to a Map enhances data handling and aligns with modern JavaScript practices.
Summary
Client.canMessageto match the browser SDKSummary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
canMessagemethod's return structure.Chores
@xmtp/node-bindings.