Update Node SDK #694
Update Node SDK #694
Conversation
🦋 Changeset detectedLatest commit: 87457d6 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 changes enhance the message handling capabilities of the Changes
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 (
|
rygine
force-pushed
the
rygine/node-sdk-updates
branch
from
4edc845 to
6c1ae40
Compare
Deploying xmtp-js-docs with
|
| Latest commit: |
87457d6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8e2cec21.xmtp-js.pages.dev |
| Branch Preview URL: | https://rygine-node-sdk-updates.xmtp-js.pages.dev |
rygine
force-pushed
the
rygine/node-sdk-updates
branch
from
6c1ae40 to
f3d57da
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.changeset/chatty-snails-roll.md (1)
7-7: Minor grammar improvement suggested.Consider adding the article "the" before "
DecodedMessage" for better readability.-Added generic typing for message content to `DecodedMessage` class and `Conversations.findMessageById` +Added generic typing for message content to the `DecodedMessage` class and `Conversations.findMessageById`🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...d generic typing for message content toDecodedMessageclass and `Conversations...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
sdks/node-sdk/src/DecodedMessage.ts (1)
13-13: LGTM! Consider adding JSDoc for type parameter.The addition of generic typing improves type safety while maintaining backward compatibility with the
anydefault.Add TypeScript documentation for the type parameter:
+/** + * @template T The type of the message content + */ export class DecodedMessage<T = any> {sdks/node-sdk/src/Conversations.ts (1)
Line range hint
82-94: Consider adding generic type support to streamAllMessagesFor consistency with the updated
getMessageById<T>method, consider adding generic type support to thestreamAllMessagesmethod as well.- async streamAllMessages(callback?: StreamCallback<DecodedMessage>) { + async streamAllMessages<T = any>(callback?: StreamCallback<DecodedMessage<T>>) { await this.sync(); const asyncStream = new AsyncStream<DecodedMessage>(); const stream = this.#conversations.streamAllMessages((err, message) => { - const decodedMessage = new DecodedMessage(this.#client, message); + const decodedMessage = new DecodedMessage<T>(this.#client, message); asyncStream.callback(err, decodedMessage); callback?.(err, decodedMessage); }); asyncStream.stopCallback = stream.end.bind(stream); return asyncStream; }sdks/node-sdk/src/Client.ts (1)
Line range hint
208-213: Enhance error message for group membership validation.While the validation is good, the error message could be more descriptive to help with debugging.
Consider this improvement:
if ( contentType.sameAs(ContentTypeGroupUpdated) && message.kind !== NapiGroupMessageKind.MembershipChange ) { - throw new Error("Error decoding group membership change"); + throw new Error(`Invalid message kind for group update: expected ${NapiGroupMessageKind.MembershipChange}, got ${message.kind}`); }
📜 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 (9)
- .changeset/chatty-snails-roll.md (1 hunks)
- sdks/node-sdk/package.json (1 hunks)
- sdks/node-sdk/rollup.config.js (1 hunks)
- sdks/node-sdk/src/Client.ts (1 hunks)
- sdks/node-sdk/src/Conversation.ts (1 hunks)
- sdks/node-sdk/src/Conversations.ts (1 hunks)
- sdks/node-sdk/src/DecodedMessage.ts (2 hunks)
- sdks/node-sdk/src/codecs/GroupUpdatedCodec.ts (0 hunks)
- sdks/node-sdk/src/index.ts (0 hunks)
💤 Files with no reviewable changes (2)
- sdks/node-sdk/src/codecs/GroupUpdatedCodec.ts
- sdks/node-sdk/src/index.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/chatty-snails-roll.md
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...d generic typing for message content toDecodedMessageclass and `Conversations...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (10)
.changeset/chatty-snails-roll.md (1)
1-3: LGTM: Appropriate version bump.The patch version bump is correct as these changes are backwards-compatible improvements and bug fixes.
sdks/node-sdk/rollup.config.js (1)
11-11: LGTM! External dependencies properly configured.The addition of
@xmtp/content-type-group-updatedand@xmtp/content-type-textas external dependencies aligns with the SDK's architectural changes, ensuring proper dependency handling during bundling.Let's verify these dependencies are properly declared in package.json:
Also applies to: 13-13
✅ Verification successful
Dependencies are correctly declared in package.json
The verification confirms that both
@xmtp/content-type-group-updatedand@xmtp/content-type-textare properly declared in the package.json dependencies with appropriate version constraints:
@xmtp/content-type-group-updated: "^1.0.0"@xmtp/content-type-text: "^1.0.0"This aligns perfectly with their external configuration in rollup.config.js.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies are properly declared in package.json # Expected: Both dependencies should be listed in package.json # Test: Check package.json for the dependencies jq '.dependencies | select(."@xmtp/content-type-group-updated" != null and ."@xmtp/content-type-text" != null)' sdks/node-sdk/package.jsonLength of output: 339
sdks/node-sdk/src/DecodedMessage.ts (2)
15-16: LGTM! Type changes align with PR objectives.The property type changes correctly implement:
- Generic typing for content
- Optional content type support
59-68: Verify error handling and add documentation for undefined cases.While the implementation correctly handles undefined content types, consider:
- Adding type guards around
decodeContentcall- Documenting the undefined behavior in class/method JSDoc
- Consider logging when content type is undefined for debugging
Let's verify the error handling in the codebase:
✅ Verification successful
Based on the shell script results, I can now provide a final response as we have sufficient information about the error handling and content type handling in the codebase:
Error handling for undefined content type is properly implemented
The current implementation is correct and follows the established patterns in the codebase:
The
decodeContentmethod inClient.tsalready has proper error handling:
- Throws an error when no codec is found for the content type
- Has specific error handling for group membership changes
- Uses type assertions appropriately with ESLint comments
The undefined case is intentionally handled as part of the API design:
- Content type is optional by design (type can be undefined)
- The conditional assignment pattern is consistent with the codebase
- Silent undefined for content is appropriate as it represents an optional field
No changes are needed as the current implementation aligns with the codebase's error handling patterns and type safety measures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if undefined content types are documented and handled consistently rg -A 2 "undefined.*content.*type" --type ts # Check for existing error handling patterns rg "decodeContent.*error" --type tsLength of output: 84
Script:
#!/bin/bash # Let's check the implementation of decodeContent and its error handling ast-grep --pattern 'decodeContent($$$) { $$$ }' # Check for any error handling patterns in the class rg -B 2 -A 4 "throw.*Error" sdks/node-sdk/src/DecodedMessage.ts # Look for documentation about content type handling rg -B 2 "contentType" sdks/node-sdk/src/DecodedMessage.ts # Check interface or type definitions related to message content fd "\.d\.ts$" --exec rg -l "content.*type"Length of output: 572
Script:
#!/bin/bash # Let's check the Client class implementation for decodeContent ast-grep --pattern 'class Client { $$$ decodeContent($$$) { $$$ } $$$ }' # Check for any type definitions or interfaces for the message parameter rg -B 2 "interface.*Message" --type ts rg -B 2 "type.*Message" --type ts # Look for any existing documentation about content handling rg -B 2 "content:" --type ts sdks/node-sdk/src/Length of output: 19679
sdks/node-sdk/package.json (1)
51-51: LGTM! Verify compatibility with existing content type packages.The addition of
@xmtp/content-type-group-updatedaligns with the PR objective of replacing the temporary group updated codec with the official content type. The version constraint and scoping are consistent with other content type dependencies.Let's verify the compatibility of content type packages:
✅ Verification successful
LGTM! Content type package versions are compatible
The version analysis shows:
@xmtp/content-type-group-updated@^1.0.0is consistently used in both node-sdk and browser-sdk- All content type packages use compatible semver ranges (^1.0.x)
- The primitives package shows minor variations (^1.0.1 and ^1.0.2) which are backward compatible
- No conflicting version requirements were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential version conflicts or inconsistencies between content type packages # Test: List all @xmtp/content-type-* dependencies and their versions across the monorepo echo "Checking content type package versions across the monorepo:" rg -A 1 '"@xmtp/content-type-[^"]+": "[^"]+"' --type jsonLength of output: 2332
sdks/node-sdk/src/Conversations.ts (1)
30-37: LGTM! Generic type parameter enhances type safetyThe addition of the generic type parameter
<T = any>improves type safety while maintaining backward compatibility through the default type. This change aligns well with the PR objectives and the updates to theDecodedMessageclass.sdks/node-sdk/src/Conversation.ts (1)
192-198: Implementation looks good, but verify impact on consumers.The changes align well with the PR objectives by filtering out messages with undefined content. However, there are a few considerations:
- Consider filtering before mapping to DecodedMessage for better performance
- The strict undefined check might want to include null values
Consider this performance optimization:
messages(options?: NapiListMessagesOptions): DecodedMessage[] { return ( this.#group .findMessages(options) - .map((message) => new DecodedMessage(this.#client, message)) - // filter out messages without content - .filter((message) => message.content !== undefined) + .filter((message) => { + const decoded = new DecodedMessage(this.#client, message); + return decoded.content !== undefined; + }) ); }Let's verify the impact on existing consumers:
✅ Verification successful
Based on the search results, I can see extensive usage of the
messages()method across the codebase, particularly in tests. The key observations are:
- All tests expect and handle messages with undefined content correctly
- The filtering is already part of expected behavior in tests
- The performance concern about creating DecodedMessage instances before filtering is not critical since:
- Tests show small message sets
- The operation is not in a tight loop
- The mapping provides type safety benefits
The review comment's concerns have been addressed by the codebase evidence
The implementation is correct and aligns with existing usage patterns. The filtering of undefined content messages is an expected behavior as demonstrated by numerous test cases. The performance suggestion is not necessary given the typical usage patterns shown in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of the messages method to identify potential impacts rg -l "\.messages\(" | xargs rg -B 5 -A 5 "\.messages\("Length of output: 53138
sdks/node-sdk/src/Client.ts (3)
Line range hint
1-270: Overall implementation looks solid.The changes successfully implement the intended updates:
- Migration to official group update content type
- Improved error handling for group messages
- Proper type safety checks
The code is well-structured and maintains good practices.
Line range hint
142-142: Consider documenting codec order significance.The addition of
GroupUpdatedCodecbefore other codecs might affect message handling priority. Consider documenting if the order of codecs is significant for message processing.#!/bin/bash # Check if codec order is mentioned in documentation rg -i "codec.*order|priority" docs/
3-6: Verify dependency on @xmtp/content-type-group-updated package.The change from local codec to an external package looks good, but let's ensure the dependency is properly added.
✅ Verification successful
Dependency verification successful
The
@xmtp/content-type-group-updatedpackage is properly listed in the dependencies section of package.json with version^1.0.0. The import change is valid and properly supported.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the package is listed in package.json jq '.dependencies["@xmtp/content-type-group-updated"]' sdks/node-sdk/package.jsonLength of output: 177
Script:
#!/bin/bash # Check package.json content cat sdks/node-sdk/package.jsonLength of output: 2258
Summary
undefinedcontent type and content in messagesConversation.messagesDecodedMessageclass andConversations.findMessageByIdSummary by CodeRabbit
Release Notes
New Features
undefinedas a valid content type.Improvements
Chores