fix: sdk remove old code #428
Conversation
WalkthroughThe changes consist of removing several unit test files and conversion functions that transformed query objects into clause formats, including entity key clause conversions. Additionally, existing files were updated to use type-only import syntax for various type declarations, clarifying their intent. A minor refactoring in the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (3)
packages/sdk/src/queryBuilder.ts (1)
7-9: Deprecated API Reminder
The class documentation still marksQueryBuilder(and related entities likeNamespace) as deprecated with the suggestion to useClauseBuilderinstead. If the goal of the PR is to remove outdated SDK code, consider whether these deprecated components should be removed entirely in a future cleanup or if a migration/deprecation timeline should be documented.packages/sdk/src/convertToMemberValue.ts (1)
30-36: BigInt Conversion Handling
Usingtorii.cairoShortStringToFelton the bigint string representation to produce aFelt252is a creative solution. Please verify that this conversion handles edge cases correctly (e.g., extremely large numbers) and that the invoked utility function behaves as expected.packages/sdk/src/clauseBuilder.ts (1)
107-110: Improve Type Safety on Clause Initialization
The constructor uses an@ts-expect-errorcomment to bypass a type assignment issue forthis.clause. Consider initializingthis.clausewith an explicit cast (e.g., using{} as Clause) to avoid suppressing TypeScript errors. This can improve maintainability and readability by making the intended type explicit.Proposed Diff:
- constructor() { - // @ts-expect-error It's ok if it's not assignable here. - this.clause = {}; - } + constructor() { + this.clause = {} as Clause; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/sdk/src/__tests__/convertQueryToClause.test.ts(0 hunks)packages/sdk/src/__tests__/convertQueryToEntityKeyClauses.test.ts(0 hunks)packages/sdk/src/clauseBuilder.ts(1 hunks)packages/sdk/src/convertQueryToEntityKeyClauses.ts(0 hunks)packages/sdk/src/convertQuerytoClause.ts(0 hunks)packages/sdk/src/convertToMemberValue.ts(1 hunks)packages/sdk/src/queryBuilder.ts(1 hunks)packages/sdk/src/toriiQueryBuilder.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/sdk/src/tests/convertQueryToEntityKeyClauses.test.ts
- packages/sdk/src/convertQuerytoClause.ts
- packages/sdk/src/tests/convertQueryToClause.test.ts
- packages/sdk/src/convertQueryToEntityKeyClauses.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (7)
packages/sdk/src/queryBuilder.ts (1)
1-1: Type-Only Import Update
Changing the standard import to a type-only import forQueryType,SchemaType, andSubscriptionQueryTypeclarifies that these are used solely for type checking and will not be emitted in the runtime JavaScript. This aligns well with TypeScript best practices.packages/sdk/src/convertToMemberValue.ts (4)
22-24: Number Conversion Check
The conversion for numbers maps the input to aU32key as expected. Please ensure that existing test cases verify that numbers are correctly processed under all expected conditions.
26-28: Boolean Conversion Confirmation
The conversion for boolean values uses theBoolkey, which matches the intended output for Torii primitives. This logic is straightforward and correct.
38-40: String Conversion Logic
Mapping a string value directly to an object with aStringkey is consistent with the expected output format. Ensure that any consumer of this conversion is prepared to handle the different key names compared to numeric/boolean cases.
42-44: Array Conversion Review
The conversion for arrays is implemented recursively, which is a good approach to ensure consistent conversion for nested structures. Please confirm that deeply nested arrays or mixed type arrays are covered in unit tests.packages/sdk/src/toriiQueryBuilder.ts (1)
1-2: Type-Only Imports for Clarity
Switching imports forClause,OrderBy, andQuery(as well as forSchemaType) to type-only imports is a solid update that reduces runtime overhead and signals their purpose as annotations only.packages/sdk/src/clauseBuilder.ts (1)
1-11: Consistency in Import Declarations
The update to use type-only imports forClause,ComparisonOperator,MemberValue,PatternMatching, andSchemaType(while keepingconvertToPrimitiveas a regular import) improves clarity. This clearly separates the type information from the runtime values and is consistent with recent updates across the SDK.
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
Chores
Refactor