Address PR comments

Author: hsubox76

common/api-review/analytics-exp.api.md

@@ -11,16 +11,19 @@ import { DynamicConfig } from '@firebase/analytics-types-exp';
 import { EventNameString } from '@firebase/analytics-types-exp';
 import { EventParams } from '@firebase/analytics-types-exp';
 import { FirebaseApp } from '@firebase/app-types-exp';
-import { FirebaseInstallations } from '@firebase/installations-types-exp';
+import { _FirebaseInstallationsInternal } from '@firebase/installations-types-exp';
 import { _FirebaseService } from '@firebase/app-types-exp';
 import { MinimalDynamicConfig } from '@firebase/analytics-types-exp';
 import { SettingsOptions } from '@firebase/analytics-types-exp';
 
+// @public
+export function analyticsSettings(options: SettingsOptions): void;
+
 // Warning: (ae-forgotten-export) The symbol "AnalyticsService" needs to be exported by the entry point index.d.ts
 // Warning: (ae-internal-missing-underscore) The name "factory" should be prefixed with an underscore because the declaration is marked as @internal
 //
 // @internal
-export function factory(app: FirebaseApp, installations: FirebaseInstallations): AnalyticsService;
+export function factory(app: FirebaseApp, installations: _FirebaseInstallationsInternal): AnalyticsService;
 
 // @public
 export function getAnalytics(app: FirebaseApp): Analytics;
