Add type-safe query builder and React hooks package#415
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds type-safe query building capabilities and a comprehensive React hooks package for the ObjectStack client SDK. The implementation introduces @objectstack/client-react with data and metadata hooks, plus a query builder for type-safe query construction.
Changes:
- Type-safe query builder with fluent API for filter construction
- New
@objectstack/client-reactpackage with data hooks (useQuery, useMutation, usePagination, useInfiniteQuery) and metadata hooks (useObject, useView, useFields, useMetadata) - Example components demonstrating hooks usage with MSW-based mocking
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/client/src/query-builder.ts | Type-safe query builder with FilterBuilder and QueryBuilder classes |
| packages/client/src/index.ts | Re-exports query builder utilities |
| packages/client-react/package.json | Package configuration for new React hooks library |
| packages/client-react/tsconfig.json | TypeScript configuration with JSX support |
| packages/client-react/src/context.tsx | React context provider for ObjectStackClient |
| packages/client-react/src/data-hooks.tsx | Data query and mutation hooks implementation |
| packages/client-react/src/metadata-hooks.tsx | Metadata fetching hooks implementation |
| packages/client-react/src/index.tsx | Main exports for the React hooks package |
| packages/client-react/README.md | Comprehensive documentation with usage examples |
| packages/client-react/CHANGELOG.md | Initial release changelog |
| examples/msw-react-crud/src/main.tsx | Toggle between traditional and hooks approaches |
| examples/msw-react-crud/src/components/TaskListWithHooks.tsx | Hooks-based task list component |
| examples/msw-react-crud/src/components/TaskFormWithHooks.tsx | Hooks-based task form component |
| examples/msw-react-crud/src/AppWithHooks.tsx | Demo app using React hooks |
| examples/msw-react-crud/package.json | Added client-react dependency |
| examples/msw-react-crud/README.md | Documentation for both approaches |
| pnpm-lock.yaml | Updated lockfile with new dependencies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| const fetchMetadata = useCallback(async () => { | ||
| if (!enabled) return; | ||
|
|
||
| try { | ||
| setIsLoading(true); | ||
| setError(null); | ||
| setFromCache(false); | ||
|
|
||
| if (useCache) { | ||
| // Use cached metadata endpoint | ||
| const result = await client.meta.getCached(objectName, { | ||
| ifNoneMatch: ifNoneMatch || etag, | ||
| ifModifiedSince | ||
| }); | ||
|
|
||
| if (result.notModified) { | ||
| setFromCache(true); | ||
| } else { | ||
| setData(result.data); | ||
| if (result.etag) { | ||
| setEtag(result.etag.value); | ||
| } | ||
| } | ||
|
|
||
| onSuccess?.(result.data || data); | ||
| } else { | ||
| // Direct fetch without cache | ||
| const result = await client.meta.getObject(objectName); | ||
| setData(result); | ||
| onSuccess?.(result); | ||
| } | ||
| } catch (err) { | ||
| const error = err instanceof Error ? err : new Error('Failed to fetch object metadata'); | ||
| setError(error); | ||
| onError?.(error); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }, [client, objectName, enabled, useCache, ifNoneMatch, ifModifiedSince, etag, data, onSuccess, onError]); | ||
|
|
||
| useEffect(() => { | ||
| fetchMetadata(); | ||
| }, [fetchMetadata]); |
There was a problem hiding this comment.
The fetchMetadata callback includes data in its dependency array on line 124, which creates a circular dependency. When data changes, fetchMetadata is recreated, which triggers the useEffect on line 126-128, which calls fetchMetadata, which may update data, causing an infinite loop.
Additionally, including onSuccess and onError callbacks in the dependency array will cause the hook to refetch whenever these callbacks change, which happens on every render if they're not memoized by the consumer. This is a common React hooks anti-pattern.
Consider:
- Remove
dataandetagfrom the dependency array (they're used internally but shouldn't trigger refetches) - Either remove
onSuccessandonErrorfrom dependencies or document that they must be memoized - Use a ref to store the latest callbacks if you need to call them without including them in dependencies
| const fetchData = useCallback(async (isRefetch = false) => { | ||
| if (!enabled) return; | ||
|
|
||
| try { | ||
| if (isRefetch) { | ||
| setIsRefetching(true); | ||
| } else { | ||
| setIsLoading(true); | ||
| } | ||
| setError(null); | ||
|
|
||
| let result: PaginatedResult<T>; | ||
|
|
||
| if (query) { | ||
| // Use advanced query API | ||
| result = await client.data.query<T>(object, query); | ||
| } else { | ||
| // Use simplified find API | ||
| result = await client.data.find<T>(object, { | ||
| select, | ||
| filters: filters as any, | ||
| sort, | ||
| top, | ||
| skip | ||
| }); | ||
| } | ||
|
|
||
| setData(result); | ||
| onSuccess?.(result); | ||
| } catch (err) { | ||
| const error = err instanceof Error ? err : new Error('Query failed'); | ||
| setError(error); | ||
| onError?.(error); | ||
| } finally { | ||
| setIsLoading(false); | ||
| setIsRefetching(false); | ||
| } | ||
| }, [client, object, query, select, filters, sort, top, skip, enabled, onSuccess, onError]); | ||
|
|
||
| // Initial fetch and dependency-based refetch | ||
| useEffect(() => { | ||
| fetchData(); | ||
| }, [fetchData]); |
There was a problem hiding this comment.
The fetchData callback includes onSuccess and onError in its dependency array (line 140), which will cause the query to refetch whenever these callbacks change. If the consumer passes inline arrow functions, this will happen on every render, causing infinite loops or unnecessary API calls.
This is the same circular dependency issue as in the metadata hooks. The typical React Query pattern uses refs to store the latest callbacks without including them in dependencies.
Consider using useRef to store callback references or document that callbacks must be memoized with useCallback.
| const fetchedCount = result.value.length; | ||
| const hasMore = fetchedCount === pageSize; |
There was a problem hiding this comment.
The hasNextPage logic is overly simplistic and may produce incorrect results. It only checks if fetchedCount === pageSize, which will incorrectly show "has next page" when the total count happens to be an exact multiple of pageSize.
For example, if there are exactly 100 records with pageSize=20, after fetching all 5 pages (0-19, 20-39, ... 80-99), the last page will have 20 items, so hasNextPage will be true, but there's actually no next page.
Consider using the count field from PaginatedResult to accurately determine if there are more pages:
const totalFetched = pages.reduce((sum, p) => sum + p.value.length, 0) + result.value.length;
const hasMore = result.count > totalFetched;
| const fetchedCount = result.value.length; | |
| const hasMore = fetchedCount === pageSize; | |
| const totalFetched = skip + result.value.length; | |
| const hasMore = | |
| typeof (result as any).count === 'number' | |
| ? (result as any).count > totalFetched | |
| : result.value.length === pageSize; |
| const { mutate: deleteTask } = useMutation('todo_task', 'delete', { | ||
| onSuccess: () => { | ||
| refetch(); | ||
| } | ||
| }); | ||
|
|
||
| const { mutate: updateTask } = useMutation('todo_task', 'update', { | ||
| onSuccess: () => { | ||
| refetch(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The inline onSuccess callbacks passed to useMutation will cause infinite loops due to the dependency array issue in the hooks. Every render creates new function references, which will recreate the mutation callbacks.
This should either be:
- Wrapped in useCallback:
const handleSuccess = useCallback(() => { refetch(); }, [refetch]); - Or the hooks should be fixed to not include callbacks in their dependency arrays
This demonstrates a fundamental issue with the hooks API design that needs to be addressed.
| * Provides ObjectStackClient instance to React components via Context API | ||
| */ | ||
|
|
||
| import * as React from 'react'; |
There was a problem hiding this comment.
Redundant React import. Line 7 imports * as React but it's not used anywhere in the file - all React APIs are imported as named imports on line 8. With modern React (17+) and the jsx transform configured in tsconfig.json, the namespace import is unnecessary.
Remove line 7 to simplify the imports.
| import * as React from 'react'; |
| export class FilterBuilder<T = any> { | ||
| private conditions: FilterCondition[] = []; | ||
|
|
||
| /** | ||
| * Equality filter: field = value | ||
| */ | ||
| equals<K extends keyof T>(field: K, value: T[K]): this { | ||
| this.conditions.push([field as string, '=', value]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Not equals filter: field != value | ||
| */ | ||
| notEquals<K extends keyof T>(field: K, value: T[K]): this { | ||
| this.conditions.push([field as string, '!=', value]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Greater than filter: field > value | ||
| */ | ||
| greaterThan<K extends keyof T>(field: K, value: T[K]): this { | ||
| this.conditions.push([field as string, '>', value]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Greater than or equal filter: field >= value | ||
| */ | ||
| greaterThanOrEqual<K extends keyof T>(field: K, value: T[K]): this { | ||
| this.conditions.push([field as string, '>=', value]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Less than filter: field < value | ||
| */ | ||
| lessThan<K extends keyof T>(field: K, value: T[K]): this { | ||
| this.conditions.push([field as string, '<', value]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Less than or equal filter: field <= value | ||
| */ | ||
| lessThanOrEqual<K extends keyof T>(field: K, value: T[K]): this { | ||
| this.conditions.push([field as string, '<=', value]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * IN filter: field IN (value1, value2, ...) | ||
| */ | ||
| in<K extends keyof T>(field: K, values: T[K][]): this { | ||
| this.conditions.push([field as string, 'in', values]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * NOT IN filter: field NOT IN (value1, value2, ...) | ||
| */ | ||
| notIn<K extends keyof T>(field: K, values: T[K][]): this { | ||
| this.conditions.push([field as string, 'not_in', values]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * LIKE filter: field LIKE pattern | ||
| */ | ||
| like<K extends keyof T>(field: K, pattern: string): this { | ||
| this.conditions.push([field as string, 'like', pattern]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * IS NULL filter: field IS NULL | ||
| */ | ||
| isNull<K extends keyof T>(field: K): this { | ||
| this.conditions.push([field as string, 'is_null', null]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * IS NOT NULL filter: field IS NOT NULL | ||
| */ | ||
| isNotNull<K extends keyof T>(field: K): this { | ||
| this.conditions.push([field as string, 'is_not_null', null]); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Build the filter condition | ||
| */ | ||
| build(): FilterCondition { | ||
| if (this.conditions.length === 0) { | ||
| throw new Error('Filter builder has no conditions'); | ||
| } | ||
| if (this.conditions.length === 1) { | ||
| return this.conditions[0]; | ||
| } | ||
| // Combine multiple conditions with AND | ||
| return ['and', ...this.conditions]; | ||
| } | ||
|
|
||
| /** | ||
| * Get raw conditions array | ||
| */ | ||
| getConditions(): FilterCondition[] { | ||
| return this.conditions; | ||
| } | ||
| } |
There was a problem hiding this comment.
The FilterBuilder is generating filter conditions in an incompatible format. It creates tuple-based conditions like ['field', '=', 'value'] and ['and', ...conditions], but the FilterCondition type from @objectstack/spec uses an object-based format with $and, $or, $eq, $gt, etc. operators.
For example, the current implementation creates:
['and', ['priority', '>', 2], ['is_completed', '=', false]]
But the correct format based on FilterConditionSchema should be:
{
$and: [
{ priority: { $gt: 2 } },
{ is_completed: { $eq: false } }
]
}
This incompatibility means queries built with this builder will not work with the actual ObjectStack backend that expects FilterCondition format. All filter-building methods (equals, greaterThan, in, like, etc.) need to be rewritten to generate the correct object-based format.
Implements P0 client SDK features: type-safe query construction and declarative React hooks for data fetching/mutations.
Changes
Type-Safe Query Builder (
@objectstack/client)equals(),greaterThan(),in(),like(), etc.select(),where(),orderBy(),limit(),paginate()QueryASTschema - no runtime changesReact Hooks Package (
@objectstack/client-react)New package with:
useQuery,useMutation,usePagination,useInfiniteQueryuseObject,useView,useFields,useMetadataObjectStackProvider,useClientAuto-handles loading/error states, refetching, caching. ETag support for metadata.
Examples
Updated
msw-react-crudwith hooks-based components demonstrating pagination and mutations.Usage
Package Details
@objectstack/client-react@0.6.1- peer dep:react >=18.0.0Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.