-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Add DQL support #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Add useQuery hook for reactive queries with sync subscriptions - Add useExecuteQuery hook for one-time query execution - Update example app to use DQL instead of collection-based API - Rename old example to vite-typescript-example-legacy - Update CI to test both DQL and legacy examples
- disable sync with v3 - install websocket url - disable Ditto cloud sync - actually start sync
… of cursor-based hooks
export function useQuery< | ||
// We default to any here and let the user specify the type if they want. | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
T = any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer unknown
here, but I see it's technically optional so I guess it's ok. I don't think there's any harm in forcing the user to specify types, personally!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also a fan of stricter type safety from defaulting to unknown
nextDitto = dittoHash[persistenceDirectory ?? Object.keys(dittoHash)[0]] | ||
} | ||
|
||
if (nextDitto == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer strict equality here ===
but I think !nextDitto
would also work. Not a big issue, though.
let finalQueryArguments: U = {} as U | ||
if (queryArguments) { | ||
finalQueryArguments = Object.assign(finalQueryArguments, queryArguments) | ||
} | ||
|
||
if (localQueryArguments) { | ||
finalQueryArguments = Object.assign( | ||
finalQueryArguments, | ||
localQueryArguments, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be simplified like this:
const finalQueryArguments = {
...queryArguments,
...localQueryArguments,
} as U
Or if you need to handle null/undefined values explicitly:
const finalQueryArguments = {
...(queryArguments || {}),
...(localQueryArguments || {}),
} as U
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a couple very low priority questions/comments, but I think this looks good!
collection: 'tasks', | ||
}) | ||
// Query documents using DQL | ||
const { documents } = useQuery<Task>('SELECT * FROM tasks WHERE isCompleted = false') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth having a note about type coercion here? I know it is called out in the doc comments for the hook, so perhaps that is good enough
persistenceDirectory: uuidv4(), | ||
}) | ||
|
||
export const DocumentUpserter: React.FC<{ persistenceDirectory: string }> = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: React.FC
is generally discouraged, as I understand it. Since it is used for tests here, I don't think it matters much, though.
export function useQuery< | ||
// We default to any here and let the user specify the type if they want. | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
T = any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also a fan of stricter type safety from defaulting to unknown
Can we get Copilot on this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for DQL (Ditto Query Language) by introducing two new React hooks: useQuery()
for continuous query observation and useExecuteQuery()
for on-demand query execution. The PR replaces the existing Vite example with a DQL-based implementation and moves the query builder example to a legacy folder.
- Adds
useQuery()
hook for real-time document subscriptions with DQL syntax - Adds
useExecuteQuery()
hook for executing queries on demand (mutations and ad-hoc queries) - Updates minimum Ditto version requirement to 4.8.1+ and example to use 4.10.0+
Reviewed Changes
Copilot reviewed 32 out of 42 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
src/queries/useQuery.ts |
Implements the main useQuery hook with store observer and sync subscription setup |
src/queries/useExecuteQuery.ts |
Implements the useExecuteQuery hook for on-demand query execution |
src/queries/useQuery.spec.tsx |
Comprehensive test suite for the useQuery hook functionality |
src/queries/useExecuteQuery.spec.tsx |
Comprehensive test suite for the useExecuteQuery hook functionality |
examples/vite-typescript-example/src/App.tsx |
Updates example app to use DQL hooks instead of query builder API |
examples/vite-typescript-example/src/AppContainer.tsx |
Updates Ditto configuration for newer API and cloud setup |
examples/vite-typescript-example/src/AuthenticationPanel.tsx |
Updates authentication method call from loginWithToken to login |
package.json |
Updates peer dependency version requirements for Ditto |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
} | ||
} | ||
|
||
setQueryResult(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting queryResult
to null
conflicts with the TypeScript type QueryResult<T> | undefined
declared on line 134. The setQueryResult
function expects QueryResult<T> | undefined
but null
is being passed.
setQueryResult(null) | |
setQueryResult(undefined) |
Copilot uses AI. Check for mistakes.
* >Add Task</button> | ||
* ``` | ||
* | ||
* @param query - The query to run. Must be a non-mutating query. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states the query must be non-mutating, but useExecuteQuery
is specifically designed for mutating queries as shown in the examples and description. This comment appears to be copied from useQuery
and should be corrected.
* @param query - The query to run. Must be a non-mutating query. | |
* @param query - The query to run. May be a mutating query. |
Copilot uses AI. Check for mistakes.
localQueryArguments?: Partial<U>, | ||
localOnError?: (error: unknown) => void, | ||
) => { | ||
setItems(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting items
to null
conflicts with the TypeScript type QueryResultItem[] | undefined
declared on line 200. The setItems
function expects QueryResultItem[] | undefined
but null
is being passed.
setItems(null) | |
setItems(undefined) |
Copilot uses AI. Check for mistakes.
localOnError?: (error: unknown) => void, | ||
) => { | ||
setItems(null) | ||
setMutatedDocumentIDs(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting mutatedDocumentIDs
to null
conflicts with the TypeScript type DocumentID[] | undefined
declared on line 202. The setMutatedDocumentIDs
function expects DocumentID[] | undefined
but null
is being passed.
setMutatedDocumentIDs(null) | |
setMutatedDocumentIDs(undefined) |
Copilot uses AI. Check for mistakes.
const reset = useCallback(() => { | ||
setError(null) | ||
setItems(undefined) | ||
setMutatedDocumentIDs(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting mutatedDocumentIDs
to null
conflicts with the TypeScript type DocumentID[] | undefined
declared on line 202. The setMutatedDocumentIDs
function expects DocumentID[] | undefined
but null
is being passed.
setMutatedDocumentIDs(null) | |
setMutatedDocumentIDs(undefined) |
Copilot uses AI. Check for mistakes.
return <h1>Loading</h1> | ||
} | ||
if (error) { | ||
if (error) console.error('Error creating Ditto instances:', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition if (error)
is redundant since it's already inside an if (error)
block on line 98. This creates a duplicated check that serves no purpose.
if (error) console.error('Error creating Ditto instances:', error) | |
console.error('Error creating Ditto instances:', error) |
Copilot uses AI. Check for mistakes.
Looks like there are some good suggestions from Copilot, I will continue here after my PTO, didn't get this done today. |
Adds support for DQL by adding two new hooks:
useQuery()
anduseExecuteQuery()
. See original API design (internal).examples/vite-typescript-example-legacy
.The minimum supported version of
@dittolive/ditto
changes to 4.8.1 (4.10.0 in the example to supportcustomAuthURL
).Usage
A quick overview, refer to the API reference for more details.
The
useQuery()
hook immediately sets up a store observer and sync subscription for a given query. This hook only supports non-mutating DQL syntax.The
useExecuteQuery()
hook returns a function that can be called to execute a query on demand, e.g. for mutating data or reacting to user actions.Out of scope