Fixed onComplete for FDC

.changeset/spotty-shirts-design.md

@@ -0,0 +1,5 @@
+---
+"@firebase/data-connect": patch
+---
+
+Fixed issue where onComplete wasn't triggering when the user calls `unsubscribe` on a subscription.

common/api-review/data-connect.api.md

@@ -109,16 +109,6 @@ export interface DataConnectResult<Data, Variables> extends OpResult<Data> {
     ref: OperationRef<Data, Variables>;
 }
 
-// @public
-export interface DataConnectSubscription<Data, Variables> {
-    // (undocumented)
-    errCallback?: (e?: DataConnectError) => void;
-    // (undocumented)
-    unsubscribe: () => void;
-    // (undocumented)
-    userCallback: OnResultSubscription<Data, Variables>;
-}
-
 // @public (undocumented)
 export type DataSource = typeof SOURCE_CACHE | typeof SOURCE_SERVER;
 

packages/data-connect/src/api.browser.ts

@@ -102,6 +102,7 @@ export function subscribe<Data, Variables>(
   return ref.dataConnect._queryManager.addSubscription(
     ref,
     onResult,
+    onComplete,
     onError,
     initialCache
   );

packages/data-connect/src/api/DataConnect.ts

@@ -198,6 +198,7 @@ export class DataConnect {
   // @internal
   enableEmulator(transportOptions: TransportOptions): void {
     if (
+      this._transportOptions &&
       this._initialized &&
       !areTransportOptionsEqual(this._transportOptions, transportOptions)
     ) {
@@ -314,7 +315,10 @@ export function validateDCOptions(dcOptions: ConnectorConfig): boolean {
     throw new DataConnectError(Code.INVALID_ARGUMENT, 'DC Option Required');
   }
   fields.forEach(field => {
-    if (dcOptions[field] === null || dcOptions[field] === undefined) {
+    if (
+      dcOptions[field as keyof ConnectorConfig] === null ||
+      dcOptions[field as keyof ConnectorConfig] === undefined
+    ) {
       throw new DataConnectError(Code.INVALID_ARGUMENT, `${field} Required`);
     }
   });

packages/data-connect/src/api/query.ts

@@ -39,14 +39,6 @@ export type OnErrorSubscription = (err?: DataConnectError) => void;
  * Signature for unsubscribe from `subscribe`
  */
 export type QueryUnsubscribe = () => void;
-/**
- * Representation of user provided subscription options.
- */
-export interface DataConnectSubscription<Data, Variables> {
-  userCallback: OnResultSubscription<Data, Variables>;
-  errCallback?: (e?: DataConnectError) => void;
-  unsubscribe: () => void;
-}
 
 /**
  * QueryRef object
@@ -124,7 +116,7 @@ export function queryRef<Data, Variables>(
     dataConnect: dcInstance,
     refType: QUERY_STR,
     name: queryName,
-    variables
+    variables: variables as Variables
   };
 }
 /**

packages/data-connect/src/core/AppCheckTokenProvider.ts

@@ -29,7 +29,7 @@ import { Provider } from '@firebase/component';
  * Abstraction around AppCheck's token fetching capabilities.
  */
 export class AppCheckTokenProvider {
-  private appCheck?: FirebaseAppCheckInternal;
+  private appCheck?: FirebaseAppCheckInternal | null;
   private serverAppAppCheckToken?: string;
   constructor(
     app: FirebaseApp,
@@ -47,13 +47,13 @@ export class AppCheckTokenProvider {
     }
   }
 
-  getToken(): Promise<AppCheckTokenResult> {
+  getToken(): Promise<AppCheckTokenResult | null> {
     if (this.serverAppAppCheckToken) {
       return Promise.resolve({ token: this.serverAppAppCheckToken });
     }
 
     if (!this.appCheck) {
-      return new Promise<AppCheckTokenResult>((resolve, reject) => {
+      return new Promise<AppCheckTokenResult | null>((resolve, reject) => {
         // Support delayed initialization of FirebaseAppCheck. This allows our
         // customers to initialize the RTDB SDK before initializing Firebase
         // AppCheck and ensures that all requests are authenticated if a token

packages/data-connect/src/core/QueryManager.ts

@@ -16,7 +16,7 @@
  */
 
 import {
-  DataConnectSubscription,
+  OnCompleteSubscription,
   OnErrorSubscription,
   OnResultSubscription,
   QueryPromise,
@@ -39,6 +39,16 @@ import { setIfNotExists } from '../util/map';
 
 import { Code, DataConnectError } from './error';
 
+/**
+ * Representation of user provided subscription options.
+ */
+interface DataConnectSubscription<Data, Variables> {
+  userCallback: OnResultSubscription<Data, Variables>;
+  errCallback?: (e?: DataConnectError) => void;
+  onCompleteCallback?: () => void;
+  unsubscribe: () => void;
+}
+
 interface TrackedQuery<Data, Variables> {
   ref: Omit<OperationRef<Data, Variables>, 'dataConnect'>;
   subscriptions: Array<DataConnectSubscription<Data, Variables>>;
@@ -97,6 +107,7 @@ export class QueryManager {
   addSubscription<Data, Variables>(
     queryRef: OperationRef<Data, Variables>,
     onResultCallback: OnResultSubscription<Data, Variables>,
+    onCompleteCallback?: OnCompleteSubscription,
     onErrorCallback?: OnErrorSubscription,
     initialCache?: OpResult<Data>
   ): () => void {
@@ -111,13 +122,15 @@ export class QueryManager {
     >;
     const subscription = {
       userCallback: onResultCallback,
+      onCompleteCallback,
       errCallback: onErrorCallback
     };
     const unsubscribe = (): void => {
       const trackedQuery = this._queries.get(key)!;
       trackedQuery.subscriptions = trackedQuery.subscriptions.filter(
         sub => sub !== subscription
       );
+      onCompleteCallback?.();
     };
     if (initialCache && trackedQuery.currentCache !== initialCache) {
       logDebug('Initial cache found. Comparing dates.');

packages/data-connect/src/network/fetch.ts

@@ -56,9 +56,9 @@ export function dcFetch<T, U>(
   url: string,
   body: DataConnectFetchBody<U>,
   { signal }: AbortController,
-  appId: string | null,
+  appId: string | null | undefined,
   accessToken: string | null,
-  appCheckToken: string | null,
+  appCheckToken: string | null | undefined,
   _isUsingGen: boolean,
   _callerSdkType: CallerSdkType,
   _isUsingEmulator: boolean
@@ -135,7 +135,7 @@ interface MessageObject {
   message?: string;
 }
 function getMessage(obj: MessageObject): string {
-  if ('message' in obj) {
+  if ('message' in obj && obj.message) {
     return obj.message;
   }
   return JSON.stringify(obj);

packages/data-connect/src/network/transport/rest.ts

@@ -34,7 +34,7 @@ export class RESTTransport implements DataConnectTransport {
   private _project = 'p';
   private _serviceName: string;
   private _accessToken: string | null = null;
-  private _appCheckToken: string | null = null;
+  private _appCheckToken: string | null | undefined = null;
   private _lastToken: string | null = null;
   private _isUsingEmulator = false;
   constructor(
@@ -106,7 +106,7 @@ export class RESTTransport implements DataConnectTransport {
     this._accessToken = newToken;
   }
 
-  async getWithAuth(forceToken = false): Promise<string> {
+  async getWithAuth(forceToken = false): Promise<string | null> {
     let starterPromise: Promise<string | null> = new Promise(resolve =>
       resolve(this._accessToken)
     );

packages/data-connect/src/util/validateArgs.ts

@@ -46,7 +46,7 @@ export function validateArgs<Variables extends object>(
   let realVars: Variables;
   if (dcOrVars && 'enableEmulator' in dcOrVars) {
     dcInstance = dcOrVars as DataConnect;
-    realVars = vars;
+    realVars = vars as Variables;
   } else {
     dcInstance = getDataConnect(connectorConfig);
     realVars = dcOrVars as Variables;

packages/data-connect/test/unit/queries.test.ts

@@ -20,15 +20,18 @@ import { expect } from 'chai';
 import * as chai from 'chai';
 import chaiAsPromised from 'chai-as-promised';
 import * as sinon from 'sinon';
+import sinonChai from 'sinon-chai';
 
-import { DataConnectOptions } from '../../src';
+import { DataConnectOptions, QueryRef, queryRef, subscribe } from '../../src';
 import {
   AuthTokenListener,
   AuthTokenProvider
 } from '../../src/core/FirebaseAuthProvider';
 import { initializeFetch } from '../../src/network/fetch';
 import { RESTTransport } from '../../src/network/transport/rest';
+import { initDatabase } from '../util';
 chai.use(chaiAsPromised);
+chai.use(sinonChai);
 const options: DataConnectOptions = {
   connector: 'c',
   location: 'l',
@@ -61,10 +64,103 @@ const fakeFetchImpl = sinon.stub().returns(
     status: 401
   } as Response)
 );
+interface PostVariables {
+  testId: string;
+}
+const TEST_ID = crypto.randomUUID();
+interface PostListResponse {
+  posts: Post[];
+}
+interface Post {
+  id: string;
+  description: string;
+}
+function getPostsRef(): QueryRef<PostListResponse, PostVariables> {
+  const dc = initDatabase();
+  return queryRef<PostListResponse, PostVariables>(dc, 'ListPosts', {
+    testId: TEST_ID
+  });
+}
 describe('Queries', () => {
   afterEach(() => {
     fakeFetchImpl.resetHistory();
   });
+  it('should call onComplete callback after subscribe is called', async () => {
+    const taskListQuery = getPostsRef();
+    const onCompleteUserStub = sinon.stub();
+    const unsubscribe = subscribe(taskListQuery, {
+      onNext: () => {},
+      onComplete: onCompleteUserStub
+    });
+    expect(onCompleteUserStub).to.not.have.been.called;
+    unsubscribe();
+    expect(onCompleteUserStub).to.have.been.calledOnce;
+  });
+  it('should call onErr callback after a 401 occurs', async () => {
+    const json = {};
+    const throwErrorFakeImpl = sinon.stub().returns(
+      Promise.resolve({
+        json: () => {
+          return Promise.resolve(json);
+        },
+        status: 401
+      } as Response)
+    );
+    initializeFetch(throwErrorFakeImpl);
+    const taskListQuery = getPostsRef();
+    const onErrStub = sinon.stub();
+    let unsubscribeFn: (() => void) | null = null;
+    const promise = new Promise((resolve, reject) => {
+      unsubscribeFn = subscribe(taskListQuery, {
+        onNext: () => {
+          resolve(null);
+        },
+        onComplete: () => {},
+        onErr: err => {
+          onErrStub();
+          reject(err);
+        }
+      });
+    });
+    expect(onErrStub).not.to.have.been.called;
+    await expect(promise).to.have.eventually.been.rejected;
+    expect(onErrStub).to.have.been.calledOnce;
+    unsubscribeFn!();
+  });
+  it('should call onErr callback after a graphql error occurs', async () => {
+    const json = {
+      errors: [{ something: 'abc' }]
+    };
+    const throwErrorFakeImpl = sinon.stub().returns(
+      Promise.resolve({
+        json: () => {
+          return Promise.resolve(json);
+        },
+        status: 200
+      } as Response)
+    );
+    initializeFetch(throwErrorFakeImpl);
+    const taskListQuery = getPostsRef();
+    const onErrStub = sinon.stub();
+    let unsubscribeFn: (() => void) | null = null;
+    const promise = new Promise((resolve, reject) => {
+      unsubscribeFn = subscribe(taskListQuery, {
+        onNext: () => {
+          resolve(null);
+        },
+        onComplete: () => {},
+        onErr: err => {
+          onErrStub();
+          reject(err);
+        }
+      });
+    });
+    expect(onErrStub).not.to.have.been.called;
+    await expect(promise).to.have.eventually.been.rejected;
+    expect(onErrStub).to.have.been.calledOnce;
+    unsubscribeFn!();
+    initializeFetch(globalThis.fetch);
+  });
   it('[QUERY] should retry auth whenever the fetcher returns with unauthorized', async () => {
     initializeFetch(fakeFetchImpl);
     const authProvider = new FakeAuthProvider();

packages/data-connect/tsconfig.json

@@ -2,7 +2,7 @@
   "extends": "../../config/tsconfig.base.json",
   "compilerOptions": {
     "outDir": "dist",
-    "strict": false
+    "strict": true
   },
   "exclude": ["dist/**/*", "test/**/*"]
 }

scripts/emulator-testing/emulators/dataconnect-emulator.ts

@@ -18,7 +18,7 @@
 import { platform } from 'os';
 import { Emulator } from './emulator';
 
-const DATACONNECT_EMULATOR_VERSION = '1.9.2';
+const DATACONNECT_EMULATOR_VERSION = '2.15.1';
 
 export class DataConnectEmulator extends Emulator {
   constructor(port = 9399) {