fix: loading state for undefined refs/queries (#16)

andipaetzold · Oct 22, 2021 · 385a5de · 385a5de
1 parent 1af8357
commit 385a5de
Show file tree

Hide file tree

Showing 13 changed files with 132 additions and 63 deletions.
diff --git a/src/auth/useAuthState.ts b/src/auth/useAuthState.ts
@@ -2,6 +2,7 @@ import { Auth, AuthError, onAuthStateChanged, User } from "firebase/auth";
 import { useCallback } from "react";
 import { ValueHookResult } from "../common";
 import { useListen, UseListenOnChange } from "../internal/useListen";
+import { LoadingState } from "../internal/useLoadingValue";
 
 export type UseAuthStateResult = ValueHookResult<User | null, AuthError>;
 
@@ -20,5 +21,5 @@ export function useAuthState(auth: Auth): UseAuthStateResult {
         []
     );
 
-    return useListen(auth, onChange, () => true, auth.currentUser ?? undefined);
+    return useListen(auth, onChange, () => true, auth.currentUser ? auth.currentUser : LoadingState);
 }
diff --git a/src/database/useObject.ts b/src/database/useObject.ts
@@ -1,6 +1,7 @@
 import { DataSnapshot, onValue, Query } from "firebase/database";
 import { ValueHookResult } from "../common";
 import { useListen } from "../internal/useListen";
+import { LoadingState } from "../internal/useLoadingValue";
 import { isQueryEqual } from "./internal";
 
 export type UseObjectResult = ValueHookResult<DataSnapshot, Error>;
@@ -15,5 +16,5 @@ export type UseObjectResult = ValueHookResult<DataSnapshot, Error>;
  * * error: `undefined` if no error occurred
  */
 export function useObject(query: Query | undefined | null): UseObjectResult {
-    return useListen(query ?? undefined, onValue, isQueryEqual);
+    return useListen(query ?? undefined, onValue, isQueryEqual, LoadingState);
 }
diff --git a/src/database/useObjectValue.ts b/src/database/useObjectValue.ts
@@ -2,6 +2,7 @@ import { DataSnapshot, onValue, Query } from "firebase/database";
 import { useCallback } from "react";
 import { ValueHookResult } from "../common";
 import { useListen, UseListenOnChange } from "../internal/useListen";
+import { LoadingState } from "../internal/useLoadingValue";
 import { isQueryEqual } from "./internal";
 
 export type UseObjectValueResult<Value = unknown> = ValueHookResult<Value, Error>;
@@ -35,7 +36,5 @@ export function useObjectValue<Value = unknown>(
         []
     );
 
-    return useListen(query ?? undefined, onChange, isQueryEqual);
+    return useListen(query ?? undefined, onChange, isQueryEqual, LoadingState);
 }
-
-useObjectValue<string>(null);
diff --git a/src/firestore/useCollection.ts b/src/firestore/useCollection.ts
@@ -2,6 +2,7 @@ import { DocumentData, FirestoreError, onSnapshot, Query, QuerySnapshot, Snapsho
 import { useCallback } from "react";
 import { ValueHookResult } from "../common/types";
 import { useListen, UseListenOnChange } from "../internal/useListen";
+import { LoadingState } from "../internal/useLoadingValue";
 import { isQueryEqual } from "./internal";
 
 export type UseCollectionResult<Value extends DocumentData = DocumentData> = ValueHookResult<
@@ -42,5 +43,5 @@ export function useCollection<Value extends DocumentData = DocumentData>(
         []
     );
 
-    return useListen(query ?? undefined, onChange, isQueryEqual);
+    return useListen(query ?? undefined, onChange, isQueryEqual, LoadingState);
 }
diff --git a/src/firestore/useCollectionData.ts b/src/firestore/useCollectionData.ts
@@ -2,6 +2,7 @@ import { DocumentData, FirestoreError, onSnapshot, Query, SnapshotListenOptions,
 import { useCallback } from "react";
 import { ValueHookResult } from "../common/types";
 import { useListen, UseListenOnChange } from "../internal/useListen";
+import { LoadingState } from "../internal/useLoadingValue";
 import { isQueryEqual } from "./internal";
 
 export type UseCollectionDataResult<Value extends DocumentData = DocumentData> = ValueHookResult<Value[], FirestoreError>;
@@ -40,5 +41,5 @@ export function useCollectionData<Value extends DocumentData = DocumentData>(
         []
     );
 
-    return useListen(query ?? undefined, onChange, isQueryEqual);
+    return useListen(query ?? undefined, onChange, isQueryEqual, LoadingState);
 }
