Add more test checks. Use update promise to populate isFetching

Author: stevensJourney

packages/common/src/client/watched/processors/AbstractQueryProcessor.ts

@@ -81,7 +81,7 @@ export abstract class AbstractQueryProcessor<
 
   protected async updateSettingsInternal(settings: Settings, signal: AbortSignal) {
     // This may have been aborted while awaiting or if multiple calls to `updateSettings` were made
-    if (signal.aborted) {
+    if (this._closed || signal.aborted) {
       return;
     }
 
@@ -124,6 +124,10 @@ export abstract class AbstractQueryProcessor<
   protected abstract linkQuery(options: LinkQueryOptions<Data>): Promise<void>;
 
   protected async updateState(update: Partial<MutableWatchedQueryState<Data>>) {
+    if (this._closed) {
+      return;
+    }
+
     if (typeof update.error !== 'undefined') {
       await this.iterateAsyncListenersWithError(async (l) => l.onError?.(update.error!));
       // An error always stops for the current fetching state
@@ -173,11 +177,11 @@ export abstract class AbstractQueryProcessor<
   }
 
   async close() {
+    this._closed = true;
     await this.initialized;
     this.abortController.abort();
     this.disposeListeners?.();
     this.disposeListeners = null;
-    this._closed = true;
     this.iterateListeners((l) => l.closed?.());
     this.listeners.clear();
   }

packages/react/src/hooks/watched/useWatchedQuery.ts

@@ -35,6 +35,7 @@ export const useWatchedQuery = <RowType = unknown>(
   }
 
   const [watchedQuery, setWatchedQuery] = React.useState(createWatchedQuery);
+  const updatingPromise = React.useRef<Promise<void> | null>();
 
   React.useEffect(() => {
     watchedQuery?.close();
@@ -57,18 +58,29 @@ export const useWatchedQuery = <RowType = unknown>(
    * We only need to override the isFetching status on the initial render where the query changed
    * (if we report the fetching status). The fetching status will be updated internally in the `updateSettings`
    * method eventually.
-   * Only overriding when the `queryChanged` value is true also prevents races if the query changes
-   * rapidly.
    */
   if (queryChanged) {
-    watchedQuery?.updateSettings({
+    // Keep track of this pending operation
+    let pendingUpdate = watchedQuery?.updateSettings({
       query,
       throttleMs: hookOptions.throttleMs,
       reportFetching: hookOptions.reportFetching
     });
+
+    // Keep track of the latest pending update operation
+    updatingPromise.current = pendingUpdate;
+
+    // Clear the latest pending update operation when the latest
+    // operation has completed.
+    pendingUpdate.then(() => {
+      // Only clear if this iteration was the latest iteration
+      if (pendingUpdate == updatingPromise.current) {
+        updatingPromise.current = null;
+      }
+    });
   }
 
-  const shouldReportCurrentlyFetching = (hookOptions.reportFetching ?? true) && queryChanged;
+  const shouldReportCurrentlyFetching = (hookOptions.reportFetching ?? true) && !!updatingPromise.current;
 
   const result = useNullableWatchedQuerySubscription(watchedQuery);
   return {

packages/react/tests/useQuery.test.tsx

@@ -255,10 +255,12 @@ describe('useQuery', () => {
           })
         );
 
-        // We should only receive the first list due to the WHERE clause
+        // Wait for the first result to be emitted
         await vi.waitFor(
           () => {
             expect(result.current.data[0]?.name).toEqual('first');
+            expect(result.current.isFetching).false;
+            expect(result.current.isLoading).false;
           },
           { timeout: 500, interval: 100 }
         );
@@ -268,6 +270,7 @@ describe('useQuery', () => {
         expect(
           hookEvents.find((event) => event.parameters[0] == '' && event.hookResults.isLoading == false)
         ).toBeUndefined();
+        // We should have an event where this was both loading and fetching
         expect(
           hookEvents.find(
             (event) =>
@@ -276,6 +279,15 @@ describe('useQuery', () => {
               event.hookResults.isFetching == true
           )
         ).toBeDefined();
+        // We should not have any event where isLoading or isFetching is false without data being presented
+        expect(
+          hookEvents.find(
+            (event) =>
+              event.parameters[0] == 'first' &&
+              (event.hookResults.isLoading || event.hookResults.isFetching) == false &&
+              event.hookResults.data[0]?.name != 'first'
+          )
+        ).toBeUndefined();
 
         // Verify that the fetching status was correlated to the parameters
         const firstResultEvent = hookEvents.find((event) => event.hookResults.data.length == 1);
@@ -287,14 +299,13 @@ describe('useQuery', () => {
         queryObserver.iterateListeners((l) =>
           l.queryUpdated?.({
             sql: 'select this is a broken query',
-            params: ['first']
+            params: ['error']
           })
         );
 
         // wait for the error to have been found
         await vi.waitFor(
           () => {
-            console.log(result.current);
             expect(result.current.error).not.equal(null);
             expect(result.current.isFetching).false;
           },
@@ -305,6 +316,15 @@ describe('useQuery', () => {
         expect(
           hookEvents.find((event) => event.hookResults.error != null && event.hookResults.isFetching == true)
         ).toBeUndefined();
+        // there should not be any results where the fetching status is false, but the error is not presented
+        expect(
+          hookEvents.find(
+            (event) =>
+              event.parameters[0] == 'error' &&
+              event.hookResults.error == null &&
+              (event.hookResults.isFetching || event.hookResults.isLoading) == false
+          )
+        ).toBeUndefined();
 
         queryObserver.iterateListeners((l) =>
           l.queryUpdated?.({
@@ -318,6 +338,8 @@ describe('useQuery', () => {
           () => {
             expect(result.current.data[0]?.name).toEqual('second');
             expect(result.current.error).null;
+            expect(result.current.isFetching).false;
+            expect(result.current.isLoading).false;
           },
           { timeout: 500, interval: 100 }
         );
@@ -326,6 +348,15 @@ describe('useQuery', () => {
         expect(secondFetchingEvent).toBeDefined();
         // We should immediately report that we are fetching once we detect new params
         expect(secondFetchingEvent?.hookResults.isFetching).true;
+        // We should never have reported that fetching was false before the results were present
+        expect(
+          hookEvents.find(
+            (event) =>
+              event.parameters[0] == 'second' &&
+              event.hookResults.data[0]?.name != 'second' &&
+              (event.hookResults.isFetching == false || event.hookResults.isLoading == false)
+          )
+        );
       });
 
       it('should execute compatible queries', async () => {

packages/react/vitest.config.ts

@@ -32,7 +32,7 @@ const config: UserConfigExport = {
        */
       isolate: true,
       provider: 'playwright',
-      headless: true,
+      headless: false,
       instances: [
         {
           browser: 'chromium'