fix: added missing params to getEventMessages() #373
Conversation
dont_include_hashed_keys, entity_updated_after
WalkthroughThe pull request introduces a comprehensive refactoring of parameter handling across multiple SDK functions. The primary change involves transforming function signatures from traditional parameter lists to destructured object formats. This modification affects functions like Changes
Possibly related PRs
Poem
Finishing Touches
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
🧹 Nitpick comments (6)
packages/sdk/src/subscribeEventQuery.ts (1)
Line range hint
8-26: Update JSDoc to reflect new parameter structure.The JSDoc comments need to be updated to match the new destructured object parameter format. This will help maintain accurate documentation for SDK users.
packages/sdk/src/subscribeEntityQuery.ts (2)
Line range hint
8-26: Update JSDoc to reflect new parameter structure.The JSDoc comments need to be updated to match the new destructured object parameter format.
Line range hint
37-43: Remove duplicate logging of convertQueryToEntityKeyClauses.The same query conversion is being logged twice. Consider removing the second logging instance to avoid redundancy.
if (options?.logging) { console.log("Query:", query); console.log( "convertQueryToEntityKeyClauses:", convertQueryToEntityKeyClauses(query, schema) ); } return client.onEntityUpdated( convertQueryToEntityKeyClauses(query, schema), (entityId: string, entityData: any) => { try { if (callback) { const parsedData = parseEntities<T>({ [entityId]: entityData, }); if (options?.logging) { - console.log( - "Converted query to entity key clauses:", - convertQueryToEntityKeyClauses(query, schema) - ); console.log("Parsed entity data:", parsedData); }Also applies to: 51-53
packages/sdk/src/getEntities.ts (1)
Line range hint
8-28: Update JSDoc to reflect new parameter structure and document new parameters.The JSDoc needs to be updated to:
- Match the new destructured object parameter format
- Document the new
dontIncludeHashedKeysandentityUpdatedAfterparameterspackages/sdk/src/getEventMessages.ts (1)
Line range hint
8-29: Update JSDoc to reflect new parameter structure.The JSDoc comments need to be updated to match the new destructured object parameter format and document all parameters, including
historical,dontIncludeHashedKeys, andentityUpdatedAfter.packages/sdk/src/types.ts (1)
419-422: Add JSDoc comments and consider renaming the parameter.
- Add JSDoc comments for the new parameters to maintain consistency with other parameters in the interface.
- Consider renaming
dontIncludeHashedKeystoincludeHashedKeysfor better clarity, as positive naming is generally preferred.// historical events historical?: boolean; - // hashed keys on events - dontIncludeHashedKeys?: boolean; - // entity updated after - entityUpdatedAfter?: number; + /** + * Whether to include hashed keys in the event data. + * Default is true. + */ + includeHashedKeys?: boolean; + /** + * Filter entities to only include those updated after this timestamp. + * Timestamp should be in milliseconds since epoch. + */ + entityUpdatedAfter?: number;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/sdk/src/getEntities.ts(3 hunks)packages/sdk/src/getEventMessages.ts(5 hunks)packages/sdk/src/index.ts(3 hunks)packages/sdk/src/subscribeEntityQuery.ts(2 hunks)packages/sdk/src/subscribeEventQuery.ts(2 hunks)packages/sdk/src/types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (6)
packages/sdk/src/subscribeEventQuery.ts (1)
28-38: LGTM! Clean parameter restructuring.The transition to a destructured object format with proper typing and default values improves code maintainability while maintaining backward compatibility.
packages/sdk/src/getEntities.ts (1)
29-44: LGTM! Well-structured parameter enhancement.The addition of
dontIncludeHashedKeysandentityUpdatedAfterparameters with proper default values enhances query flexibility while maintaining backward compatibility.Also applies to: 58-59
packages/sdk/src/getEventMessages.ts (2)
76-78: LGTM! Improved historical data handling.The simplified historical data handling with direct use of the
historicalparameter improves code readability.Also applies to: 100-102
41-42: Consider aligning default values across functions.
dontIncludeHashedKeysdefaults totruehere butfalseingetEntities. Consider aligning these defaults to maintain consistency across the SDK.Run this script to verify the impact of this inconsistency:
packages/sdk/src/index.ts (2)
49-49: LGTM! Parameter restructuring is consistent.The changes maintain a consistent pattern across all functions, improving code readability and maintainability.
Also applies to: 57-64, 82-94, 113-126
82-94: Verify the default behavior for the new parameters.Please ensure that the default behavior when
dontIncludeHashedKeysandentityUpdatedAfterare not provided maintains backward compatibility.Also applies to: 113-126
✅ Verification successful
Default behavior for new parameters maintains backward compatibility
The new parameters
dontIncludeHashedKeysandentityUpdatedAfterare properly implemented with safe defaults:
dontIncludeHashedKeysdefaults tofalseentityUpdatedAfterdefaults to0These defaults ensure that existing code continues to work without modification, maintaining the original behavior when parameters are not provided.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing usages of getEntities and getEventMessages to verify impact echo "Searching for getEntities usage..." rg "getEntities\(" -A 5 echo "Searching for getEventMessages usage..." rg "getEventMessages\(" -A 5Length of output: 12428
Script:
#!/bin/bash # Find getEntities function implementation and type definitions echo "Searching for getEntities function definition..." ast-grep --pattern 'export $_ function getEntities($_) { $$$ }' echo "Searching for getEntities interface/type definitions..." rg "interface.*getEntities|type.*getEntities" -A 10Length of output: 349
Script:
#!/bin/bash # Search for getEntities implementation and type definitions with refined patterns echo "Searching for getEntities implementation file..." fd "getEntities.ts" packages/sdk/src echo "Searching for getEntities function definition..." ast-grep --pattern 'export const getEntities = $_' echo "Searching for type definitions..." rg "type.*Get.*Entities|interface.*Get.*Entities" -A 10 echo "Showing content of getEntities implementation..." cat packages/sdk/src/getEntities.tsLength of output: 7389
Introduced changes
getEventMessages():dont_include_hashed_keys,entity_updated_aftergetEventMessages()andgetEntities()extendingGetParams<T>subscribeEntityQuery()andsubscribeEventQuery()reusingSubscribeParams<T>example, from...
to...
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores