diff --git a/.changeset/spotty-shirts-design.md b/.changeset/spotty-shirts-design.md new file mode 100644 index 00000000000..77c6a8c8307 --- /dev/null +++ b/.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. diff --git a/common/api-review/data-connect.api.md b/common/api-review/data-connect.api.md index 9e3d2424876..27a9d4af201 100644 --- a/common/api-review/data-connect.api.md +++ b/common/api-review/data-connect.api.md @@ -109,16 +109,6 @@ export interface DataConnectResult extends OpResult { ref: OperationRef; } -// @public -export interface DataConnectSubscription { - // (undocumented) - errCallback?: (e?: DataConnectError) => void; - // (undocumented) - unsubscribe: () => void; - // (undocumented) - userCallback: OnResultSubscription; -} - // @public (undocumented) export type DataSource = typeof SOURCE_CACHE | typeof SOURCE_SERVER; diff --git a/packages/data-connect/src/api.browser.ts b/packages/data-connect/src/api.browser.ts index 1ffcb8d1647..d31c3253537 100644 --- a/packages/data-connect/src/api.browser.ts +++ b/packages/data-connect/src/api.browser.ts @@ -102,6 +102,7 @@ export function subscribe( return ref.dataConnect._queryManager.addSubscription( ref, onResult, + onComplete, onError, initialCache ); diff --git a/packages/data-connect/src/api/DataConnect.ts b/packages/data-connect/src/api/DataConnect.ts index b7311363784..15e713ba23b 100644 --- a/packages/data-connect/src/api/DataConnect.ts +++ b/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`); } }); diff --git a/packages/data-connect/src/api/query.ts b/packages/data-connect/src/api/query.ts index a1cd0726160..43683cafd63 100644 --- a/packages/data-connect/src/api/query.ts +++ b/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 { - userCallback: OnResultSubscription; - errCallback?: (e?: DataConnectError) => void; - unsubscribe: () => void; -} /** * QueryRef object @@ -124,7 +116,7 @@ export function queryRef( dataConnect: dcInstance, refType: QUERY_STR, name: queryName, - variables + variables: variables as Variables }; } /** diff --git a/packages/data-connect/src/core/AppCheckTokenProvider.ts b/packages/data-connect/src/core/AppCheckTokenProvider.ts index 4b49a8f674a..615b45f3891 100644 --- a/packages/data-connect/src/core/AppCheckTokenProvider.ts +++ b/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 { + getToken(): Promise { if (this.serverAppAppCheckToken) { return Promise.resolve({ token: this.serverAppAppCheckToken }); } if (!this.appCheck) { - return new Promise((resolve, reject) => { + return new Promise((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 diff --git a/packages/data-connect/src/core/QueryManager.ts b/packages/data-connect/src/core/QueryManager.ts index 8b7c59aea85..109f1d105b4 100644 --- a/packages/data-connect/src/core/QueryManager.ts +++ b/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 { + userCallback: OnResultSubscription; + errCallback?: (e?: DataConnectError) => void; + onCompleteCallback?: () => void; + unsubscribe: () => void; +} + interface TrackedQuery { ref: Omit, 'dataConnect'>; subscriptions: Array>; @@ -97,6 +107,7 @@ export class QueryManager { addSubscription( queryRef: OperationRef, onResultCallback: OnResultSubscription, + onCompleteCallback?: OnCompleteSubscription, onErrorCallback?: OnErrorSubscription, initialCache?: OpResult ): () => void { @@ -111,6 +122,7 @@ export class QueryManager { >; const subscription = { userCallback: onResultCallback, + onCompleteCallback, errCallback: onErrorCallback }; const unsubscribe = (): void => { @@ -118,6 +130,7 @@ export class QueryManager { trackedQuery.subscriptions = trackedQuery.subscriptions.filter( sub => sub !== subscription ); + onCompleteCallback?.(); }; if (initialCache && trackedQuery.currentCache !== initialCache) { logDebug('Initial cache found. Comparing dates.'); diff --git a/packages/data-connect/src/network/fetch.ts b/packages/data-connect/src/network/fetch.ts index 3e8e2cab476..62cef6fb220 100644 --- a/packages/data-connect/src/network/fetch.ts +++ b/packages/data-connect/src/network/fetch.ts @@ -56,9 +56,9 @@ export function dcFetch( url: string, body: DataConnectFetchBody, { 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); diff --git a/packages/data-connect/src/network/transport/rest.ts b/packages/data-connect/src/network/transport/rest.ts index f16154dcb2a..4a3af8ac41a 100644 --- a/packages/data-connect/src/network/transport/rest.ts +++ b/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 { + async getWithAuth(forceToken = false): Promise { let starterPromise: Promise = new Promise(resolve => resolve(this._accessToken) ); diff --git a/packages/data-connect/src/util/validateArgs.ts b/packages/data-connect/src/util/validateArgs.ts index 15d1effa3da..e957786aa48 100644 --- a/packages/data-connect/src/util/validateArgs.ts +++ b/packages/data-connect/src/util/validateArgs.ts @@ -46,7 +46,7 @@ export function validateArgs( 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; diff --git a/packages/data-connect/test/unit/queries.test.ts b/packages/data-connect/test/unit/queries.test.ts index 68bd96268a6..02d19bf856e 100644 --- a/packages/data-connect/test/unit/queries.test.ts +++ b/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 { + const dc = initDatabase(); + return queryRef(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(); diff --git a/packages/data-connect/tsconfig.json b/packages/data-connect/tsconfig.json index 58561f50f5d..f97ac9fe899 100644 --- a/packages/data-connect/tsconfig.json +++ b/packages/data-connect/tsconfig.json @@ -2,7 +2,7 @@ "extends": "../../config/tsconfig.base.json", "compilerOptions": { "outDir": "dist", - "strict": false + "strict": true }, "exclude": ["dist/**/*", "test/**/*"] } diff --git a/scripts/emulator-testing/emulators/dataconnect-emulator.ts b/scripts/emulator-testing/emulators/dataconnect-emulator.ts index 9dc6add5df1..729889364d1 100644 --- a/scripts/emulator-testing/emulators/dataconnect-emulator.ts +++ b/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) {