@@ -233,9 +236,6 @@ export function setAnalyticsCollectionEnabled(analyticsInstance: Analytics, enab
 // @public
 export function setCurrentScreen(analyticsInstance: Analytics, screenName: string, options?: AnalyticsCallOptions): void;
 
-// @public
-export function settings(options: SettingsOptions): void;
-
 // @public
 export function setUserId(analyticsInstance: Analytics, id: string, options?: AnalyticsCallOptions): void;
 

packages-exp/analytics-exp/package.json

@@ -55,7 +55,7 @@
   "bugs": {
     "url": "https://github.com/firebase/firebase-js-sdk/issues"
   },
-  "typings": "dist/index.d.ts",
+  "typings": "dist/analytics-exp-public.d.ts",
   "nyc": {
     "extension": [
       ".ts"

packages-exp/analytics-exp/src/api.ts

@@ -48,7 +48,7 @@ import {
   setAnalyticsCollectionEnabled as internalSetAnalyticsCollectionEnabled
 } from './functions';
 
-export { settings } from './factory';
+export { analyticsSettings } from './factory';
 
 declare module '@firebase/component' {
   interface NameServiceMapping {

packages-exp/analytics-exp/src/constants.ts

@@ -41,7 +41,7 @@ export enum GtagCommand {
  * Officially recommended event names for gtag.js
  * Any other string is also allowed.
  */
-export enum EventName {
+export const enum EventName {
   ADD_SHIPPING_INFO = 'add_shipping_info',
   ADD_PAYMENT_INFO = 'add_payment_info',
   ADD_TO_CART = 'add_to_cart',

packages-exp/analytics-exp/src/factory.ts

@@ -29,9 +29,9 @@ import {
   findGtagScriptOnPage
 } from './helpers';
 import { AnalyticsError, ERROR_FACTORY } from './errors';
-import { FirebaseInstallations } from '@firebase/installations-types-exp';
+import { _FirebaseInstallationsInternal } from '@firebase/installations-types-exp';
 import { areCookiesEnabled, isBrowserExtension } from '@firebase/util';
-import { initializeIds } from './initialize-ids';
+import { initializeAnalytics } from './initialize-ids';
 import { logger } from './logger';
 import { FirebaseApp, _FirebaseService } from '@firebase/app-types-exp';
 
@@ -137,14 +137,14 @@ export function getGlobalVars(): {
  * Intended to be used if `gtag.js` script has been installed on
  * this page independently of Firebase Analytics, and is using non-default
  * names for either the `gtag` function or for `dataLayer`.
- * Must be called before calling `firebase.analytics()` or it won't
+ * Must be called before calling `getAnalytics()` or it won't
  * have any effect.
  *
  * @public
  *
  * @param options - Custom gtag and dataLayer names.
  */
-export function settings(options: SettingsOptions): void {
+export function analyticsSettings(options: SettingsOptions): void {
   if (globalInitDone) {
     throw ERROR_FACTORY.create(AnalyticsError.ALREADY_INITIALIZED);
   }
@@ -186,7 +186,7 @@ function warnOnBrowserContextMismatch(): void {
  */
 export function factory(
   app: FirebaseApp,
-  installations: FirebaseInstallations
+  installations: _FirebaseInstallationsInternal
 ): AnalyticsService {
   warnOnBrowserContextMismatch();
   const appId = app.options.appId;
@@ -234,7 +234,7 @@ export function factory(
   }
   // Async but non-blocking.
   // This map reflects the completion state of all promises for each appId.
-  initializationPromisesMap[appId] = initializeIds(
+  initializationPromisesMap[appId] = initializeAnalytics(
     app,
     dynamicConfigPromisesList,
     measurementIdToAppId,

packages-exp/analytics-exp/src/helpers.test.ts

@@ -23,7 +23,8 @@ import {
   getOrCreateDataLayer,
   insertScriptTag,
   wrapOrCreateGtag,
-  findGtagScriptOnPage
+  findGtagScriptOnPage,
+  promiseAllSettled
 } from './helpers';
 import { GtagCommand } from './constants';
 import { Deferred } from '@firebase/util';
@@ -99,7 +100,7 @@ describe('Gtag wrapping functions', () => {
 
       initPromise2.resolve('other-measurement-id'); // Resolves second initialization promise.
       await Promise.all([initPromise1, initPromise2]); // Wait for resolution of Promise.all()
-      await Promise.all(fakeDynamicConfigPromises);
+      await promiseAllSettled(fakeDynamicConfigPromises);
 
       expect((window['dataLayer'] as DataLayer).length).to.equal(1);
     });
@@ -132,7 +133,7 @@ describe('Gtag wrapping functions', () => {
 
         initPromise2.resolve(); // Resolves second initialization promise.
         await Promise.all([initPromise1, initPromise2]); // Wait for resolution of Promise.all()
-        await Promise.all(fakeDynamicConfigPromises);
+        await promiseAllSettled(fakeDynamicConfigPromises);
 
         expect((window['dataLayer'] as DataLayer).length).to.equal(1);
       }
@@ -194,7 +195,7 @@ describe('Gtag wrapping functions', () => {
         expect((window['dataLayer'] as DataLayer).length).to.equal(0);
 
         initPromise1.resolve(); // Resolves first initialization promise.
-        await Promise.all(fakeDynamicConfigPromises);
+        await promiseAllSettled(fakeDynamicConfigPromises);
         await Promise.all([initPromise1]); // Wait for resolution of Promise.all()
 
         expect((window['dataLayer'] as DataLayer).length).to.equal(1);
@@ -240,7 +241,7 @@ describe('Gtag wrapping functions', () => {
       expect((window['dataLayer'] as DataLayer).length).to.equal(0);
 
       initPromise1.resolve(fakeMeasurementId);
-      await Promise.all(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
+      await promiseAllSettled(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
       expect((window['dataLayer'] as DataLayer).length).to.equal(0);
 
       await Promise.all([initPromise1]); // Wait for resolution of Promise.all()
@@ -254,7 +255,7 @@ describe('Gtag wrapping functions', () => {
       (window['gtag'] as Gtag)(GtagCommand.CONFIG, fakeMeasurementId, {
         'transaction_id': 'abcd123'
       });
-      await Promise.all(fakeDynamicConfigPromises);
+      await promiseAllSettled(fakeDynamicConfigPromises);
       await Promise.resolve(); // Config call is always chained onto initialization promise list, even if empty.
       expect((window['dataLayer'] as DataLayer).length).to.equal(1);
     });
@@ -294,7 +295,7 @@ describe('Gtag wrapping functions', () => {
       expect(existingGtagStub).to.not.be.called;
 
       initPromise2.resolve(); // Resolves second initialization promise.
-      await Promise.all(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
+      await promiseAllSettled(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
       expect(existingGtagStub).to.not.be.called;
 
       await Promise.all([initPromise1, initPromise2]); // Wait for resolution of Promise.all()
@@ -331,7 +332,7 @@ describe('Gtag wrapping functions', () => {
         expect(existingGtagStub).to.not.be.called;
 
         initPromise2.resolve(); // Resolves second initialization promise.
-        await Promise.all(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
+        await promiseAllSettled(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
         expect(existingGtagStub).to.not.be.called;
 
         await Promise.all([initPromise1, initPromise2]); // Wait for resolution of Promise.all()
@@ -406,7 +407,7 @@ describe('Gtag wrapping functions', () => {
         expect(existingGtagStub).to.not.be.called;
 
         initPromise1.resolve(); // Resolves first initialization promise.
-        await Promise.all(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
+        await promiseAllSettled(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
         expect(existingGtagStub).to.not.be.called;
 
         await Promise.all([initPromise1]); // Wait for resolution of Promise.all()
@@ -462,7 +463,7 @@ describe('Gtag wrapping functions', () => {
       expect(existingGtagStub).to.not.be.called;
 
       initPromise1.resolve(fakeMeasurementId);
-      await Promise.all(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
+      await promiseAllSettled(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
       expect(existingGtagStub).to.not.be.called;
 
       await Promise.all([initPromise1]); // Wait for resolution of Promise.all()
@@ -482,7 +483,7 @@ describe('Gtag wrapping functions', () => {
       (window['gtag'] as Gtag)(GtagCommand.CONFIG, fakeMeasurementId, {
         'transaction_id': 'abcd123'
       });
-      await Promise.all(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
+      await promiseAllSettled(fakeDynamicConfigPromises); // Resolves dynamic config fetches.
       expect(existingGtagStub).to.not.be.called;
       await Promise.resolve(); // Config call is always chained onto initialization promise list, even if empty.
       expect(existingGtagStub).to.be.calledWith(

packages-exp/analytics-exp/src/helpers.ts

@@ -27,6 +27,17 @@ import {
 import { GtagCommand, GTAG_URL } from './constants';
 import { logger } from './logger';
 
+/**
+ * Makeshift polyfill for Promise.allSettled()
+ *
+ * @param promises Array of promises to wait for.
+ */
+export function promiseAllSettled<T>(
+  promises: Array<Promise<T>>
+): Promise<T[]> {
+  return Promise.all(promises.map(promise => promise.catch(e => e)));
+}
+
 /**
  * Inserts gtag script tag into the page to asynchronously download gtag.
  * @param dataLayerName Name of datalayer (most often the default, "_dataLayer").
@@ -86,7 +97,9 @@ async function gtagOnConfig(
       // find the appId (if any) corresponding to this measurementId. If there is one, wait on
       // that appId's initialization promise. If there is none, promise resolves and gtag
       // call goes through.
-      const dynamicConfigResults = await Promise.all(dynamicConfigPromisesList);
+      const dynamicConfigResults = await promiseAllSettled(
+        dynamicConfigPromisesList
+      );
       const foundConfig = dynamicConfigResults.find(
         config => config.measurementId === measurementId
       );
@@ -131,7 +144,9 @@ async function gtagOnEvent(
       }
       // Checking 'send_to' fields requires having all measurement ID results back from
       // the dynamic config fetch.
-      const dynamicConfigResults = await Promise.all(dynamicConfigPromisesList);
+      const dynamicConfigResults = await promiseAllSettled(
+        dynamicConfigPromisesList
+      );
       for (const sendToId of gaSendToList) {
         // Any fetched dynamic measurement ID that matches this 'send_to' ID
         const foundConfig = dynamicConfigResults.find(