Refactor on the core observable engine (#210)

Refactoring `useObservable` and `preloadObservable` to be more suspense friendly.

The core idea here is that our "observable" should expose a promise that resolves on first emission (which can be thrown for suspense), a synchronous means of getting the latest value (to pass into `useState`), and a shared subscription for use thereafter (for `useEffect`).

As a matter of optimization they will keep their cache around for only a present amount of time (30 seconds by default) unless they are subscribed to (`useEffect`); in which case the cache will remain until the component is unloaded. This should be enough time to prevent suspense from thrashing.

To encapsulate this behavior this we've implemented our own `Subject`.

Other changes in this PR:

* Ensure that the observable cache has to be keyed very specifically taking into account the firebase app, options, etc.
* Make sure no cache keys overlap on tests & different exports
* We're using more typescript in the tests, build them as part of the main build step since jest wasn't playing nice
* There's still some meh types kicking around here, should take as an AI
* Errors are thrown again until the cache timeout elapses, put down a TODO for now
* `FirebaseAppProvider` now throws if the provided config does not match the existing instance options, rather than simply returning the default
* `FirebaseAppProvider` now takes an `appName` property allowing you to have more than one app

Author: jamesdaniels

.gitignore

@@ -10,7 +10,9 @@ node_modules/
 
 reactfire/docs/reactfire-metadata.json
 reactfire/firestore-debug.log
+reactfire/firebase-debug.log
 reactfire/pub/**
 pub
 yarn-error.log
 reactfire/database-debug.log
+reactfire/reactfire-*.tgz

reactfire/.npmignore

@@ -0,0 +1,3 @@
+cjs/index.esm-*
+**/*.test.d.ts
+**/*.test.js

reactfire/auth/auth.test.tsx

@@ -15,6 +15,10 @@ class MockAuth {
     this.subscriber = null;
   }
 
+  app = {
+    name: '[DEFAULT]'
+  };
+
   notifySubscriber() {
     if (this.subscriber) {
       this.subscriber.next(this.user);
@@ -39,16 +43,16 @@ const mockFirebase = {
 };
 
 const Provider = ({ children }) => (
-  <FirebaseAppProvider firebaseApp={mockFirebase}>
+  <FirebaseAppProvider firebaseApp={(mockFirebase as any) as firebase.app.App}>
     {children}
   </FirebaseAppProvider>
 );
 
-const Component = ({ children }) => (
+const Component = (props?: { children?: any }) => (
   <Provider>
     <React.Suspense fallback={'loading'}>
       <AuthCheck fallback={<h1 data-testid="signed-out">not signed in</h1>}>
-        {children || <h1 data-testid="signed-in">signed in</h1>}
+        {props?.children || <h1 data-testid="signed-in">signed in</h1>}
       </AuthCheck>
     </React.Suspense>
   </Provider>
@@ -57,12 +61,14 @@ const Component = ({ children }) => (
 describe('AuthCheck', () => {
   beforeEach(() => {
     // clear the signed in user
-    mockFirebase.auth().updateUser(null);
+    act(() => mockFirebase.auth().updateUser(null));
   });
 
   afterEach(() => {
-    cleanup();
-    jest.clearAllMocks();
+    act(() => {
+      cleanup();
+      jest.clearAllMocks();
+    });
   });
 
   it('can find firebase Auth from Context', () => {
@@ -95,7 +101,7 @@ describe('AuthCheck', () => {
   });
 
   it('renders children if a user is logged in', async () => {
-    mockFirebase.auth().updateUser({ uid: 'testuser' });
+    act(() => mockFirebase.auth().updateUser({ uid: 'testuser' }));
     const { getByTestId } = render(<Component />);
 
     await wait(() => expect(getByTestId('signed-in')).toBeInTheDocument());

reactfire/auth/index.tsx

@@ -14,9 +14,9 @@ export function preloadUser(firebaseApp: firebase.app.App) {
   return preloadAuth(firebaseApp).then(auth => {
     const result = preloadObservable(
       user(auth() as firebase.auth.Auth),
-      'auth: user'
+      `auth:user:${firebaseApp.name}`
     );
-    return result.request.promise;
+    return result.toPromise();
   });
 }
 
@@ -31,18 +31,8 @@ export function useUser<T = unknown>(
   options?: ReactFireOptions<T>
 ): User | T {
   auth = auth || useAuth()();
-
-  let currentUser = undefined;
-
-  if (options && options.startWithValue !== undefined) {
-    currentUser = options.startWithValue;
-  } else if (auth.currentUser) {
-    // if auth.currentUser is undefined or null, we won't use it
-    // because null can mean "not signed in" OR "still loading"
-    currentUser = auth.currentUser;
-  }
-
-  return useObservable(user(auth), 'auth: user', currentUser);
+  const currentUser = auth.currentUser || options?.startWithValue;
+  return useObservable(user(auth), `auth:user:${auth.app.name}`, currentUser);
 }
 
 export function useIdTokenResult(user: User, forceRefresh: boolean = false) {
@@ -52,7 +42,10 @@ export function useIdTokenResult(user: User, forceRefresh: boolean = false) {
 
   const idToken$ = from(user.getIdTokenResult(forceRefresh));
 
-  return useObservable<any>(idToken$, `${user.uid}-claims`);
+  return useObservable<any>(
+    idToken$,
+    `auth:idTokenResult:${user.uid}:forceRefresh=${forceRefresh}`
+  );
 }
 
 export interface AuthCheckProps {

reactfire/database/index.tsx

@@ -22,7 +22,7 @@ export function useDatabaseObject<T = unknown>(
 ): QueryChange | T {
   return useObservable(
     object(ref),
-    `RTDB Doc: ${ref.toString()}`,
+    `database:object:${ref.toString()}`,
     options ? options.startWithValue : undefined
   );
 }
@@ -54,9 +54,10 @@ export function useDatabaseObjectData<T>(
   ref: database.Reference,
   options?: ReactFireOptions<T>
 ): T {
+  const idField = checkIdField(options);
   return useObservable(
-    objectVal(ref, checkIdField(options)),
-    `RTDB DocData: ${ref.toString()}`,
+    objectVal(ref, idField),
+    `database:objectVal:${ref.toString()}:idField=${idField}`,
     checkStartWithValue(options)
   );
 }
@@ -78,7 +79,7 @@ export function useDatabaseList<T = { [key: string]: unknown }>(
   ref: database.Reference | database.Query,
   options?: ReactFireOptions<T[]>
 ): QueryChange[] | T[] {
-  const hash = `RTDB List: ${ref.toString()}|${(ref as _QueryWithId).queryIdentifier()}`;
+  const hash = `database:list:${ref.toString()}|${(ref as _QueryWithId).queryIdentifier()}`;
 
   return useObservable(
     list(ref),
@@ -91,9 +92,10 @@ export function useDatabaseListData<T = { [key: string]: unknown }>(
   ref: database.Reference | database.Query,
   options?: ReactFireOptions<T[]>
 ): T[] {
+  const idField = checkIdField(options);
   return useObservable(
-    listVal(ref, checkIdField(options)),
-    `RTDB ListData: ${ref.toString()}`,
+    listVal(ref, idField),
+    `database:listVal:${ref.toString()}|${(ref as _QueryWithId).queryIdentifier()}:idField=${idField}`,
     checkStartWithValue(options)
   );
 }

reactfire/firebaseApp/firebaseApp.test.tsx

@@ -8,21 +8,21 @@ import { FirebaseAppProvider } from './index';
 
 afterEach(cleanup);
 
+const DEFAULT_APP_CONFIG = { appId: '12345' };
+
 describe('FirebaseAppProvider', () => {
   it('calls firebase.initializeApp with the provided config', () => {
-    const config = { appId: '12345' };
-
     const spy = jest.spyOn(firebase, 'initializeApp');
 
-    render(<FirebaseAppProvider firebaseConfig={config} />);
-    expect(spy).toBeCalledWith(config);
+    render(<FirebaseAppProvider firebaseConfig={DEFAULT_APP_CONFIG} />);
+    expect(spy).toBeCalledWith(DEFAULT_APP_CONFIG, undefined);
 
     spy.mockRestore();
   });
 
   it('does not call firebase.initializeApp if the firebaseApp is provided', () => {
     const spy = jest.spyOn(firebase, 'initializeApp');
-    const app = {};
+    const app: firebase.app.App = {} as any;
     render(<FirebaseAppProvider firebaseApp={app} />);
     expect(spy).not.toBeCalled();
 
@@ -32,7 +32,7 @@ describe('FirebaseAppProvider', () => {
   it('initializes fireperf if specified', async () => {
     const mockPerf = jest.fn();
     firebase['performance' as any] = mockPerf;
-    const app = { performance: mockPerf };
+    const app: firebase.app.App = { performance: mockPerf } as any;
 
     render(<FirebaseAppProvider firebaseApp={app} initPerformance />);
 
@@ -42,7 +42,7 @@ describe('FirebaseAppProvider', () => {
   it('does not initialize fireperf if not specified', async () => {
     const mockPerf = jest.fn();
     firebase['performance' as any] = mockPerf;
-    const app = { performance: mockPerf };
+    const app: firebase.app.App = { performance: mockPerf } as any;
 
     render(<FirebaseAppProvider firebaseApp={app} />);
 
@@ -52,7 +52,7 @@ describe('FirebaseAppProvider', () => {
 
 describe('useFirebaseApp', () => {
   it('finds firebase from Context', () => {
-    const firebaseApp = { a: 1 };
+    const firebaseApp: firebase.app.App = { a: 1 } as any;
 
     const wrapper = ({ children }) => (
       <FirebaseAppProvider firebaseApp={firebaseApp}>
@@ -65,6 +65,57 @@ describe('useFirebaseApp', () => {
     expect(result.current).toBe(firebaseApp);
   });
 
+  it('can initialize more than one firebase app', () => {
+    const config = { a: 1 };
+
+    const initializeApp = jest.spyOn(firebase, 'initializeApp');
+
+    const wrapper = ({ children }) => (
+      <div>
+        <FirebaseAppProvider firebaseConfig={DEFAULT_APP_CONFIG}>
+          {children}
+        </FirebaseAppProvider>
+        <FirebaseAppProvider firebaseConfig={config} appName="app-2">
+          appA
+        </FirebaseAppProvider>
+      </div>
+    );
+
+    const { result } = renderHook(() => useFirebaseApp(), { wrapper });
+
+    expect(initializeApp).toBeCalledWith(config, 'app-2');
+    initializeApp.mockRestore();
+
+    expect(result.error).toBeUndefined();
+  });
+
+  it('will throw if configs dont match, and same name', () => {
+    const config = { a: 1 };
+
+    const initializeApp = jest.spyOn(firebase, 'initializeApp');
+
+    const wrapper = ({ children }) => (
+      <div>
+        <FirebaseAppProvider firebaseConfig={DEFAULT_APP_CONFIG}>
+          {children}
+        </FirebaseAppProvider>
+        <FirebaseAppProvider firebaseConfig={config}>appA</FirebaseAppProvider>
+      </div>
+    );
+
+    try {
+      const { result } = renderHook(() => useFirebaseApp(), { wrapper });
+      fail('expected a throw');
+    } catch (e) {
+      expect(e).toEqual(
+        'Does not match the options already provided to the default firebase app instance, give this new instance a different appName.'
+      );
+    }
+
+    expect(initializeApp).not.toBeCalled();
+    initializeApp.mockRestore();
+  });
+
   it('throws an error if Firebase is not in context', () => {
     const { result } = renderHook(() => useFirebaseApp());
 

reactfire/firebaseApp/index.tsx

@@ -5,34 +5,54 @@ export * from './sdk';
 
 type FirebaseAppContextValue = firebase.app.App;
 
+// INVESTIGATE I don't like magic strings, can we have export this in js-sdk?
+const DEFAULT_APP_NAME = '[DEFAULT]';
+
 const FirebaseAppContext = React.createContext<
   FirebaseAppContextValue | undefined
 >(undefined);
 
-export function FirebaseAppProvider(props) {
-  const { firebaseConfig, initPerformance } = props;
-  let { firebaseApp } = props;
+type Props = {
+  initPerformance?: boolean;
+  firebaseApp?: firebase.app.App;
+  firebaseConfig?: Object;
+  appName?: string;
+};
+
+const shallowEq = (a: Object, b: Object) =>
+  a == b ||
+  [...Object.keys(a), ...Object.keys(b)].every(key => a[key] == b[key]);
 
-  firebaseApp =
-    firebaseApp ||
+export function FirebaseAppProvider(props: Props & { [key: string]: unknown }) {
+  const { firebaseConfig, appName, initPerformance = false } = props;
+  const firebaseApp: firebase.app.App =
+    props.firebaseApp ||
     React.useMemo(() => {
-      if (!firebase.apps.length) {
-        firebase.initializeApp(firebaseConfig);
+      const existingApp = firebase.apps.find(
+        app => app.name == (appName || DEFAULT_APP_NAME)
+      );
+      if (existingApp) {
+        if (shallowEq(existingApp.options, firebaseConfig)) {
+          return existingApp;
+        } else {
+          throw `Does not match the options already provided to the ${appName ||
+            'default'} firebase app instance, give this new instance a different appName.`;
+        }
+      } else {
+        return firebase.initializeApp(firebaseConfig, appName);
       }
-
-      return firebase;
-    }, [firebaseConfig]);
+    }, [firebaseConfig, appName]);
 
   React.useMemo(() => {
-    if (initPerformance === true && !!firebase.apps.length) {
-      if (!firebase.performance) {
+    if (initPerformance === true) {
+      if (firebaseApp.performance) {
+        // initialize Performance Monitoring
+        firebaseApp.performance();
+      } else {
         throw new Error(
           'firebase.performance not found. Did you forget to import it?'
         );
       }
-
-      // initialize Performance Monitoring
-      firebase.performance();
     }
   }, [initPerformance, firebaseApp]);