diff --git a/src/firestore/useDocument.ts b/src/firestore/useDocument.ts
@@ -9,6 +9,7 @@ import {
 import { useCallback } from "react";
 import type { ValueHookResult } from "../common/types";
 import { useListen, UseListenOnChange } from "../internal/useListen";
+import { LoadingState } from "../internal/useLoadingValue";
 import { isDocRefEqual } from "./internal";
 
 export type UseDocumentResult<Value extends DocumentData = DocumentData> = ValueHookResult<
@@ -49,5 +50,5 @@ export function useDocument<Value extends DocumentData = DocumentData>(
         []
     );
 
-    return useListen(reference ?? undefined, onChange, isDocRefEqual);
+    return useListen(reference ?? undefined, onChange, isDocRefEqual, LoadingState);
 }
diff --git a/src/firestore/useDocumentData.ts b/src/firestore/useDocumentData.ts
@@ -9,6 +9,7 @@ import {
 import { useCallback } from "react";
 import type { ValueHookResult } from "../common/types";
 import { useListen, UseListenOnChange } from "../internal/useListen";
+import { LoadingState } from "../internal/useLoadingValue";
 import { isDocRefEqual } from "./internal";
 
 export type UseDocumentDataResult<Value extends DocumentData = DocumentData> = ValueHookResult<Value, FirestoreError>;
@@ -47,5 +48,5 @@ export function useDocumentData<Value extends DocumentData = DocumentData>(
         []
     );
 
-    return useListen(reference ?? undefined, onChange, isDocRefEqual);
+    return useListen(reference ?? undefined, onChange, isDocRefEqual, LoadingState);
 }
diff --git a/src/internal/useListen.spec.ts b/src/internal/useListen.spec.ts
@@ -1,6 +1,7 @@
 import { act, renderHook } from "@testing-library/react-hooks";
 import { newSymbol } from "../__testfixtures__";
 import { useListen } from "./useListen";
+import { LoadingState } from "./useLoadingValue";
 
 const result1 = newSymbol("Result 1");
 const result2 = newSymbol("Result 2");
