fix: sdk function params refac to object param #337
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
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 (
|
![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 (5)
packages/sdk/src/index.ts (3)
Line range hint
60-70: Consider refactoring the underlying getEntities function.While the interface change is good, the implementation still passes individual parameters to the underlying function. Consider refactoring
./getEntitiesto also accept an object parameter to maintain consistency and avoid unnecessary parameter destructuring.
Line range hint
76-87: Consider refactoring the underlying getEventMessages function.Similar to getEntities, consider refactoring
./getEventMessagesto accept an object parameter for consistency across the codebase.
Line range hint
1-159: Consider completing the object parameter pattern throughout the codebase.While this PR successfully refactors the SDK's public interface to use object parameters, consider extending this pattern to all underlying functions (
./getEntities,./getEventMessages, etc.) in a follow-up PR. This would:
- Maintain consistent coding patterns
- Simplify parameter passing
- Make future parameter additions easier throughout the codebase
packages/sdk/src/types.ts (2)
347-361: Document default values for pagination parametersThe
GetParamsinterface includes pagination parameters but their default values are only mentioned in comments. Consider adding JSDoc comments with@defaulttags for better documentation:export interface GetParams<T extends SchemaType> { // The query object used to filter entities. query: QueryType<T>; // The callback function to handle the response. callback: (response: { data?: StandardizedQueryResult<T>; error?: Error; }) => void; - // The maximum number of entities to fetch per request. Default is 100. + /** + * The maximum number of entities to fetch per request. + * @default 100 + */ limit?: number; - // The offset to start fetching entities from. Default is 0. + /** + * The offset to start fetching entities from. + * @default 0 + */ offset?: number; // Optional settings. options?: SDKFunctionOptions; }
330-333: Consider expanding SDKFunctionOptions for future extensibilityThe
SDKFunctionOptionsinterface currently only includes a logging option. Consider adding more common configuration options that might be useful across SDK functions:export interface SDKFunctionOptions { /** * If true, enables logging of the fetching process * @default false */ logging?: boolean; /** * Timeout in milliseconds for the operation * @default 30000 */ timeout?: number; /** * Number of retry attempts for failed operations * @default 3 */ retryAttempts?: number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk/src/index.ts(4 hunks)packages/sdk/src/types.ts(2 hunks)
🔇 Additional comments (5)
packages/sdk/src/index.ts (3)
44-48: LGTM! Good refactoring to object parameters.
The change to use object parameters improves code readability and makes the function more maintainable. It also provides better flexibility for future parameter additions.
52-56: LGTM! Consistent with subscribeEntityQuery changes.
The refactoring maintains consistency across the SDK interface.
Line range hint 130-152: Verify the impact of the isSessionSignature parameter.
The addition of the isSessionSignature parameter to publishMessage could be a breaking change. Please ensure all consumers of this API are updated accordingly.
packages/sdk/src/types.ts (2)
266-302: Well-structured parameter object refactoring!
The refactoring of method signatures to use parameter objects (SubscribeParams<T> and GetParams<T>) is a positive change that:
- Makes the API more flexible for future parameter additions
- Improves readability at call sites
- Makes optional parameters easier to handle
- Maintains backward compatibility through unchanged return types
The documentation is also properly updated to reflect these changes.
266-302: Verify the impact of parameter object refactoring
The changes to method signatures could potentially break existing code. Let's verify the impact:
Also applies to: 330-361
✅ Verification successful
Based on the search results, I can now generate the final response:
Parameter object refactoring is safely implemented
The changes to method signatures are already properly implemented throughout the codebase:
- All usages in
packages/sdk/src/index.tsare using the new parameter object syntax - Example usages in
examples/directory already follow the new parameter object pattern - Test files in
packages/sdk/src/__tests__/are aligned with the changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of the affected methods
echo "Searching for method usages that might need updates..."
rg -A 2 "subscribeEntityQuery|subscribeEventQuery|getEntities|getEventMessages" --type ts
# Look for potential test files that need updating
echo "Checking for affected test files..."
fd -e test.ts -e spec.ts
Length of output: 14482
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
sendMessagefunction with a new optional parameter to differentiate session signatures.Bug Fixes
Documentation