feat: add useModels hook #420
Conversation
WalkthroughThis pull request introduces a new changelog file documenting patch updates for multiple Dojo Engine packages and fixes related to Cairo options and enum handling. It updates example app logic by modifying address handling and importing a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant SDK as SDK.init Function
participant DC as Default Config
participant Store as State Store
App->>SDK: Call init(options)
SDK->>DC: Retrieve defaultClientConfig
SDK->>SDK: Merge options.client with default config
SDK->>Store: Initialize subscriptions & error handling
Store-->>SDK: Return event data/errors
sequenceDiagram
participant Comp as Component
participant UM as useModels Hook
participant Dojo as useDojoStore
participant GE as getEntitiesByModel
Comp->>UM: Invoke useModels("namespace-modelName")
UM->>Dojo: Access global store
Dojo->>GE: Fetch entities by model
GE-->>Dojo: Return entity map
Dojo-->>UM: Provide entities
UM-->>Comp: Return mapped entities
Possibly related PRs
Poem
🪧 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: 1
🧹 Nitpick comments (5)
examples/example-vite-react-sdk/src/App.tsx (1)
13-13: Import added but not usedThe
useModelshook is imported but never used in this component. Consider either removing the unused import or implementing it where needed.- useModels,packages/sdk/src/react/hooks.ts (1)
75-75: Avoid using 'any' typeThe
anytype disables TypeScript's type checking features. Consider using a more specific type or a generic constraint for better type safety.- Client extends (...args: any) => any, + Client extends (...args: unknown[]) => unknown,🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/utils/src/utils/index.ts (2)
382-391: Well-implemented default port helper functionThe
getDefaultPortForProtocolfunction provides a clean way to determine default ports for different protocols. Consider adding more protocols likehttp:andws:for completeness.function getDefaultPortForProtocol(protocol: string): number { switch (protocol) { case "https:": return 443; case "wss:": return 443; + case "http:": + return 80; + case "ws:": + return 80; default: return 80; } }
436-442: Special protocol handling could use a commentThe special handling for
wssprotocol (converting tox-parity-wss) might not be obvious to users. Consider adding a comment explaining why this transformation is necessary.// Append protocol if it's not http or https if (!["http", "https"].includes(protocol)) { + // Convert wss to x-parity-wss for compatibility with multiaddr protocol naming conventions // Replace wss with x-parity-wss const protocolToAppend = protocol === "wss" ? "x-parity-wss" : protocol; multiaddr += `/${protocolToAppend}`; }packages/sdk/src/state/zustand.ts (1)
77-133: Fix theanytype usage in enum handling.The implementation uses
anytype fornewEnumContentwhich could be more strictly typed to improve type safety.- const newEnumContent: Record<string, any> = {}; + const newEnumContent: Record<string, unknown> = {};Also apply this change for the second
newEnumContentdeclaration in this function.🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.changeset/brave-kiwis-explode.md(1 hunks)examples/example-vite-react-sdk/src/App.tsx(2 hunks)examples/example-vite-react-sdk/src/main.tsx(0 hunks)package.json(1 hunks)packages/sdk/src/index.ts(5 hunks)packages/sdk/src/parseEntities.ts(1 hunks)packages/sdk/src/react/hooks.ts(2 hunks)packages/sdk/src/state/zustand.ts(6 hunks)packages/sdk/src/types.ts(2 hunks)packages/utils/src/_test_/utils/index.test.ts(2 hunks)packages/utils/src/utils/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- examples/example-vite-react-sdk/src/main.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/state/zustand.ts
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/react/hooks.ts
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🔇 Additional comments (26)
.changeset/brave-kiwis-explode.md (1)
1-15: Well-structured changeset file for version managementThis changeset file properly documents patch updates for multiple Dojo Engine packages and specifically mentions a fix for "cairo option and enum ignore in zustand merge". This corresponds to the code changes we see in the parseEntities.ts file.
Using changesets is a good practice for versioning in monorepos, ensuring changes are properly tracked and documented before release.
package.json (1)
14-14: Development environment enhancementAdding the "watch" parameter to the dev script enables automatic rebuilding when files change during development, which improves developer experience by providing faster feedback during the implementation of features like the new
useModelshook mentioned in the PR title.packages/sdk/src/parseEntities.ts (1)
79-80: Cairo Option handling fixThis change corrects the control flow for handling Cairo enum values by decoupling the checks for "Some" and "None" options. Previously, the "None" check was only evaluated if the "Some" check failed, which could lead to incorrect behavior in certain scenarios.
This aligns with the changeset description mentioning a fix for "cairo option and enum ignore in zustand merge".
packages/utils/src/_test_/utils/index.test.ts (2)
6-6: Adding new utility importThe import statement is updated to include the new
convertToRelayUrifunction that will be tested later in the file.
109-128: Comprehensive test coverage for URI conversion utilityThis test case verifies the
convertToRelayUrifunction's behavior with four different input URI formats:
- Standard HTTP with localhost
- HTTP with IP address
- HTTPS with path segments
- WSS with path segments
The test ensures that each URI is correctly converted to its multiaddr representation, which is important for properly configuring network connections in the Dojo ecosystem.
examples/example-vite-react-sdk/src/App.tsx (1)
39-43: Improved address handlingThis change enhances the clarity of address handling by explicitly using
undefinedwhen the address is not available, rather than defaulting to a placeholder value. This approach makes the intent clearer and prevents potential issues with using "0" as a fallback.packages/sdk/src/types.ts (2)
458-460: Good type refinement for client configurationThe new
SDKClientConfigtype enhances type safety by ensuring thatworldAddressis always provided while making other properties fromtorii.ClientConfigoptional. This is a good practice for configuration objects, allowing flexibility while guaranteeing required properties.
470-470: Improved client property typeUpdating the
clientproperty to use the newSDKClientConfigtype ensures consistent typing throughout the SDK and prevents configuration errors at compile time.packages/sdk/src/react/hooks.ts (1)
66-89: Good implementation of the useModels hookThe addition of the
useModelshook complements the existinguseModelhook by providing a way to retrieve all entities with a specific model. This is a valuable addition for components that need to display collections of entities.🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/utils/src/utils/index.ts (1)
404-454: Comprehensive URI conversion utilityThe
convertToRelayUrifunction is well-implemented with thorough handling of different URI formats (IPv4, IPv6, domain names) and good error handling. The detailed comments and examples make it very clear how to use this function.packages/sdk/src/index.ts (7)
2-2: Good type import optimization.Changing the import to a type import ensures these types are only used for type checking and not at runtime, which can lead to more efficient bundling.
4-14: Good type safety improvement.Converting regular imports to type imports for type-only dependencies helps with cleaner code separation between runtime values and type information.
18-18: Type import is properly implemented.The
ToriiQueryBuilderis correctly imported as a type since it's only used for type annotations.
38-41: Default client configuration is a good practice.Adding default values for configuration parameters is a good practice as it improves usability by requiring less boilerplate from users.
52-56: Good fallback configuration implementation.This creates a client configuration by merging default values with user-provided options, allowing users to override only the values they need to customize while maintaining sensible defaults for the rest.
95-98: Improved error handling.The error check ensures that errors are always wrapped in an Error object, which provides more consistent error handling behavior.
162-164: Consistent error handling pattern.This follows the same error handling pattern as implemented elsewhere in the codebase, ensuring errors are always proper Error objects with stack traces.
packages/sdk/src/state/zustand.ts (9)
9-9: Appropriate import for Cairo-specific types.Adding imports for
CairoCustomEnum,CairoOption, andCairoOptionVariantis necessary for the new type-specific handling functions.
24-31: Good type guard implementation.The
isCairoOptiontype guard function is well-implemented using the instanceof check, which enables TypeScript to narrow types in conditional blocks.
33-66: Well-structured Cairo option merge function.The
mergeCairoOptionfunction correctly handles merging CairoOption instances with appropriate priority logic, preferring "Some" values from the source when available.
68-75: Consistent type guard implementation.The
isCairoCustomEnumtype guard follows the same pattern asisCairoOption, providing consistent type narrowing capabilities.
147-152: Good extension of the deepMerge function.The updated
deepMergefunction now handles Cairo-specific types correctly by delegating to specialized merge functions, which is a clean approach to extending functionality.
197-201: Improved loop syntax.Changing from
forEachtofor...ofloops provides better readability and is generally more idiomatic in modern JavaScript/TypeScript.
216-217: Good flow control improvement.Using
continueinstead of areturnstatement maintains the loop flow more clearly, making the code more maintainable.
223-244: Enhanced entity merging logic.The updated entity merging logic correctly uses the
deepMergefunction to handle Cairo types, ensuring proper merging of complex nested structures.
272-292: Consistent application of the merging pattern.The same merging pattern is applied consistently in the
updateEntitymethod, which is good for maintainability and predictable behavior.
| const modelData = useDojoStore((state) => | ||
| state.getEntitiesByModel(namespace, modelName).map((entity) => ({ | ||
| [entity.entityId]: entity.models?.[namespace]?.[modelName], | ||
| })) | ||
| ) as unknown as { [entityId: string]: SchemaType[N][M] | undefined }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with return value mapping
The current implementation is mapping entities to an array of objects (each with a single key-value pair) but then casting it to a single object. This might not work as intended.
- const modelData = useDojoStore((state) =>
- state.getEntitiesByModel(namespace, modelName).map((entity) => ({
- [entity.entityId]: entity.models?.[namespace]?.[modelName],
- }))
- ) as unknown as { [entityId: string]: SchemaType[N][M] | undefined };
+ const modelData = useDojoStore((state) =>
+ state.getEntitiesByModel(namespace, modelName).reduce((acc, entity) => ({
+ ...acc,
+ [entity.entityId]: entity.models?.[namespace]?.[modelName],
+ }), {})
+ ) as { [entityId: string]: SchemaType[N][M] | undefined };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const modelData = useDojoStore((state) => | |
| state.getEntitiesByModel(namespace, modelName).map((entity) => ({ | |
| [entity.entityId]: entity.models?.[namespace]?.[modelName], | |
| })) | |
| ) as unknown as { [entityId: string]: SchemaType[N][M] | undefined }; | |
| const modelData = useDojoStore((state) => | |
| state.getEntitiesByModel(namespace, modelName).reduce((acc, entity) => ({ | |
| ...acc, | |
| [entity.entityId]: entity.models?.[namespace]?.[modelName], | |
| }), {}) | |
| ) as { [entityId: string]: SchemaType[N][M] | undefined }; |
MartianGreed
force-pushed
the
feat/sdk-hooks
branch
from
e71507e to
e282c5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/sdk/src/react/hooks.ts (1)
66-89:⚠️ Potential issueFix the return value mapping in useModels hook.
The current implementation has a logic error. It maps entities to an array of objects (each with a single key-value pair) and then incorrectly casts it to a single object, which won't work as expected at runtime.
Apply this correction to properly accumulate entities into a single object:
const modelData = useDojoStore((state) => - state.getEntitiesByModel(namespace, modelName).map((entity) => ({ - [entity.entityId]: entity.models?.[namespace]?.[modelName], - })) -) as unknown as { [entityId: string]: SchemaType[N][M] | undefined }; + state.getEntitiesByModel(namespace, modelName).reduce((acc, entity) => ({ + ...acc, + [entity.entityId]: entity.models?.[namespace]?.[modelName], + }), {}) +) as { [entityId: string]: SchemaType[N][M] | undefined };🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🧹 Nitpick comments (4)
packages/sdk/src/react/hooks.ts (2)
75-75: Consider adding a more specific type than 'any'.The
Client extends (...args: any) => anytype parameter usesany, which reduces type safety. If possible, define a more specific interface for the client function.🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
251-251: Remove unnecessary empty line.There's an extra blank line here that doesn't serve any purpose.
packages/utils/src/utils/index.ts (1)
393-454: Good implementation of URI conversion with complete error handlingThe
convertToRelayUrifunction is well-documented with examples and includes proper error handling. The implementation correctly handles different address types (IPv4, IPv6, domain names) and includes special case handling for protocol conversion.A few minor suggestions for improvement:
- The IPv4 regex pattern (
/^\d+\.\d+\.\d+\.\d+$/) is simplistic and might not validate all IPv4 addresses correctly.- Consider adding more robust validation for IPv6 addresses.
- if (hostname.match(/^\d+\.\d+\.\d+\.\d+$/)) { + // More robust IPv4 validation with octet range check (0-255) + if (hostname.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/) && + hostname.split('.').every(octet => parseInt(octet, 10) <= 255)) {packages/sdk/src/state/zustand.ts (1)
68-133: Type-safe implementation for CairoCustomEnum handling with minor typing issuesThe functions for checking and merging CairoCustomEnum types are well-implemented with appropriate fallbacks. The code correctly handles active variants and maintains type safety.
However, there's a static analysis warning about using
anytypes at lines 96 and 116:- const newEnumContent: Record<string, any> = {}; + const newEnumContent: Record<string, unknown> = {};This would improve type safety while still allowing for the dynamic property assignment pattern you're using.
🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (14)
.changeset/brave-kiwis-explode.md(1 hunks)examples/example-vite-react-sdk/src/App.tsx(1 hunks)examples/example-vite-react-sdk/src/main.tsx(0 hunks)package.json(1 hunks)packages/sdk/src/experimental/index.ts(1 hunks)packages/sdk/src/index.ts(5 hunks)packages/sdk/src/parseEntities.ts(1 hunks)packages/sdk/src/react/hooks.ts(2 hunks)packages/sdk/src/state/zustand.ts(6 hunks)packages/sdk/src/types.ts(2 hunks)packages/torii-wasm/package.json(0 hunks)packages/utils/src/_test_/utils/index.test.ts(2 hunks)packages/utils/src/utils/index.ts(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/torii-wasm/package.json
- examples/example-vite-react-sdk/src/main.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/utils/src/test/utils/index.test.ts
- package.json
- packages/sdk/src/parseEntities.ts
- examples/example-vite-react-sdk/src/App.tsx
- .changeset/brave-kiwis-explode.md
- packages/sdk/src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/react/hooks.ts
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/state/zustand.ts
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
turbo.json (1)
19-19: Build dependency chain update looks good.The addition of
^build:depsto the build task's dependencies ensures that all dependency builds are completed before the main build runs, which improves build reliability and correctness.packages/sdk/src/types.ts (2)
458-460: Improved type definition for client configuration.The new
SDKClientConfigtype provides better type safety by makingworldAddressrequired while keeping other client config properties optional. This enables more flexible configuration with defaults.
470-470: LGTM! Client type updated correctly.The client property now uses the new
SDKClientConfigtype, which aligns with the client configuration merging implementation.packages/sdk/src/experimental/index.ts (2)
6-6: Added import for default client configuration.This import is necessary for the client configuration merging implemented below.
14-19: Improved client configuration with defaults.The implementation now properly merges default client configuration with user-provided options, allowing for sensible defaults while still enabling overrides. This is a standard pattern for configuration management.
packages/utils/src/utils/index.ts (1)
376-391: Well-implemented utility function with clear purposeThe
getDefaultPortForProtocolfunction is correctly implemented to return default ports for different protocols, with appropriate JSDoc documentation. The implementation is straightforward and handles the common cases correctly.packages/sdk/src/state/zustand.ts (3)
24-66: Good implementation of CairoOption handlingThe
isCairoOptionandmergeCairoOptionfunctions are well-documented and correctly implement type checking and merging logic for CairoOption instances. The merge logic appropriately prioritizes "Some" values over "None" values.
147-152: Good integration with deepMerge functionThe special case handling for Cairo types in the deepMerge function is well-implemented, correctly delegating to the appropriate merge functions based on type checks.
197-200: Improved loop structure and merging logicChanging from
forEachtofor...ofloops improves readability and the use ofcontinueinstead ofreturnstatements maintains proper loop flow. The updated merging logic correctly leverages the new type-specific merge functions.Also applies to: 205-244, 272-292
MartianGreed
force-pushed
the
feat/sdk-hooks
branch
from
e282c5b to
0e64e57
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/sdk/src/react/hooks.ts (1)
66-89:⚠️ Potential issueGood addition of the useModels hook, but mapping implementation needs correction
The new
useModelshook is a valuable addition that complements the existinguseModelfunction. However, the implementation has an issue in how it's transforming the data.The current implementation maps entities to an array of objects (each with a single key-value pair) but then casts it to a single object. This won't work as intended because it will result in an array structure rather than the expected object structure.
- const modelData = useDojoStore((state) => - state.getEntitiesByModel(namespace, modelName).map((entity) => ({ - [entity.entityId]: entity.models?.[namespace]?.[modelName], - })) - ) as unknown as { [entityId: string]: SchemaType[N][M] | undefined }; + const modelData = useDojoStore((state) => + state.getEntitiesByModel(namespace, modelName).reduce((acc, entity) => ({ + ...acc, + [entity.entityId]: entity.models?.[namespace]?.[modelName], + }), {}) + ) as { [entityId: string]: SchemaType[N][M] | undefined };This implementation uses
reduceto properly combine all entity data into a single object, creating the expected return type structure.🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🧹 Nitpick comments (2)
packages/sdk/src/react/hooks.ts (1)
72-76: Type safety improvement needed for generic parametersThe
Clientgeneric parameter is using theanytype, which bypasses type safety checks.Consider using a more specific type for the
Clientparameter:export function useModels< N extends keyof SchemaType, M extends keyof SchemaType[N] & string, - Client extends (...args: any) => any, + Client extends (...args: unknown[]) => unknown, Schema extends SchemaType >(model: `${N}-${M}`): { [entityId: string]: SchemaType[N][M] | undefined } {This provides better type safety while maintaining the flexibility needed for different client implementations.
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/state/zustand.ts (1)
83-133: Well-implemented mergeCairoCustomEnum function with type improvement neededThe
mergeCairoCustomEnumfunction appropriately handles the merging of CairoCustomEnum instances by prioritizing the source's active variant. However, there's a use ofanythat should be addressed.Replace the
anytype with a more specific type:- const newEnumContent: Record<string, any> = {}; + const newEnumContent: Record<string, unknown> = {};Same change should be applied to the similar declaration on line 116.
This change maintains the flexibility needed while improving type safety.
🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (14)
.changeset/brave-kiwis-explode.md(1 hunks)examples/example-vite-react-sdk/src/App.tsx(1 hunks)examples/example-vite-react-sdk/src/main.tsx(0 hunks)package.json(1 hunks)packages/sdk/src/experimental/index.ts(1 hunks)packages/sdk/src/index.ts(5 hunks)packages/sdk/src/parseEntities.ts(1 hunks)packages/sdk/src/react/hooks.ts(2 hunks)packages/sdk/src/state/zustand.ts(6 hunks)packages/sdk/src/types.ts(2 hunks)packages/torii-wasm/package.json(0 hunks)packages/utils/src/_test_/utils/index.test.ts(2 hunks)packages/utils/src/utils/index.ts(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/torii-wasm/package.json
- examples/example-vite-react-sdk/src/main.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/sdk/src/parseEntities.ts
- packages/sdk/src/types.ts
- examples/example-vite-react-sdk/src/App.tsx
- packages/utils/src/test/utils/index.test.ts
- packages/sdk/src/experimental/index.ts
- turbo.json
- .changeset/brave-kiwis-explode.md
- package.json
- packages/sdk/src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/react/hooks.ts
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/state/zustand.ts
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (7)
packages/utils/src/utils/index.ts (2)
377-391: Well-structured helper function for determining default ports.The function logic correctly assigns port 443 to secure protocols (HTTPS, WSS) and 80 to other protocols, which is appropriate for web and WebSocket services. The function is properly scoped as internal (not exported) and includes good JSDoc documentation.
393-454:❓ Verification inconclusive
Good implementation of URI to multiaddr conversion with comprehensive documentation.
The function properly handles different address types (IPv4, IPv6, domain) and includes appropriate error handling. The examples in the documentation are particularly helpful.
There are a couple of points to consider:
- The URL pathname encoding in line 447 might cause double-encoding if the path already contains encoded components.
- The function has special handling for the "wss" protocol, converting it to "x-parity-wss", which suggests this utility was created with specific use cases in mind.
Let's verify the behavior with some examples:
🏁 Script executed:
#!/bin/bash # Create a small test script to verify the behavior of convertToRelayUri cat > test-uri-conversion.js << 'EOF' const { convertToRelayUri } = require('./packages/utils/src/utils/index.ts'); // Test cases const testCases = [ "http://example.com", "https://example.com:8443", "http://127.0.0.1:8080", "http://[::1]:8080", "wss://relay.dojo.io", "http://example.com/path%20with%20spaces" ]; for (const uri of testCases) { console.log(`${uri} → ${convertToRelayUri(uri)}`); } EOF # Check if ts-node is available for running TypeScript directly if command -v npx &> /dev/null && npx ts-node -v &> /dev/null; then echo "Running URI conversion tests..." npx ts-node test-uri-conversion.js else echo "ts-node not available. Cannot run the test directly." echo "The test would verify convertToRelayUri for various URI formats." fiLength of output: 1828
Attention: Manual verification required for test harness compatibility with ES Modules
The implementation of
convertToRelayUrilooks robust—it handles various address types (IPv4, IPv6, domain names), applies proper default port logic, and includes special handling for protocols (including the intended conversion of"wss"to"x-parity-wss"). The thorough inline documentation and examples are very helpful.However, please note the following points:
- URL Pathname Encoding: The use of
encodeURIComponentonurl.pathname(line 447) might lead to double encoding if the path is already encoded. Please verify that this behavior is intended.- Test Harness ES Module Issue: The provided test script fails because it uses
require()to loadindex.ts, which is an ES module (as indicated by"type": "module"in the package config). To proceed:
- Update the test script to use ES module syntax (e.g., replacing
require()with a dynamicimport()or converting the test file to a.mjsfile).- Re-run tests to validate the conversion outputs against the example cases.
Please adjust the testing approach accordingly and manually verify the functionality.
packages/sdk/src/state/zustand.ts (5)
29-31: Good implementation of the CairoOption type guardThis type guard is well implemented and will help with type safety when working with CairoOption instances.
39-66: Good implementation of the mergeCairoOption functionThe
mergeCairoOptionfunction correctly prioritizes the source value if it's aSomevariant, which is a logical merging strategy.
73-75: Good implementation of the CairoCustomEnum type guardThis type guard is well implemented and will help with type safety when working with CairoCustomEnum instances.
147-152: Good enhancements to the deepMerge functionThe additions to the
deepMergefunction properly handle the Cairo-specific types by delegating to the specialized merge functions.
197-201: Improved readability with for...of loops and proper deep mergingThe code changes from using
forEachtofor...ofloops and properly implements deep merging for Cairo types. This is a good improvement for readability and correctness.Also applies to: 205-217, 223-244, 272-292
MartianGreed
force-pushed
the
feat/sdk-hooks
branch
from
0e64e57 to
811ae57
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/sdk/src/react/hooks.ts (1)
82-86:⚠️ Potential issueFix the return value mapping implementation
The current implementation is mapping entities to an array of objects (each with a single key-value pair) but then casting it to a single object with string keys. This will not work correctly at runtime.
Apply this fix to correctly convert the array of entities into a single object:
- const modelData = useDojoStore((state) => - state.getEntitiesByModel(namespace, modelName).map((entity) => ({ - [entity.entityId]: entity.models?.[namespace]?.[modelName], - })) - ) as unknown as { [entityId: string]: SchemaType[N][M] | undefined }; + const modelData = useDojoStore((state) => + state.getEntitiesByModel(namespace, modelName).reduce((acc, entity) => ({ + ...acc, + [entity.entityId]: entity.models?.[namespace]?.[modelName], + }), {}) + ) as { [entityId: string]: SchemaType[N][M] | undefined };This uses
reduce()to properly accumulate the entities into a single object, eliminating the need for theunknowncast.
🧹 Nitpick comments (3)
packages/sdk/src/react/hooks.ts (2)
72-81: Type definitions match the existing patternThe type parameters follow the same pattern as other hooks in the file, maintaining consistency. However, consider replacing the
anytype with more specific types if possible to improve type safety.- Client extends (...args: any) => any, + Client extends (...args: unknown[]) => unknown,🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
251-251: Remove unnecessary blank lineThere's an extra blank line that doesn't serve any purpose.
-packages/sdk/src/state/zustand.ts (1)
96-96: Consider replacing 'any' with more specific types.The static analysis tools flagged the use of
anytype in the code. While it's acceptable in some cases, consider using more specific types or interface definitions where possible to improve type safety.You could replace these with more specific types or use a generic parameter constrained to the expected structure.
Also applies to: 116-116
🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (14)
.changeset/brave-kiwis-explode.md(1 hunks)examples/example-vite-react-sdk/src/App.tsx(1 hunks)examples/example-vite-react-sdk/src/main.tsx(0 hunks)package.json(1 hunks)packages/sdk/src/experimental/index.ts(1 hunks)packages/sdk/src/index.ts(5 hunks)packages/sdk/src/parseEntities.ts(1 hunks)packages/sdk/src/react/hooks.ts(2 hunks)packages/sdk/src/state/zustand.ts(6 hunks)packages/sdk/src/types.ts(2 hunks)packages/torii-wasm/package.json(0 hunks)packages/utils/src/_test_/utils/index.test.ts(2 hunks)packages/utils/src/utils/index.ts(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (2)
- examples/example-vite-react-sdk/src/main.tsx
- packages/torii-wasm/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- examples/example-vite-react-sdk/src/App.tsx
- packages/sdk/src/parseEntities.ts
- packages/sdk/src/experimental/index.ts
- turbo.json
- packages/utils/src/test/utils/index.test.ts
- packages/sdk/src/types.ts
- package.json
- .changeset/brave-kiwis-explode.md
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/state/zustand.ts
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/react/hooks.ts
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (16)
packages/sdk/src/react/hooks.ts (1)
66-71: Great addition of the useModels hook with clear documentationThe JSDoc comments clearly explain the purpose of the hook and its parameters. This is a useful addition that complements the existing
useModelhook by allowing retrieval of all entities of a specific model type.packages/utils/src/utils/index.ts (2)
376-391: Well-implemented utility function for port determination.The
getDefaultPortForProtocolfunction provides a clean way to determine default ports for various protocols. The function handles the common secure protocols (https, wss) and provides a sensible default for other protocols.
393-454:❓ Verification inconclusive
Good implementation of URI to multiaddr conversion with comprehensive error handling.
The
convertToRelayUrifunction is well-documented with examples and handles various URI formats (IPv4, IPv6, domain names). The logic is clear and follows a logical flow with proper error handling.A few observations:
- The function properly handles special cases like IPv6 addresses
- Including error handling with try/catch is a good practice
- The commented examples provide clarity on expected behavior
🏁 Script executed:
#!/bin/bash # Test the convertToRelayUri function with various URI formats # Create a simple test file to verify the function cat > test.js << 'EOF' const { convertToRelayUri } = require('./packages/utils/src/utils/index'); // Test cases const testCases = [ { input: 'http://example.com', expected: '/dns4/example.com/tcp/80' }, { input: 'https://example.com', expected: '/dns4/example.com/tcp/443' }, { input: 'http://127.0.0.1:8080', expected: '/ip4/127.0.0.1/tcp/8080' }, { input: 'http://[::1]:8080', expected: '/ip6/::1/tcp/8080' }, { input: 'wss://example.com', expected: '/dns4/example.com/tcp/443/x-parity-wss' }, { input: 'invalid-uri', expected: '' } ]; testCases.forEach(({ input, expected }) => { const result = convertToRelayUri(input); console.log(`Input: ${input}`); console.log(`Result: ${result}`); console.log(`Expected: ${expected}`); console.log(`Matches: ${result.includes(expected)}`); console.log('---'); }); EOF # Run the test node test.jsLength of output: 926
Action Required: Verify Test Environment Setup for TypeScript Module Resolution
The implementation of the
convertToRelayUrifunction remains well-documented and robust with comprehensive URI handling and proper error management. However, note that the test script failed due to a module resolution error when attempting to require the TypeScript file directly. This issue typically arises because Node.js needs the file to be compiled (or run via a TypeScript-compatible runner such as ts-node).Please verify and adjust the test environment accordingly:
- Confirm that the file at
packages/utils/src/utils/index.tsis either compiled to JavaScript or run with a TypeScript runner (e.g., ts-node).- Update the test script or build process to ensure proper module resolution for TypeScript files.
packages/sdk/src/index.ts (4)
2-2: Improved type imports for better code organization.Converting regular imports to type imports for types that are only used for type checking (not at runtime) is a good practice. This helps with tree-shaking and clarifies the intent of these imports.
Also applies to: 4-17
37-40: Good addition of default client configuration.Adding default configuration values improves the developer experience by providing sensible defaults while still allowing customization. This change aligns with modern JavaScript practices of providing default values that can be overridden.
51-55: Improved client configuration with defaults.The implementation properly merges default configuration with user-provided options, using the spread operator. This ensures defaults are applied only when values aren't explicitly provided.
158-165: Enhanced error handling in callback.The error handling improvement ensures that callbacks always receive a proper Error instance, making error handling more consistent for consumers of this API.
packages/sdk/src/state/zustand.ts (9)
24-31: Well-documented type guard function for CairoOption.The
isCairoOptionfunction provides a clear type guard that follows TypeScript best practices. The function is properly documented with JSDoc comments.
33-66: Proper implementation of CairoOption merging with clear logic.The
mergeCairoOptionfunction handles the merging of Cairo options with appropriate priority rules (prefers source's value if it's "Some", keeps target otherwise). The function handles edge cases well.
68-75: Well-documented type guard function for CairoCustomEnum.The
isCairoCustomEnumtype guard follows the same pattern asisCairoOption, providing a consistent API for type checking. The function is well-documented with JSDoc comments.
77-133: Comprehensive implementation of CairoCustomEnum merging.The
mergeCairoCustomEnumfunction handles complex merging of Cairo custom enums by:
- Checking if both values are CairoCustomEnum instances
- Prioritizing the source's active variant if available
- Falling back to the target's active variant
- Creating a new enum with the appropriate active variant
The implementation is thorough and handles edge cases appropriately.
🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
147-152: Enhanced deepMerge function to support Cairo types.The modification to
deepMergeproperly integrates the new Cairo type support while maintaining compatibility with existing code. The implementation follows a clear pattern of checking types before applying specialized merging logic.
197-201: Improved loop structure for better readability.Changing from
forEachto afor...ofloop improves readability and makes the code structure more consistent with the other loops in the file.
205-217: Enhanced entity merging with better control flow.The loop structure has been improved by using
for...ofand replacing thereturnstatement withcontinue, which makes the control flow more intuitive and maintains consistency throughout the loop.
223-246: Improved deep merging of namespace models.The implementation now properly handles deep merging of models with nested structures and Cairo types. The loop has been restructured for better readability and the type assertions have been updated for consistency.
274-296: Consistent implementation of deep merging in updateEntity.The changes to the
updateEntityfunction mirror those in themergeEntitiesfunction, ensuring consistent behavior between the two. The use offor...ofloops and proper deep merging improves the reliability of model updates.
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
@dojoengine/torii-wasmpackage.