@@ -24,62 +25,99 @@ beforeEach(() => {
 });
 
 describe("initial state", () => {
-    it("with undefined reference", () => {
-        const { result } = renderHook(() => useListen(undefined, onChange, isEqual));
+    it.each`
+        reference    | initialState    | expectedValue | expectedLoading
+        ${undefined} | ${result1}      | ${undefined}  | ${false}
+        ${undefined} | ${undefined}    | ${undefined}  | ${false}
+        ${undefined} | ${LoadingState} | ${undefined}  | ${false}
+        ${refA1}     | ${result1}      | ${result1}    | ${false}
+        ${refA1}     | ${undefined}    | ${undefined}  | ${false}
+        ${refA1}     | ${LoadingState} | ${undefined}  | ${true}
+    `(
+        "reference=$reference initialState=$initialState",
+        ({ reference, initialState, expectedValue, expectedLoading }: any) => {
+            const { result } = renderHook(() => useListen(reference, onChange, isEqual, initialState));
+            expect(result.current).toStrictEqual([expectedValue, expectedLoading, undefined]);
+        }
+    );
+});
 
-        expect(onChange).toHaveBeenCalledTimes(0);
-        expect(result.current).toStrictEqual([undefined, false, undefined]);
-    });
+describe("when changing ref", () => {
+    it("should not resubscribe for equal ref", async () => {
+        // first ref
+        const { result, rerender } = renderHook(({ ref }) => useListen(ref, onChange, isEqual, LoadingState), {
+            initialProps: { ref: refA1 },
+        });
+        expect(onChangeUnsubscribe).toHaveBeenCalledTimes(0);
+        expect(onChange).toHaveBeenCalledTimes(1);
 
-    it("with defined reference", () => {
-        const { result } = renderHook(() => useListen(refA1, onChange, isEqual));
+        // emit value
+        act(() => onChange.mock.calls[0][1](result1));
+        expect(result.current).toStrictEqual([result1, false, undefined]);
 
+        // change ref
+        rerender({ ref: refA2 });
+        expect(result.current).toStrictEqual([result1, false, undefined]);
+        expect(onChangeUnsubscribe).toHaveBeenCalledTimes(0);
         expect(onChange).toHaveBeenCalledTimes(1);
-        expect(onChange).toHaveBeenCalledWith(refA1, expect.any(Function), expect.any(Function));
-
-        expect(result.current).toStrictEqual([undefined, true, undefined]);
     });
 
-    it("with default value", () => {
-        const { result } = renderHook(() => useListen(refA1, onChange, isEqual, result1));
-
+    it("should resubscribe for different ref", () => {
+        // first ref
+        const { result, rerender } = renderHook(({ ref }) => useListen(ref, onChange, isEqual, LoadingState), {
+            initialProps: { ref: refA1 },
+        });
+        expect(onChangeUnsubscribe).toHaveBeenCalledTimes(0);
         expect(onChange).toHaveBeenCalledTimes(1);
-        expect(onChange).toHaveBeenCalledWith(refA1, expect.any(Function), expect.any(Function));
 
+        // emit value
+        act(() => onChange.mock.calls[0][1](result1));
         expect(result.current).toStrictEqual([result1, false, undefined]);
+
+        // change ref
+        rerender({ ref: refB1 });
+        expect(result.current).toStrictEqual([undefined, true, undefined]);
+        expect(onChange).toHaveBeenCalledTimes(2);
+        expect(onChangeUnsubscribe).toHaveBeenCalledTimes(1);
+
+        // emit value
+        act(() => onChange.mock.calls[1][1](result2));
+        expect(result.current).toStrictEqual([result2, false, undefined]);
     });
-});
 
-describe("when changing ref", () => {
-    it("should not resubscribe for equal ref", () => {
-        const { rerender } = renderHook(({ ref }) => useListen(ref, onChange, isEqual), {
-            initialProps: { ref: refA1 },
+    it("from undefined ref to defined", () => {
+        const { result, rerender } = renderHook(({ ref }) => useListen(ref, onChange, isEqual, LoadingState), {
+            initialProps: { ref: undefined },
         });
 
-        expect(onChange).toHaveBeenCalledTimes(1);
+        expect(onChangeUnsubscribe).toHaveBeenCalledTimes(0);
+        expect(onChange).toHaveBeenCalledTimes(0);
 
-        rerender({ ref: refA2 });
+        rerender({ ref: refA1 });
+        expect(result.current).toStrictEqual([undefined, true, undefined]);
 
+        expect(onChangeUnsubscribe).toHaveBeenCalledTimes(0);
         expect(onChange).toHaveBeenCalledTimes(1);
     });
 
-    it("should resubscribe for different ref", () => {
-        const { rerender } = renderHook(({ ref }) => useListen(ref, onChange, isEqual), {
+    it("from defined ref to undefined", () => {
+        const { result, rerender } = renderHook(({ ref }) => useListen(ref, onChange, isEqual, LoadingState), {
             initialProps: { ref: refA1 },
         });
 
         expect(onChangeUnsubscribe).toHaveBeenCalledTimes(0);
         expect(onChange).toHaveBeenCalledTimes(1);
 
-        rerender({ ref: refB1 });
+        rerender({ ref: undefined });
+        expect(result.current).toStrictEqual([undefined, false, undefined]);
 
-        expect(onChange).toHaveBeenCalledTimes(2);
         expect(onChangeUnsubscribe).toHaveBeenCalledTimes(1);
+        expect(onChange).toHaveBeenCalledTimes(1);
     });
 });
 
 it("should return emitted values", () => {
-    const { result } = renderHook(() => useListen(refA1, onChange, isEqual));
+    const { result } = renderHook(() => useListen(refA1, onChange, isEqual, LoadingState));
     const setValue = onChange.mock.calls[0][1];
 
     expect(result.current).toStrictEqual([undefined, true, undefined]);
@@ -96,7 +134,7 @@ it("should return emitted values", () => {
 });
 
 it("should return emitted error", () => {
-    const { result } = renderHook(() => useListen(refA1, onChange, isEqual));
+    const { result } = renderHook(() => useListen(refA1, onChange, isEqual, LoadingState));
     const setValue = onChange.mock.calls[0][1];
     const setError = onChange.mock.calls[0][2];
 

diff --git a/src/internal/useListen.ts b/src/internal/useListen.ts
@@ -1,6 +1,6 @@
 import { useEffect, useMemo, useRef } from "react";
 import { ValueHookResult } from "../common";
-import { useLoadingValue } from "./useLoadingValue";
+import { useLoadingValue, LoadingState } from "./useLoadingValue";
 import { useStableValue } from "./useStableValue";
 
 /**
@@ -19,16 +19,23 @@ export function useListen<Value, Error, Reference>(
     reference: Reference | undefined,
     onChange: UseListenOnChange<Value, Error, Reference>,
     isEqual: (a: Reference | undefined, b: Reference | undefined) => boolean,
-    defaultValue?: Value
+    initialState: Value | typeof LoadingState
 ): ValueHookResult<Value, Error> {
-    const { error, loading, setLoading, setError, setValue, value } = useLoadingValue<Value, Error>(defaultValue);
+    const { error, loading, setLoading, setError, setValue, value } = useLoadingValue<Value, Error>(
+        reference === undefined ? undefined : initialState
+    );
 
     const stableRef = useStableValue(reference ?? undefined, isEqual);
     const firstRender = useRef<boolean>(true);
 
     useEffect(() => {
         if (stableRef === undefined) {
-            setValue();
+            // value doesn't change on first render with undefined ref
+            if (firstRender.current) {
+                firstRender.current = false;
+            } else {
+                setValue();
+            }
         } else {
             // do not set loading state on first render
             // otherwise, the defaultValue gets overwritten

diff --git a/src/internal/useLoadingValue.spec.ts b/src/internal/useLoadingValue.spec.ts
@@ -1,13 +1,13 @@
 import { act, renderHook } from "@testing-library/react-hooks";
 import { newSymbol } from "../__testfixtures__";
-import { useLoadingValue } from "./useLoadingValue";
+import { LoadingState, useLoadingValue } from "./useLoadingValue";
 
 const value = newSymbol("Value");
 const error = newSymbol("Error");
 
 describe("initial state", () => {
     it("without default value", () => {
-        const { result } = renderHook(() => useLoadingValue());
+        const { result } = renderHook(() => useLoadingValue(LoadingState));
 
         expect(result.current.value).toBeUndefined();
         expect(result.current.loading).toBe(true);
@@ -21,6 +21,14 @@ describe("initial state", () => {
         expect(result.current.loading).toBe(false);
         expect(result.current.error).toBeUndefined();
     });
+
+    it("with default value undefined", () => {
+        const { result } = renderHook(() => useLoadingValue(undefined));
+
+        expect(result.current.value).toBe(undefined);
+        expect(result.current.loading).toBe(false);
+        expect(result.current.error).toBeUndefined();
+    });
 });
 
 describe("setValue", () => {
@@ -34,7 +42,7 @@ describe("setValue", () => {
     });
 
     it("with a value", () => {
-        const { result } = renderHook(() => useLoadingValue<Symbol>());
+        const { result } = renderHook(() => useLoadingValue<Symbol>(LoadingState));
         act(() => result.current.setValue(value));
 
         expect(result.current.value).toBe(value);
@@ -43,7 +51,7 @@ describe("setValue", () => {
     });
 
     it("with error", () => {
-        const { result } = renderHook(() => useLoadingValue<Symbol>());
+        const { result } = renderHook(() => useLoadingValue<Symbol>(LoadingState));
         act(() => result.current.setError(error));
 
         act(() => result.current.setValue(value));
@@ -56,7 +64,7 @@ describe("setValue", () => {
 
 describe("setError", () => {
     it("without value", () => {
-        const { result } = renderHook(() => useLoadingValue<Symbol>());
+        const { result } = renderHook(() => useLoadingValue<Symbol>(LoadingState));
         act(() => result.current.setError(error));
 
         expect(result.current.value).toBeUndefined();

diff --git a/src/internal/useLoadingValue.ts b/src/internal/useLoadingValue.ts
@@ -1,5 +1,7 @@
 import { useCallback, useMemo, useState } from "react";
 
+export const LoadingState = Symbol();
+
 /**
  * @internal
  */
@@ -13,7 +15,7 @@ interface State<Value, Error> {
  * @internal
  */
 export interface UseLoadingValueResult<Value, Error> {
-    value?: Value;
+    value: Value | undefined;
     setValue: (value?: Value) => void;
     loading: boolean;
     setLoading: () => void;
@@ -24,11 +26,13 @@ export interface UseLoadingValueResult<Value, Error> {
 /**
  * @internal
  */
-export function useLoadingValue<Value, Error = unknown>(defaultValue?: Value): UseLoadingValueResult<Value, Error> {
+export function useLoadingValue<Value, Error = unknown>(
+    initialState: Value | undefined | typeof LoadingState
+): UseLoadingValueResult<Value, Error> {
     const [state, setState] = useState<State<Value, Error>>({
         error: undefined,
-        loading: defaultValue === undefined ? true : false,
-        value: defaultValue,
+        loading: initialState === LoadingState ? true : false,
+        value: initialState === LoadingState ? undefined : initialState,
     });
 
     const setValue = useCallback((value?: Value) => {