chore: refac sdk react hooks #405
Conversation
WalkthroughA new changeset file is added to patch multiple dependencies for the Dojo Engine SDK, including updates for various libraries. Additionally, the subscription logic in the SDK's React hooks has been refactored. The functions useEntityQuery, useEventQuery, and useHistoricalEventsQuery now delegate to a new generic function, createSubscriptionHook, which centralizes configuration, subscription management, and error handling. This modular change aims to simplify and standardize the subscription lifecycle across the SDK. Changes
Sequence Diagram(s)sequenceDiagram
participant R as React Component
participant H as Hook (useEntityQuery / useEventQuery / useHistoricalEventsQuery)
participant C as createSubscriptionHook
participant D as Data Service
R->>H: Call query hook
H->>C: Delegate to createSubscriptionHook with config
C->>D: Initiate subscription via subscribeMethod
D-->>C: Return subscription updates
C-->>H: Provide processed data/error updates
H-->>R: Return final subscription data
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: 7
🧹 Nitpick comments (2)
packages/sdk/src/react/hooks.ts (2)
121-127: Caution with multiple mutable refs
subscriptionRef,fetchingRef, andisUpdatingstore state outside React's lifecycle. If asynchronous calls overlap, the code may encounter tricky race conditions. A React state oruseReducercan sometimes provide safer updates.
196-202: Consider unsubscribing in cleanup
The subscription cancellation is commented out. If subscriptions are no longer needed, uncomment or remove to reduce potential resource leaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/red-feet-reply.md(1 hunks)packages/sdk/src/react/hooks.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/red-feet-reply.md
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/react/hooks.ts
[error] 109-109: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 117-117: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 140-140: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 153-153: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 215-215: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 245-245: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 129-129: This hook does not specify all of its dependencies: config.getErrorPrefix
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.processInitialData
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.subscribeMethod
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.updateSubscriptionMethod
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.queryToHashedKeysMethod
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.historical
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.processUpdateData
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 173-173: This hook does not specify all of its dependencies: config.getErrorPrefix
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (12)
packages/sdk/src/react/hooks.ts (12)
10-16: No issues with these type imports.
No correctness or security concerns identified here.
21-22: Imports look good
No concerns with the added type imports fromtoriiQueryBuilderandtorii-client.
98-98: Clear documentation
The doc comment for the base hook factory is concise and helps clarify its purpose.
169-170: Initialization logic
Storing initial data viaprocessInitialDataand returning the subscription is straightforward and correct.
172-172: Incomplete dependency array
This line referencesqueryalone. Missing dependencies were previously noted.
207-208: Enhanced documentation
The revised doc comments more clearly describe usage and expected behavior.
212-212: Refactored to a subscription hook
Delegating to the generic subscription hook promotes code reuse and consistency.
218-232: Entity subscription registration
The subscription logic here is coherent and integrates well withsdk.subscribeEntityQuery.
234-234: Subscription hook invocation
InvokinguseEntityQueryHook(query)is straightforward and ensures the entity query is subscribed.
237-238: Improved event query documentation
The short docstring clarifies the hook intent and usage well.
280-295: Historical events subscription
Matches the same subscription pattern. Implementation is consistent and appears correct.
297-297: Hook invocation
useHistoricalEventsQueryHook(query)is called in a similar fashion to the other hooks, maintaining the pattern.
| const fetchData = useCallback(async () => { | ||
| // Wait until lock is released | ||
| while (isUpdating.current) { | ||
| await sleep(50); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check your dependencies and concurrency
The fetchData callback references config but includes only [query] in its useCallback dependency array. Also, the while (isUpdating.current) loop can cause blocking or race conditions. Consider using a different concurrency technique or a safe lock mechanism.
🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: This hook does not specify all of its dependencies: config.getErrorPrefix
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.processInitialData
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.subscribeMethod
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.updateSubscriptionMethod
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.queryToHashedKeysMethod
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.historical
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 129-129: This hook does not specify all of its dependencies: config.processUpdateData
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
| const [initialData, subscription] = await config.subscribeMethod({ | ||
| query: fetchingRef.current!, | ||
| callback: ({ data, error }) => { | ||
| if (data) { | ||
| config.processUpdateData(data); | ||
| } | ||
| } | ||
| if (error) { | ||
| console.error( | ||
| "Dojo.js - useEntityQuery - error subscribing to entity with query : ", | ||
| query.toString() | ||
| ); | ||
| console.error(error); | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| state.mergeEntities(initialData); | ||
|
|
||
| return subscription; | ||
| }, [query, subscriptionRef]); | ||
|
|
||
| useEffect(() => { | ||
| if (!deepEqual(query, fetchingRef.current)) { | ||
| fetchingRef.current = query; | ||
|
|
||
| fetchData() | ||
| .then((s) => { | ||
| if (s !== null) { | ||
| subscriptionRef.current = s; | ||
| if (error) { | ||
| console.error( | ||
| `${config.getErrorPrefix()} - error subscribing with query: `, | ||
| query.toString() | ||
| ); | ||
| console.error(error); | ||
| } | ||
| // Important to release lock at this point. | ||
| isUpdating.current = false; | ||
| }) | ||
| .catch((err) => { | ||
| console.error( | ||
| "Dojo.js - useEntityQuery - error fetching entities for query ", | ||
| JSON.stringify(query) | ||
| ); | ||
| console.error(err); | ||
| }) | ||
| .finally(() => { | ||
| // Important to release lock at this point. | ||
| isUpdating.current = false; | ||
| }); | ||
| } | ||
|
|
||
| return () => { | ||
| if (subscriptionRef.current) { | ||
| // subscriptionRef.current?.cancel(); | ||
| // subscriptionRef.current = null; | ||
| // setSub(null); | ||
| }, | ||
| historical: config.historical, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid non-null assertion in subscribeMethod
Similar issue at line 153: fetchingRef.current!. Prevent possible null reference errors by verifying the ref before usage. Also consider providing explicit param types for the callback arguments to avoid implicit any.
🧰 Tools
🪛 Biome (1.9.4)
[error] 153-153: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| useEffect(() => { | ||
| if (!deepEqual(query, fetchingRef.current)) { | ||
| fetchingRef.current = query; | ||
|
|
||
| fetchData() | ||
| .then((s) => { | ||
| if (s !== null) { | ||
| subscriptionRef.current = s; | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| console.error( | ||
| `${config.getErrorPrefix()} - error fetching data for query: `, | ||
| JSON.stringify(query) | ||
| ); | ||
| console.error(err); | ||
| }) | ||
| .finally(() => { | ||
| // Release lock | ||
| isUpdating.current = false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include all config properties in dependencies
useEffect references config.getErrorPrefix() and other config values but omits them from [query, fetchData]. Add those if they can change over time to prevent stale closures.
🧰 Tools
🪛 Biome (1.9.4)
[error] 173-173: This hook does not specify all of its dependencies: config.getErrorPrefix
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
| query: ToriiQueryBuilder<Schema> | ||
| ) { | ||
| const { sdk, useDojoStore } = useDojoSDK(); | ||
| const { sdk, useDojoStore } = useDojoSDK<() => any, Schema>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid () => any type
Providing a more specific function signature for your sdk can help catch errors at compile-time and improve maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 215-215: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| export function useEventQuery<Schema extends SchemaType>( | ||
| query: ToriiQueryBuilder<Schema> | ||
| ) { | ||
| const { sdk, useDojoStore } = useDojoSDK<() => any, Schema>(); | ||
| const state = useDojoStore((s) => s); | ||
|
|
||
| return () => { | ||
| if (subscriptionRef.current) { | ||
| // subscriptionRef.current?.cancel(); | ||
| // subscriptionRef.current = null; | ||
| // setSub(null); | ||
| const useEventQueryHook = createSubscriptionHook<Schema>({ | ||
| subscribeMethod: (options) => sdk.subscribeEventQuery(options), | ||
| updateSubscriptionMethod: (subscription, clause) => | ||
| sdk.updateEventMessageSubscription(subscription, clause, false), | ||
| queryToHashedKeysMethod: (query) => | ||
| sdk.toriiEventMessagesQueryIntoHashedKeys(query, false), | ||
| processInitialData: (data) => state.mergeEntities(data), | ||
| processUpdateData: (data) => { | ||
| const event = data.pop(); | ||
| if (event && event.entityId !== "0x0") { | ||
| state.updateEntity(event); | ||
| } | ||
| }; | ||
| }, [query, subscriptionRef, fetchData]); | ||
| }, | ||
| getErrorPrefix: () => "Dojo.js - useEventQuery", | ||
| historical: false, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactored event subscription
The approach matches useEntityQuery, though line 245 again uses () => any for sdk. Use a narrower type to improve safety.
🧰 Tools
🪛 Biome (1.9.4)
[error] 245-245: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| if (subscriptionRef.current) { | ||
| const [results, clause] = await config.queryToHashedKeysMethod( | ||
| fetchingRef.current!, | ||
| config.historical | ||
| ); | ||
| await config.updateSubscriptionMethod( | ||
| subscriptionRef.current, | ||
| clause, | ||
| config.historical | ||
| ); | ||
| config.processInitialData(results); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove non-null assertion in query invocation
Using fetchingRef.current! at line 140 can lead to runtime errors if current is null. Replace with a safe check or fallback.
- const [results, clause] = await config.queryToHashedKeysMethod(
- fetchingRef.current!,
- config.historical
- );
+ const currentQuery = fetchingRef.current;
+ if (!currentQuery) return;
+ const [results, clause] = await config.queryToHashedKeysMethod(
+ currentQuery,
+ config.historical
+ );📝 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.
| if (subscriptionRef.current) { | |
| const [results, clause] = await config.queryToHashedKeysMethod( | |
| fetchingRef.current!, | |
| config.historical | |
| ); | |
| await config.updateSubscriptionMethod( | |
| subscriptionRef.current, | |
| clause, | |
| config.historical | |
| ); | |
| config.processInitialData(results); | |
| if (subscriptionRef.current) { | |
| const currentQuery = fetchingRef.current; | |
| if (!currentQuery) return; | |
| const [results, clause] = await config.queryToHashedKeysMethod( | |
| currentQuery, | |
| config.historical | |
| ); | |
| await config.updateSubscriptionMethod( | |
| subscriptionRef.current, | |
| clause, | |
| config.historical | |
| ); | |
| config.processInitialData(results); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| function createSubscriptionHook< | ||
| Schema extends SchemaType, | ||
| Historical extends boolean = false, | ||
| >(config: { | ||
| subscribeMethod: ( | ||
| options: SubscribeParams<Schema, Historical> | ||
| ) => Promise<SubscribeResponse<Schema, Historical>>; | ||
| updateSubscriptionMethod: ( | ||
| subscription: Subscription, | ||
| clause: any, | ||
| historical?: boolean | ||
| ) => Promise<void>; | ||
| queryToHashedKeysMethod: ( | ||
| query: ToriiQueryBuilder<Schema>, | ||
| historical?: boolean | ||
| ) => Promise<[ToriiResponse<Schema, Historical>, EntityKeysClause[]]>; | ||
| processInitialData: (data: ToriiResponse<Schema, Historical>) => void; | ||
| processUpdateData: (data: any) => void; | ||
| getErrorPrefix: () => string; | ||
| historical: Historical; | ||
| }) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any in function signatures
Lines 109 and 117 declare parameters as any, which reduces type safety. Consider replacing any with either a more specific type or unknown to maintain better compile-time checks.
Example fix:
- updateSubscriptionMethod: (
- subscription: Subscription,
- clause: any,
- historical?: boolean
- ) => Promise<void>;
+ updateSubscriptionMethod: (
+ subscription: Subscription,
+ clause: EntityKeysClause | Record<string, unknown>,
+ historical?: boolean
+ ) => Promise<void>;
- processUpdateData: (data: any) => void;
+ processUpdateData: (data: ToriiResponse<Schema, Historical>) => void;📝 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.
| function createSubscriptionHook< | |
| Schema extends SchemaType, | |
| Historical extends boolean = false, | |
| >(config: { | |
| subscribeMethod: ( | |
| options: SubscribeParams<Schema, Historical> | |
| ) => Promise<SubscribeResponse<Schema, Historical>>; | |
| updateSubscriptionMethod: ( | |
| subscription: Subscription, | |
| clause: any, | |
| historical?: boolean | |
| ) => Promise<void>; | |
| queryToHashedKeysMethod: ( | |
| query: ToriiQueryBuilder<Schema>, | |
| historical?: boolean | |
| ) => Promise<[ToriiResponse<Schema, Historical>, EntityKeysClause[]]>; | |
| processInitialData: (data: ToriiResponse<Schema, Historical>) => void; | |
| processUpdateData: (data: any) => void; | |
| getErrorPrefix: () => string; | |
| historical: Historical; | |
| }) { | |
| function createSubscriptionHook< | |
| Schema extends SchemaType, | |
| Historical extends boolean = false, | |
| >(config: { | |
| subscribeMethod: ( | |
| options: SubscribeParams<Schema, Historical> | |
| ) => Promise<SubscribeResponse<Schema, Historical>>; | |
| updateSubscriptionMethod: ( | |
| subscription: Subscription, | |
| clause: EntityKeysClause | Record<string, unknown>, | |
| historical?: boolean | |
| ) => Promise<void>; | |
| queryToHashedKeysMethod: ( | |
| query: ToriiQueryBuilder<Schema>, | |
| historical?: boolean | |
| ) => Promise<[ToriiResponse<Schema, Historical>, EntityKeysClause[]]>; | |
| processInitialData: (data: ToriiResponse<Schema, Historical>) => void; | |
| processUpdateData: (data: ToriiResponse<Schema, Historical>) => void; | |
| getErrorPrefix: () => string; | |
| historical: Historical; | |
| }) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 109-109: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 117-117: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
Closes #
Introduced changes
Checklist
Summary by CodeRabbit