-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: parseEntities returns array instead of record<string, parsedenti… #382
Conversation
…ty<T>>
WalkthroughThis pull request introduces a significant change to the Changes
Sequence DiagramsequenceDiagram
participant Client
participant ParseEntities
participant ToriiClient
Client->>ToriiClient: Request Entities
ToriiClient-->>ParseEntities: Return Raw Entities
ParseEntities->>ParseEntities: Transform Entities
ParseEntities-->>Client: Return Array of Parsed Entities
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/sdk/src/getEntities.ts (1)
49-57
: Consider adding input validation for query parameters.The
toriiQuery
construction could benefit from validation of the input parameters, especially forlimit
andoffset
, to ensure they are non-negative values.+ if (limit < 0 || offset < 0) { + throw new Error('Limit and offset must be non-negative values'); + } + const toriiQuery: torii.Query = { limit, offset: cursor, order_by: orderBy, entity_models: entityModels, clause, dont_include_hashed_keys: dontIncludeHashedKeys, entity_updated_after: entityUpdatedAfter, };packages/sdk/src/getEventMessages.ts (1)
61-83
: Consider handling rate limits and retries.The single fetch approach might be more susceptible to rate limiting or temporary network issues. Consider adding retry logic for resilience.
+ const MAX_RETRIES = 3; + const RETRY_DELAY = 1000; // ms + + async function fetchWithRetry(attempt = 1): Promise<any> { + try { + return await client.getEventMessages(toriiQuery, historical); + } catch (error) { + if (attempt < MAX_RETRIES && error.message?.includes('rate limit')) { + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return fetchWithRetry(attempt + 1); + } + throw error; + } + } + try { - const entities = await client.getEventMessages(toriiQuery, historical); + const entities = await fetchWithRetry();packages/sdk/src/__tests__/parseEntities.test.ts (1)
225-227
: Consider adding array bounds check.While optional chaining handles undefined values, an explicit check for array length would make the tests more robust.
+ expect(res).toHaveLength(1); expect(res[0]?.models?.onchain_dash?.CallerCounter?.timestamp).toEqual( expected );
Also applies to: 279-279, 336-336
.changeset/curvy-queens-whisper.md (1)
16-16
: Enhance the changeset description with migration guidance.While the description clearly states the change, it would be helpful to include:
- Rationale for the change
- Migration guide for existing consumers
- Example of how to adapt existing code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/curvy-queens-whisper.md
(1 hunks)examples/example-vite-react-phaser-recs/tsconfig.json
(1 hunks)packages/sdk/src/__tests__/parseEntities.test.ts
(4 hunks)packages/sdk/src/getEntities.ts
(1 hunks)packages/sdk/src/getEventMessages.ts
(1 hunks)packages/sdk/src/parseEntities.ts
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (5)
packages/sdk/src/getEntities.ts (1)
59-80
: Verify pagination behavior after removing batch fetching.The removal of the batch fetching mechanism might affect the handling of large datasets. Ensure that:
- The client can handle the specified limit efficiently
- Upstream systems are aware of the change in pagination behavior
- Performance implications are understood for large result sets
✅ Verification successful
Pagination behavior verified after batch fetching removal
The pagination mechanism remains intact and properly implemented through limit/offset parameters. The removal of batch fetching doesn't affect the pagination behavior as it's handled at the query level through ToriiQueryBuilder. Large datasets are efficiently managed through the standard pagination interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for pagination-related code and comments rg -A 3 'limit|offset|pagination' --type tsLength of output: 29174
packages/sdk/src/getEventMessages.ts (1)
51-59
: LGTM! Query construction is consistent with getEntities.The
toriiQuery
construction follows the same pattern as ingetEntities.ts
, maintaining consistency across the codebase.packages/sdk/src/parseEntities.ts (1)
55-55
: Consider preserving entity order.
Object.values()
doesn't guarantee order preservation. If order matters, consider using an array from the start or maintaining order explicitly.packages/sdk/src/__tests__/parseEntities.test.ts (1)
143-186
: LGTM! Test cases properly validate the new array-based structure.The test expectations correctly reflect the change from object to array-based return type.
examples/example-vite-react-phaser-recs/tsconfig.json (1)
20-24
: LGTM! Verify path alias compatibility with build system.The TypeScript path alias configuration looks good and follows common practices. However, ensure that your build system (Vite) is configured to handle these aliases correctly.
Run this script to check if the path alias is properly configured in Vite:
✅ Verification successful
Path alias is properly configured in both TypeScript and Vite ✅
The path alias
@
is correctly set up in both:
tsconfig.json
:"@/*": ["./src/*"]
vite.config.ts
:"@": path.resolve(__dirname, "./src")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Vite configuration includes the same path alias # Search for path alias configuration in vite.config.ts rg -l 'resolve.*alias.*@/' examples/example-vite-react-phaser-recs/vite.config.ts if [ $? -ne 0 ]; then echo "Warning: Path alias might not be configured in Vite. Check vite.config.ts" echo "Add the following to vite.config.ts:" echo "resolve: { alias: { '@': '/src' } }" fiLength of output: 434
Script:
#!/bin/bash # Check if vite.config.ts exists and show its content if [ -f "examples/example-vite-react-phaser-recs/vite.config.ts" ]; then echo "vite.config.ts exists. Content:" cat examples/example-vite-react-phaser-recs/vite.config.ts else echo "vite.config.ts not found" # Check for alternative config files fd -t f "vite.config" examples/example-vite-react-phaser-recs/ fiLength of output: 660
@@ -8,7 +8,7 @@ export function parseEntities<T extends SchemaType>( | |||
options?: { logging?: boolean } | |||
): StandardizedQueryResult<T> { | |||
// @ts-ignore | |||
const result: StandardizedQueryResult<T> = entities; | |||
const result: Record<string, ParsedEntity> = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type declaration mismatch with return type.
The function's return type is StandardizedQueryResult<T>
but the implementation returns an array. This should be updated to match the new array-based return type.
-): StandardizedQueryResult<T> {
+): ParsedEntity<T>[] {
Committable suggestion skipped: line range outside the PR's diff.
"@dojoengine/sdk": patch | ||
"template-vite-ts": patch | ||
"@dojoengine/core": patch | ||
"@dojoengine/create-burner": patch | ||
"@dojoengine/create-dojo": patch | ||
"@dojoengine/predeployed-connector": patch | ||
"@dojoengine/react": patch | ||
"@dojoengine/state": patch | ||
"@dojoengine/torii-client": patch | ||
"@dojoengine/torii-wasm": patch | ||
"@dojoengine/utils": patch | ||
"@dojoengine/utils-wasm": patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update version bump type to reflect breaking changes.
All packages are currently marked for patch updates, but changing the return type of parseEntities
from Record<string, ParsedEntity<T>>
to Array<ParsedEntity<T>>
is a breaking change that will affect API consumers. This should be reflected in the version numbers.
Apply this diff to update the version bump type:
-"@dojoengine/sdk": patch
+"@dojoengine/sdk": major
Committable suggestion skipped: line range outside the PR's diff.
…ty>
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
parseEntities
function to return an array of parsed entities instead of an objectRefactor
getEntities
andgetEventMessages
functionsChores