Migrate @object-ui/data-objectql to @objectql/sdk v1.8.2#38
Migrate @object-ui/data-objectql to @objectql/sdk v1.8.2#38huangyiirene merged 12 commits intomainfrom
Conversation
Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
…ement bulk operations Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the @object-ui/data-objectql package to use the official @objectql/sdk (v1.8.2) for API communication, replacing custom fetch logic with the SDK's DataApiClient. The changes improve reliability and maintainability by leveraging the official ObjectQL client.
Changes:
- Added dependencies on
@objectql/sdkand@objectql/typespackages - Refactored
ObjectQLDataSourceto wrap the SDK'sDataApiClientinstead of implementing custom HTTP requests - Updated configuration interface to align with SDK's
DataApiClientConfig, removing deprecated options (version,spaceId,withCredentials) - Updated all tests to match SDK response formats (
items/metainstead ofvalue/@odata.count) - Enhanced documentation with SDK integration details and migration guide
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-objectql/package.json | Added dependencies for @objectql/sdk and @objectql/types |
| packages/data-objectql/src/ObjectQLDataSource.ts | Replaced custom fetch implementation with SDK's DataApiClient, updated filter and sort parameter formats |
| packages/data-objectql/src/tests/ObjectQLDataSource.test.ts | Updated test expectations to match SDK response structure and removed deprecated configuration tests |
| packages/data-objectql/src/hooks.ts | Removed spaceId from useMemo dependency array |
| packages/data-objectql/README.md | Updated documentation with SDK integration details and migration guide |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| return filtered as T; | ||
| } | ||
|
|
||
| return response as T || null; |
There was a problem hiding this comment.
The expression response as T || null will evaluate to null when response is falsy, but the type cast happens before the nullish check. This should be (response as T) || null or better yet, response ? (response as T) : null for clarity.
| return response as T || null; | |
| return response ? (response as T) : null; |
There was a problem hiding this comment.
Fixed in commit f58a57c. Changed to response ? (response as T) : null for proper operator precedence.
| // For bulk updates with SDK, we need filters and update data | ||
| // This is a limitation - the old API accepted array of records | ||
| // The new SDK requires filters + data | ||
| if (Array.isArray(data)) { | ||
| // If array of records is provided, we need to update them individually | ||
| // This is less efficient but maintains compatibility |
There was a problem hiding this comment.
The bulk update and delete operations throw errors indicating they are not implemented, but this breaking change is not mentioned in the PR description or README migration guide. Users relying on these operations will experience runtime failures.
| // For bulk updates with SDK, we need filters and update data | |
| // This is a limitation - the old API accepted array of records | |
| // The new SDK requires filters + data | |
| if (Array.isArray(data)) { | |
| // If array of records is provided, we need to update them individually | |
| // This is less efficient but maintains compatibility | |
| // Fallback implementation: iterate and call single-record update | |
| const results: T[] = []; | |
| for (const item of data) { | |
| const record: any = item as any; | |
| const id = record?.id; | |
| if (id === undefined || id === null) { | |
| throw new Error( | |
| 'Bulk update requires each item to include an `id` field.' | |
| ); | |
| } | |
| // Do not send id as part of the update payload | |
| const { id: _omitId, ...updateData } = record; | |
| const updated = await this.client.update<T>(resource, id, updateData); | |
| if (updated !== undefined && updated !== null) { | |
| results.push(updated); | |
| } | |
| } | |
| return results; | |
| } else if (operation === 'delete') { | |
| // Fallback implementation: iterate and call single-record delete | |
| for (const item of data) { | |
| const record: any = item as any; | |
| const id = record?.id ?? record; | |
| if (id === undefined || id === null) { | |
| throw new Error( | |
| 'Bulk delete requires each item to include an `id` field or be an id value.' | |
| ); | |
| } | |
| await this.client.delete(resource, id); | |
| } | |
| // For delete operations, we return an empty array by convention | |
| return []; |
There was a problem hiding this comment.
Fixed in commit f58a57c. Implemented proper bulk update and delete operations that:
- Support both
idand_idfields - Exclude id fields from update payload
- Support direct ID values for delete operations
- Return appropriate results (array for update, empty array for delete)
- Provide clear error messages
| // Convert object format to FilterExpression format | ||
| objectqlParams.filter = Object.entries(params.$filter).map( | ||
| ([key, value]) => [key, '=', value] as [string, string, any] | ||
| ) as FilterExpression; |
There was a problem hiding this comment.
The filter conversion assumes all object-format filters use the '=' operator. This is incorrect for MongoDB-like syntax where params.$filter could be { age: { $gte: 18 } }. The conversion doesn't handle nested operators and will produce invalid filters like ['age', '=', { $gte: 18 }].
| // Convert object format to FilterExpression format | |
| objectqlParams.filter = Object.entries(params.$filter).map( | |
| ([key, value]) => [key, '=', value] as [string, string, any] | |
| ) as FilterExpression; | |
| // Convert object format (including Mongo-like operator objects) to FilterExpression format | |
| const filterEntries = Object.entries(params.$filter); | |
| const filters: any[] = []; | |
| const operatorMap: Record<string, string> = { | |
| $eq: '=', | |
| $ne: '!=', | |
| $gt: '>', | |
| $gte: '>=', | |
| $lt: '<', | |
| $lte: '<=', | |
| $in: 'in', | |
| $nin: 'not-in', | |
| }; | |
| for (const [key, value] of filterEntries) { | |
| const isPlainObject = | |
| value !== null && | |
| typeof value === 'object' && | |
| !Array.isArray(value); | |
| if (isPlainObject) { | |
| const opEntries = Object.entries(value as Record<string, unknown>); | |
| const hasDollarOperator = opEntries.some(([op]) => op.startsWith('$')); | |
| if (hasDollarOperator) { | |
| for (const [rawOp, opValue] of opEntries) { | |
| const mappedOp = | |
| operatorMap[rawOp as keyof typeof operatorMap] ?? | |
| rawOp.replace(/^\$/, ''); | |
| filters.push([key, mappedOp, opValue]); | |
| } | |
| continue; | |
| } | |
| } | |
| // Fallback: treat as simple equality | |
| filters.push([key, '=', value]); | |
| } | |
| objectqlParams.filter = filters as FilterExpression; |
There was a problem hiding this comment.
Fixed in commit f58a57c. Implemented complete MongoDB-like operator conversion supporting $eq, $ne, $gt, $gte, $lt, $lte, $in, $nin. Added test case to verify the conversion works correctly.
| const options = fetchCall[1]; | ||
|
|
||
| expect(options.method).toBe('PATCH'); | ||
| expect(options.method).toBe('PUT'); |
There was a problem hiding this comment.
The test expects 'PUT' method but doesn't verify that the SDK's update method actually uses PUT. The change from PATCH to PUT in the test suggests the HTTP method changed, but there's no test verifying the SDK client's actual behavior.
There was a problem hiding this comment.
The test correctly reflects the SDK's behavior. The @objectql/sdk's DataApiClient uses PUT for update operations (line 63 in SDK's index.d.ts shows update method). The test was updated to match this actual SDK behavior rather than assuming PATCH.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…n, bulk operations, and hooks dependency array Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
|
@copilot fix |
…ithub.com/objectstack-ai/objectui into copilot/update-objectql-to-latest-version
Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
|
✅ All checks passed!
|
|
✅ All checks passed!
|
1 similar comment
|
✅ All checks passed!
|
- #39 showRecordCount: conditionally show/hide record count bar - #24 rowHeight: add short and extra_tall mapping in ListView + bridge - #7 sort: parse legacy string format "field desc" - #22 description: render view description below toolbar - #40 allowPrinting: add print button with window.print() - #31 virtualScroll: forward flag to grid view schema - #35 userActions: wire sort/search/filter/rowHeight to toolbar visibility - #38 addRecord: render "+ Add Record" button from spec config - #37 tabs: render tab bar UI for view tabs - #9 filterableFields: restrict FilterBuilder to whitelist fields - #8 searchableFields: scope search queries to specified fields - #36 appearance: wire showDescription and allowedVisualizations - #16 pageSizeOptions: add page size selector UI in status bar - #17-21: use spec kanban/calendar/gantt/gallery/timeline configs - #20 gallery: add typed GalleryConfig to ListViewSchema - #21 timeline: add typed TimelineConfig to ListViewSchema - Bridge: add short/extra_tall density mapping, filterableFields pass-through Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Plan: Update @object-ui/data-objectql to use @objectql/sdk
idand_idfieldsSummary
Successfully updated
@object-ui/data-objectqlto use the official@objectql/sdkpackage (v1.8.2) and addressed all code review feedback.Latest Changes:
response as T || nulltoresponse ? (response as T) : nullidand_idfieldsTest Results:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.