Create modularized Analytics package. #4116
Conversation
|
Changeset File Check ✅
|
| "bugs": { | ||
| "url": "https://github.com/firebase/firebase-js-sdk/issues" | ||
| }, | ||
| "typings": "dist/index.d.ts", |
There was a problem hiding this comment.
It's not pointing to the rollup d.ts file created by api-extractor.
| * | ||
| * @param options - Custom gtag and dataLayer names. | ||
| */ | ||
| export function settings(options: SettingsOptions): void { |
There was a problem hiding this comment.
Is the name approved in the API proposal? Feels a little weird for a standalone function( it sounds like a property name).
There was a problem hiding this comment.
I think I neglected to include it in the API proposal. I changed it to analyticsSettings. Should I amend the API proposal?
There was a problem hiding this comment.
I actually prefer settings to analyticsSettings since everything is already namespaced by the package name by default. I just thought a name with "verb + thing" pattern like updateSettings() feels more natural for a free function.
Yes, we should amend the API proposal. Let's start with settings(). We should be able to do everything via emails.
| * Officially recommended event names for gtag.js | ||
| * Any other string is also allowed. | ||
| */ | ||
| export enum EventName { |
There was a problem hiding this comment.
Consider changing it to const enum to produce smaller bundle.
| // 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); |
There was a problem hiding this comment.
We should use Promise.allSettled() instead of Promise.all(), because we should proceed to try to find a match even if some promises rejected.
Promise.allSettled() is a pretty new addition to the es spec, so we need to use a polyfill. We can create one ourselves:
const a = Promise.resolve(1);
const b = Promise.reject(new Error(2));
const c = Promise.resolve(3);
// allSettled polyfill code
Promise.all([a, b, c].map(p => p.catch(e => e)))
.then(results => console.log(results)) // 1,Error: 2,3
There was a problem hiding this comment.
I made a polyfill function. It adds an extra tick to the async stack so this same function has to be used in tests (see helpers.test.ts).
| const fidPromise: Promise<string | undefined> = validateIndexedDB().then( | ||
| envIsValid => { | ||
| if (envIsValid) { | ||
| return getId(installations); |
There was a problem hiding this comment.
The installations here is an internal installations instance where you can do installations.getId(). We never import/use functions from other Firebase SDKs directly. We rely entirely on the component framework for inter-SDK communication.
There was a problem hiding this comment.
Ah, I was confused about how it works.
|
|
||
| export const GTAG_URL = 'https://www.googletagmanager.com/gtag/js'; | ||
|
|
||
| export enum GtagCommand { |
| * Officially recommended event names for gtag.js | ||
| * Any other string is also allowed. | ||
| */ | ||
| export const enum EventName { |
There was a problem hiding this comment.
This seems to be only used in tests? Should it be moved to a test file instead of in the source file?
There was a problem hiding this comment.
I think we wanted to make the enum available to users so they could easily get a list of official EventNames that they can then put into logEvent. The string literals are already available as autocompletes because of the overloads but maybe they want to save it to a variable? Feel safer from typos?
It wasn't exported so I added it to exports from api.ts.
Basically, if you import the EventName enum you can do:
logEvent(analytics, EventName.LOGIN, { method: 'email' });
or
logEvent(analytics, 'login', { method: 'email' });
Also it gets the list into documentation with some deprecation comments on 2 of them?
There was a problem hiding this comment.
If we want to export it, it has to be a normal enum instead of const enum because const enum only exists in compile time. Is it part of the API proposal? I'm okay exporting it since it's useful, but I don't think enums are tree-shakable, so it will be included in bundle even if not used.
There was a problem hiding this comment.
I think we should keep it especially as it's available in the current API. I'll add it to the API proposal and send an email.
hsubox76
force-pushed
the
ch-analytics-exp
branch
2 times, most recently
from
03d2b0f to
ac9e469
Compare
Size Analysis ReportAffected Products
|
Created "analytics-exp" modularized analytics package.
One question:
logEventhas a lot of signature overloads in the public interface (convenient recommended param autocompletes for all the standard eventNames). I haven't added them here yet (they can be seen in the old packages/firebase/index.d.ts monolith). Is there a good place for me to put them that doesn't clutter up the code?