Upgrade node bindings #709
Conversation
🦋 Changeset detectedLatest commit: 5a41542 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 across multiple files in 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: |
5a41542
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5cbdd962.xmtp-js.pages.dev |
| Branch Preview URL: | https://rygine-node-sdk-updates.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: 1
🧹 Outside diff range and nitpick comments (5)
sdks/node-sdk/src/AsyncStream.ts (1)
Line range hint
68-75: Consider adding error handling for stream termination.While the type changes look good, consider adding error handling for cases where the stream is terminated abnormally. This could help with the streaming issues mentioned in the PR objectives.
return = (value: T | undefined) => { this.#done = true; + // Clean up any pending operations + this.#queue = []; + this.#resolveNext = null; this.onReturn?.(); return Promise.resolve({ done: true, value, }); };sdks/node-sdk/src/Conversation.ts (4)
4-6: Consider renaming theGrouptype alias for clarity.The alias
Conversation as Groupcould be confusing since this file defines its ownConversationclass. Consider using a more distinctive name likeConversationGrouporNativeConversationto avoid potential confusion.
116-121: LGTM: Improved stream error handling.The changes enhance error handling by:
- Using more conventional parameter names (error, value)
- Explicitly handling undefined values
- Properly propagating errors through both the AsyncStream and callback
Consider adding a JSDoc comment to document the possible undefined case for better developer experience.
+ /** + * Streams messages from the conversation. + * @param callback Optional callback that receives either a DecodedMessage or undefined + * when the stream value is null, along with any potential errors. + */ stream(callback?: StreamCallback<DecodedMessage>) {
Line range hint
197-204: Document the message filtering behavior.The method filters out messages with undefined content, which is an important implementation detail that should be documented. This behavior could affect consumers of the API.
+ /** + * Retrieves messages from the conversation. + * @param options Optional parameters for listing messages + * @returns An array of DecodedMessage, excluding any messages with undefined content + */ messages(options?: ListMessagesOptions): DecodedMessage[] {
211-213: Add return type annotation for consistency.The
updateConsentStatemethod should have an explicit return type annotation for consistency with other methods.- updateConsentState(consentState: ConsentState) { + updateConsentState(consentState: ConsentState): void {
📜 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 (10)
sdks/node-sdk/package.json(1 hunks)sdks/node-sdk/src/AsyncStream.ts(2 hunks)sdks/node-sdk/src/Client.ts(7 hunks)sdks/node-sdk/src/Conversation.ts(5 hunks)sdks/node-sdk/src/Conversations.ts(12 hunks)sdks/node-sdk/src/DecodedMessage.ts(3 hunks)sdks/node-sdk/test/Client.test.ts(6 hunks)sdks/node-sdk/test/Conversation.test.ts(4 hunks)sdks/node-sdk/test/Conversations.test.ts(5 hunks)sdks/node-sdk/test/helpers.ts(2 hunks)
🔇 Additional comments (32)
sdks/node-sdk/src/AsyncStream.ts (2)
16-16: LGTM! Queue type properly aligned with callback changes.
The queue type modification to (T | undefined)[] maintains type safety and correctly supports the new undefined value handling.
8-11: Verify handling of undefined values in consuming code.
The type change to allow undefined values in StreamCallback is a good improvement for flexibility. However, we should ensure all consumers properly handle the undefined case.
✅ Verification successful
Based on the search results, I can now generate the final response:
StreamCallback's undefined value handling is properly implemented
The codebase shows consistent and safe handling of undefined values in all StreamCallback implementations:
- All stream methods use ternary operators with null checks:
value ? new Something(value) : undefined - The AsyncStream base implementation has proper error and completion state handling
- No direct property access on
valuewithout prior existence checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for StreamCallback usage to verify undefined handling
rg -l "StreamCallback" | xargs rg -A 5 "StreamCallback"
# Look for potential value access without undefined checks
ast-grep --pattern 'value.$$$'
Length of output: 4946
sdks/node-sdk/src/DecodedMessage.ts (4)
3-5: LGTM! Clean import updates.
The updated imports align with the node-bindings upgrade, properly using type imports where appropriate.
28-28: Verify constructor usage after type change.
The constructor parameter type change from NapiMessage to Message looks good, but we should verify all instantiations of DecodedMessage have been updated.
#!/bin/bash
# Search for DecodedMessage instantiations to verify they use the updated type
rg -l "new DecodedMessage" | xargs rg -A 2 "new DecodedMessage"46-56: Verify delivery status enum coverage.
The switch statement handles Unpublished, Published, and Failed statuses. Let's verify these cover all possible DeliveryStatus values.
✅ Verification successful
Switch statement correctly handles all delivery statuses
The verification confirms the switch statement is complete and correct:
- The
MessageDeliveryStatustype is explicitly defined as"unpublished" | "published" | "failed" - The switch statement handles all three possible values from
DeliveryStatusenum - The same pattern is consistently used in both node-sdk and browser-sdk
- The mapping aligns perfectly with the defined type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DeliveryStatus definition to verify all cases are covered
ast-grep --pattern 'enum DeliveryStatus {
$$$
}'
Length of output: 52
Script:
#!/bin/bash
# Let's try searching for DeliveryStatus using ripgrep to find its definition
rg "enum.*DeliveryStatus" -A 5
Length of output: 32
Script:
#!/bin/bash
# Let's try a broader search for DeliveryStatus to find its definition and usage
rg "DeliveryStatus" -A 3 -B 3
Length of output: 5668
36-43: Verify message kind enum coverage.
The switch statement handles Application and MembershipChange kinds. Let's verify these cover all possible GroupMessageKind values.
sdks/node-sdk/package.json (2)
55-55: LGTM! Dependency update aligns with PR objectives.
The upgrade of @xmtp/node-bindings to version ^0.0.17 corresponds with the PR's focus on updating type exports and fixing streaming issues.
Line range hint 2-2: Verify version compatibility and consider version bump.
Since this PR introduces type changes and streaming fixes through the node-bindings upgrade, consider:
- Verifying that all type changes are backward compatible
- Potentially bumping the node-sdk version (currently 0.0.21) if the changes warrant it
Also applies to: 55-55
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Version bump recommended for streaming changes and type refactoring
The changelog shows that version 0.0.21 already includes significant changes:
- Refactored streams for better error handling
- Added 1:1 messages and stream error handling
- Added consent state methods and signature functionality
- Added new client capabilities
These changes are already properly versioned and documented, and there's no evidence of breaking type changes that would require additional version bumps.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in type usage across the codebase
# Test: Look for any TODO/FIXME comments related to types that might indicate compatibility issues
rg -i "TODO|FIXME|XXX" --type ts
# Test: Look for type-related changes in test files that might indicate breaking changes
rg -i "type|interface" --type test
Length of output: 1362
Script:
#!/bin/bash
# Let's gather more specific information about type changes and version impacts
# Check the current version in package.json
echo "Current version in package.json:"
grep '"version"' sdks/node-sdk/package.json
# Look for type definitions and interfaces in node-sdk
echo -e "\nType definitions in node-sdk:"
rg -g '*.ts' -g '*.d.ts' 'type|interface' sdks/node-sdk/
# Check for recent changes in type definitions
echo -e "\nRecent changes in type-related files:"
git log -p --since="1 month ago" sdks/node-sdk/ | grep -A 5 -B 5 'type|interface'
# Check for breaking changes markers in commit messages
echo -e "\nBreaking change indicators in recent commits:"
git log --since="1 month ago" --grep="BREAKING" sdks/node-sdk/
# Look for dependencies that might affect types
echo -e "\nDependencies that might affect types:"
grep -A 10 '"dependencies"' sdks/node-sdk/package.json
Length of output: 5800
Script:
#!/bin/bash
# Let's check for any changes in the node-bindings dependency and its impact
# Check the git history for changes related to node-bindings
echo "Recent changes related to node-bindings:"
git log -p --since="1 month ago" sdks/node-sdk/ | grep -B 5 -A 5 "@xmtp/node-bindings"
# Look for any changes in streaming-related types
echo -e "\nStreaming-related type changes:"
rg -g '*.ts' 'stream|Stream' sdks/node-sdk/
# Check test files for any streaming or type-related changes
echo -e "\nTest file changes for streaming and types:"
git log -p --since="1 month ago" sdks/node-sdk/test/ | grep -B 5 -A 5 'stream\|Stream\|type\|interface'
# Look for any migration guides or breaking change documentation
echo -e "\nChecking for migration or breaking change documentation:"
fd -e md . sdks/node-sdk/ -x grep -l "migration\|breaking\|upgrade"
Length of output: 30787
Script:
#!/bin/bash
# Let's check the changelog and recent commits for breaking changes
# Check CHANGELOG.md for recent version changes
echo "Recent changelog entries:"
head -n 20 sdks/node-sdk/CHANGELOG.md
# Check recent commits for node-bindings upgrade details
echo -e "\nRecent commits mentioning node-bindings upgrade:"
git log --grep="node-bindings" --since="1 month ago" -p sdks/node-sdk/
# Check for any type-related changes in the diff
echo -e "\nType-related changes in recent commits:"
git log -p --since="1 month ago" sdks/node-sdk/ | grep -A 5 -B 5 "type|interface|NapiType"
Length of output: 911
sdks/node-sdk/test/helpers.ts (2)
8-8: LGTM! Import statement updated for new type name.
The change from NapiSignatureRequestType to SignatureRequestType aligns with the node bindings upgrade and improves type naming clarity.
64-64: LGTM! Verify type usage consistency.
The usage of SignatureRequestType.CreateInbox is correctly updated. Let's verify that all other occurrences have been similarly updated.
✅ Verification successful
Type usage is consistent across the codebase
The verification shows that:
- Node SDK consistently uses
SignatureRequestTypefrom@xmtp/node-bindings - Browser SDK consistently uses
WasmSignatureRequestTypefrom@xmtp/wasm-bindings - No occurrences of the old
NapiSignatureRequestTypewere found - Each SDK maintains its own appropriate type variant throughout its codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all occurrences of NapiSignatureRequestType have been replaced
# with SignatureRequestType across the codebase
echo "Checking for any remaining occurrences of NapiSignatureRequestType..."
rg "NapiSignatureRequestType"
echo "Verifying all current uses of SignatureRequestType..."
rg "SignatureRequestType" -A 2
Length of output: 6133
sdks/node-sdk/src/Conversation.ts (1)
15-15: LGTM: Type updates are consistent.
The changes to the private field and constructor parameter types align with the new type system while maintaining the same functionality.
Also applies to: 17-17
sdks/node-sdk/src/Conversations.ts (5)
2-4: LGTM! Type updates improve clarity
The renaming of types from Napi* prefix to more semantic names (CreateGroupOptions, ListConversationsOptions, NodeConversations) improves code clarity and maintainability.
Also applies to: 13-13
15-18: LGTM! Constructor signature updated correctly
The constructor signature has been properly updated to use the new NodeConversations type while maintaining the same initialization logic.
52-52: LGTM! Method signatures consistently updated
Method signatures have been properly updated to use the new type names while preserving the existing type constraints and method behaviors.
Also applies to: 68-68, 77-77, 86-86
101-104: LGTM! Improved stream error handling
The changes introduce robust error handling across all streaming methods by:
- Safely handling undefined values in stream callbacks
- Consistently applying the pattern across all streaming methods
- Preventing potential null pointer exceptions during stream errors
This improvement aligns with the PR objective of fixing streaming issues.
Also applies to: 117-120, 133-136, 152-155, 171-177, 190-193
101-104: Verify consistent stream error handling across the codebase
Let's verify that similar stream error handling improvements have been applied to all streaming methods in the codebase.
✅ Verification successful
Based on the search results, I can now generate a final response:
Stream error handling is consistently implemented across the codebase
The error handling pattern is consistently implemented in all streaming methods:
Conversations.ts:stream((err, value) => {...})Conversation.ts:stream((error, value) => {...})- Both implementations properly handle the error parameter and pass it through to callbacks
- JS SDK uses a different streaming pattern (async iterators) which handles errors through Promise rejection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other streaming methods that might need similar error handling updates
ast-grep --pattern 'stream($$$) {
$$$
.stream((err, value) => {
$$$
});
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search for stream-related patterns
rg -A 5 "\.stream\(" --type ts
# Also search for method definitions containing 'stream'
ast-grep --pattern 'stream($$$) {
$$$
}'
Length of output: 4395
sdks/node-sdk/src/Client.ts (6)
17-22: LGTM! Clean type imports.
The removal of the Napi prefix from types makes the code cleaner while maintaining functionality.
87-87: LGTM! Consistent type updates.
The NodeClient type updates in both the property and constructor are consistent with the node bindings upgrade.
Also applies to: 91-91
189-191: LGTM! Type signature updated correctly.
The SignatureRequestType parameter type update aligns with the node bindings upgrade.
224-224: LGTM! Message type and enum updates.
The Message type and GroupMessageKind enum updates maintain the same validation logic while aligning with the new type system.
Also applies to: 233-233
268-273: LGTM! Consent type updates.
The Consent and ConsentEntityType updates maintain the same functionality while aligning with the new type system.
242-244: Verify the method rename impact.
The method call has been renamed from requestHistorySync to sendHistorySyncRequest. While the change is straightforward, let's verify there are no other callers that need updating.
✅ Verification successful
Let me gather more context about the usage of both method names.
Let me check if there are any other files that might be using this client method.
Let me check for test files specifically with common test file extensions.
Method rename is self-contained and safe
The method rename from requestHistorySync to sendHistorySyncRequest is localized to the Client.ts file. The only occurrences found are:
- The method definition itself
- A configuration option property named
requestHistorySyncwhich is unrelated to the method rename - The internal call to the new method name
sendHistorySyncRequest
No other usages were found in the codebase, including tests and examples, indicating this is a safe change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old method name
rg "requestHistorySync" --type ts
Length of output: 216
Script:
#!/bin/bash
# Get more context around the occurrences
rg "requestHistorySync|sendHistorySyncRequest" --type ts -A 3 -B 3
Length of output: 1297
Script:
#!/bin/bash
# Search for Client class usage and instantiation
rg "new Client\(" --type ts -A 3 -B 3
# Also search for any test files that might be using these methods
rg "requestHistorySync|sendHistorySyncRequest" --type test
Length of output: 904
Script:
#!/bin/bash
# Search in test files with common extensions
rg "requestHistorySync|sendHistorySyncRequest" -g "*.test.ts" -g "*.spec.ts" -g "*.test.js" -g "*.spec.js" -A 3 -B 3
# Also check if the method is used in any examples
fd -e ts -e js -p examples/ | xargs rg "requestHistorySync|sendHistorySyncRequest"
Length of output: 203
sdks/node-sdk/test/Client.test.ts (4)
2-4: LGTM! Clean type imports from node-bindings.
The removal of the "Napi" prefix from types indicates a cleaner API design in the upgraded package.
119-120: LGTM! Consistent usage of SignatureRequestType.
The type has been correctly updated across all signature-related operations while maintaining the original test functionality for wallet associations and installation management.
Also applies to: 150-151, 164-164, 198-198
223-224: LGTM! Comprehensive consent state management test.
The test thoroughly covers the consent state lifecycle with the updated types while maintaining complete test coverage of all state transitions (Unknown → Allowed → Denied).
Also applies to: 228-230, 235-236, 238-238, 240-240, 243-244
2-4: Verify consistent type usage across the codebase.
Let's ensure all usages of these types have been updated throughout the codebase:
✅ Verification successful
Type changes have been consistently updated across the codebase
The verification shows that:
- No occurrences of old
Napi-prefixed types were found - The new types are consistently used:
node-sdkusesConsentEntityType,ConsentState,SignatureRequestTypefrom@xmtp/node-bindingsbrowser-sdkusesWasmConsentEntityType,WasmConsentState,WasmSignatureRequestTypefrom@xmtp/wasm-bindingsjs-sdkuses its own string-based type definition forConsentState
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of the old type names
echo "Checking for any remaining Napi-prefixed types..."
rg "NapiConsentEntityType|NapiConsentState|NapiSignatureRequestType"
# Verify the new type usage is consistent
echo "Verifying new type usage..."
rg "ConsentEntityType|ConsentState|SignatureRequestType"
Length of output: 12435
sdks/node-sdk/test/Conversation.test.ts (2)
1-1: LGTM: Import change aligns with node-bindings upgrade
The change from NapiConsentState to ConsentState is consistent with the package upgrade objectives.
417-417: LGTM: Consent state checks properly updated
The consent state checks have been properly updated to use the new ConsentState enum from the upgraded node-bindings package. The tests correctly verify the state transitions from Unknown to Allowed.
Also applies to: 419-419, 424-424, 426-426
sdks/node-sdk/test/Conversations.test.ts (4)
1-1: LGTM! Clean type imports.
The removal of the "Napi" prefix from imported types aligns with the node bindings upgrade and improves type clarity.
33-33: LGTM! Consistent type usage in test assertions.
The test cases correctly use the updated type names while maintaining the same validation logic for permissions and consent states.
Also applies to: 87-87, 109-109
214-221: LGTM! Proper type usage in conversation options.
The conversation creation with admin-only permissions correctly uses the updated type name while maintaining the same permission validation logic.
Line range hint 266-447: Comprehensive streaming test coverage maintained.
The test suite thoroughly covers various streaming scenarios for both DM and group conversations. Since no changes were needed in the streaming test logic, please ensure that the streaming fixes mentioned in the PR objectives are implemented and tested in the relevant files.
✅ Verification successful
Streaming implementation and tests are properly aligned
The streaming functionality is well-implemented and thoroughly tested:
AsyncStreamclass provides robust async iteration support with proper error handlingConversationsclass implements all streaming methods (stream, streamGroups, streamDms, streamAllMessages, etc.) consistently- The test coverage in the review matches the actual implementation, validating both success and error cases
The streaming test coverage is appropriate and no issues were found in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for streaming-related changes in other files
# Test: Search for streaming-related code changes
rg -l "stream|Stream" --type ts
Length of output: 884
Script:
#!/bin/bash
# Let's check for recent changes in streaming-related files
git diff HEAD~10..HEAD -- sdks/node-sdk/src/AsyncStream.ts sdks/node-sdk/src/Conversations.ts sdks/node-sdk/src/Conversation.ts
# Also check the implementation of streaming methods
ast-grep --pattern 'stream($$$) {
$$$
}'
Length of output: 210
Script:
#!/bin/bash
# Let's examine the streaming implementations in the node-sdk
cat sdks/node-sdk/src/AsyncStream.ts
cat sdks/node-sdk/src/Conversations.ts
# Also check the test file for any recent changes
cat sdks/node-sdk/test/AsyncStream.test.ts
Length of output: 9358
Summary
@xmtp/node-bindingsSummary by CodeRabbit
Release Notes
New Features
undefinedvalues.Improvements
Bug Fixes
These updates improve overall robustness and maintainability of the SDK, ensuring a smoother user experience.