-
Notifications
You must be signed in to change notification settings - Fork 50
Incremental Watched Queries #614
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: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 43f24fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Happy with the changeset, while massive. Well documented and structured.
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.
It's a big PR, so just giving some high-level comments to start with:
Internals & Hooks
I really like the refactoring to move more of the logic into the internals - it's nice that the hooks are now fairly thin wrappers around the internal methods.
incrementalWatch
The db.incrementalWatch
API feels a little difficult to follow with the current structure. It feels like the API changes completely based on which mode you select, and should perhaps rather be two separate APIs?
Another option is to invert the API to start with the query part first, for example something like this:
db.createQuery(sql, params).differentialWatch(...)
This can avoid the explosion of new methods on the database itself, instead having the different methods on a new Query-type class. You could perhaps also have different methods for creating this from different sources, e.g. to create from a Kysely or Drizzle query.
Incremental comparison queries
I see the main benefit of these to avoid re-renders, to improve rendering performance. It would be good to see some benchmarks to validate that this is worth the additional complexity, that we're not just moving overhead from the rendering part to the query part.
I also wonder if we should unify comparitors and differentiators, always using the differentiator API (identify + compareBy instead of just compare). This way, you can do the re-rendering on an even more granular level. So if you have 1 row out of 1000 results that changed, you can keep the other 999 row objects from the previous results, avoiding further re-renders on those rows. Once again, we'd need some form of benchmarks to check that this is actually worth it.
* A hook to access and subscribe to the results of an existing {@link WatchedQuery} instance. | ||
* @example | ||
* export const ContentComponent = () => { | ||
* const { data: lists } = useWatchedQuerySuspenseSubscription(listsQuery); |
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 name is wrong in this example.
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.
👍 Fixed
if (signal.aborted) { | ||
return; // Abort if the signal is already aborted | ||
} | ||
console.log('done with query refresh'); |
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.
Should remove these log statements (including the one above)
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.
🙏 Removed this
…ces better in differential queries.
…fferentiator implementation.
Update: I've updated the implementation to guard the new watch methods behind the new Comparison based queries are now accessed via The The An automated React profiling benchmark suite has been added in |
@@ -55,7 +55,8 @@ Deno.serve(async (req) => { | |||
// insert the new merged update as new single update for the document | |||
const supabaseInsert = await supabase.from('document_updates').insert({ | |||
document_id: document_id, | |||
update_data: Uint8ArrayToHex(docState) | |||
update_data: Uint8ArrayToHex(docState), | |||
editor_id: 'merged_update' |
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.
editor_id is a uuid in the sql, so this will fail.
But storing the data as hex also looks weird - not sure if this function works at all?
if (origin === this) { | ||
// update originated from the database / PowerSync - ignore | ||
return; | ||
} |
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.
This shouldn't be removed - it is used to avoid writing synced updates back to the db again.
updateQuery.registerListener({ | ||
onStateChange: async () => { | ||
for (const added of updateQuery.state.diff.added) { | ||
Y.applyUpdateV2(doc, b64ToUint8Array(added.update_b64)); |
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.
Need this
as the origin (see _storeUpdate below). Could alternatively potentially use a different value, e.g. a Symbol indicating the source is the db.
// This ID is updated on every new instance of the provider. | ||
private id = uuidv4(); |
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 feel like the previous approach of using the Set was simpler, since it didn't require an additional field in the database to filter the updates. We could still use the Set to only filter out the local updates, or alternatively apply those updates again (should be a no-op to apply the same update again).
break; | ||
updateQuery.registerListener({ | ||
onStateChange: async () => { | ||
for (const added of updateQuery.state.diff.added) { |
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.
When I tested this with only one tab open, I often get into a state where the initial "diff" is sent with every update, e.g. "all: 20 rows, added: 20 rows". Haven't debugged further to figure out what is going wrong.
data: | ||
- SELECT id, document_id, base64(update_data) as update_b64 FROM document_updates | ||
- SELECT * FROM documents WHERE id = bucket.document_id | ||
- SELECT * FROM documents WHERE id = bucket.document_id |
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.
This is duplicated.
|
||
export interface DifferentialWatchedQuery<RowType> | ||
extends WatchedQuery<ReadonlyArray<Readonly<RowType>>, DifferentialWatchedQuerySettings<RowType>> { | ||
readonly state: DifferentialWatchedQueryState<RowType>; |
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 haven't confirmed with testing, but it feels like accessing the differential state on the query could easily lead to race conditions where updates may be missed. Specifically, there could potentially be a case where the state is processed twice before it is processed in the app, leading to the diff from the first update to be missing.
With the normal async listeners this may not be an issue if the query waits for all listeners to return before processing the next one, but React hooks for example may be called at any later time.
So I'm wondering if we could send the diff in the callback, rather than the state on the query?
React hooks may have bigger issues though, since we have no control over when/how many times they are called. I'm not sure if it is actually feasible to expose the diff over a react hook's state at all.
Overview
Our current Watched query implementations emit results whenever a change to a dependant SQLite table occurs. The table changes might not affect the query result set, but we still query and emit a new result set for each table change. The result sets typically contain the same data, but these results are new Array/object references which will cause re-renders in certain frameworks like React.
This PR overhauls, improves and extends upon the existing watched query implementations by introducing incremental watched queries.
Incrementally Watched Queries can be constructed with varying behaviour. This PR introduces the concept of comparison and differential watched queries.
Comparison based queries behave similar to standard watched queries. These queries still query the SQLite DB under the hood on each dependant table change, but they compare the result set and only incrementally yield results if a change has been made. The latest query result is yielded as the result set.
Differential queries watch a SQL query and report detailed information on the changes between result sets. This gives additional information such as the
added
,removed
,updated
rows between result set changes.Implementation
The logic required for incrementally watched queries requires additional computation and introduces additional complexity to the implementation. For these reasons a new concept of a
WatchedQuery
class is introduced, along with a newquery
method allows building a instances ofWatchedQuery
s viawatch
anddifferentialWatch
methods.The
listsQuery
is smart, it:updateSchema
WatchedQuery
instances retain the latest state in memory. SharingWatchedQuery
instances can be used to introduce caching and reduce the number of duplicate DB queries between components.The incremental logic is customisable. Diff based queries can specify custom logic for performing diffs on the relevant data set. By default a
JSON.stringify
approach is used. Different data sets might have more optimal implementations.Updates to query parameters can be performed in a single place, affecting all subscribers.
Reactivity
The existing
watch
method and Reactivity packages have been updated to use incremental queries with differentiation defined as an opt-in feature (defaults to no changes).New hooks have also been added to use shared
WatchedQuery
instances.The Vue and React hooks packages have been updated to remove duplicate implementations of common watched query logic. Historically these packages relied on manually implementing watched queries based off the
onChange
API in order to cater for exposing additional state and custom query executors. The newWatchedQuery
APIs now support all the hook packages' requirements - this effectively reduces the heavy lifting in reactivity packages.The
React Supabase Todolist
demo has been updated with some best practices for reducing re-renders using comparison based incrementally watched queries.Differential Queries
These watched queries report the changes between result sets. The
data
member of aWatchedQuery
'sstate
is of the formA common use case for this is processing newly created items as they are added. The
YJS React Supabase Text Collab
demo has been updated to take advantage of this feature. Document updates are watched via a differential incremental query. New updates are passed to YJS for consolidation as they are synced - the application code no longer needs to track which items were already passed to YJS.