Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(analytics): Type defs #8363

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(analytics): Type defs #8363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe19f81
Type defs fix
MichaelVerdon Feb 24, 2025
87cd81b
throw error not supported
MichaelVerdon Feb 24, 2025
81d411c
code format fix
MichaelVerdon Feb 24, 2025
7651c99
Test for function definition
MichaelVerdon Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/analytics/__tests__/analytics.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
firebase,
getAnalytics,
initializeAnalytics,
getGoogleAnalyticsClientId,
logEvent,
setAnalyticsCollectionEnabled,
setSessionTimeoutDuration,
Expand Down Expand Up @@ -711,6 +712,10 @@ describe('Analytics', function () {
expect(initializeAnalytics).toBeDefined();
});

it('`getGoogleAnalyticsClientId` function is properly exposed to end user', function () {
expect(getGoogleAnalyticsClientId).toBeDefined();
});

it('`logEvent` function is properly exposed to end user', function () {
expect(logEvent).toBeDefined();
});
Expand Down
18 changes: 15 additions & 3 deletions packages/analytics/lib/modular/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ export declare function initializeAnalytics(
options?: FirebaseAnalyticsTypes.AnalyticsSettings,
): Analytics;

/**
* Retrieves a unique Google Analytics identifier for the web client.
*
* @param analyticsInstance - Instance of analytics (web - only)
*
*/
export declare function getGoogleAnalyticsClientId(analyticsInstance: Analytics): Promise<string>;

/**
* Log a custom event with optional params. Note that there are various limits that applied
* to event parameters (total parameter count, etc), but analytics applies the limits during
Expand Down Expand Up @@ -605,10 +613,14 @@ export function getAppInstanceId(analytics: Analytics): Promise<string | null>;
* Gives a user a unique identification.
*
* @param analytics Analytics instance.
* @param id Set to null to remove a previously assigned ID from analytics
* events
* @param id Set to null to remove a previously assigned ID from analytics events
* @param options Additional options that can be passed to Analytics method calls such as logEvent, etc.
*/
export function setUserId(analytics: Analytics, id: string | null): Promise<void>;
export function setUserId(
analytics: Analytics,
id: string | null,
options?: AnalyticsCallOptions,
): Promise<void>;

/**
* Sets a key/value pair of data on the current user. Each Firebase project can have up to 25 uniquely named (case-sensitive) user properties.
Expand Down
10 changes: 10 additions & 0 deletions packages/analytics/lib/modular/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,16 @@
return getApp(app.name).analytics();
}

/**
* Retrieves a unique Google Analytics identifier for the web client.
*
* @param {FirebaseAnalytics} analytics - Instance of analytics (web - only)
* @returns {string}
*/
export function getGoogleAnalyticsClientId(analytics) {
throw new Error('getGoogleAnalyticsClientId is web-only and not yet supported.');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for web we have an implementation already (I'm aware because I was in there semi-recently - it works...)

react-native-firebase/packages/analytics/lib/web/api.js

Line 165 in 8f59579

async _getCid() {

if you haven't done it, running yarn tests:macos:pod:install && yarn tests:macos:build && yarn tests:macos:test-cover runs the "web" version, and if you put an it.only or describe.only on any part of the e2e suite it'll run just those so it's fast to test+iterate

shame not to expose it since that might be half the work that went into the "other" platform analytics!

return analytics;

Check warning on line 75 in packages/analytics/lib/modular/index.js

View check run for this annotation

Codecov / codecov/patch

packages/analytics/lib/modular/index.js#L73-L75 

Added lines #L73 - L75 were not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...but, @returns {string} 🤔 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was put there temporarily so the linter wouldn't get angry at me.

}
/**
* Log a custom event with optional params.
* @param {FirebaseAnalytics} analytics - Analytics instance.
Expand Down
Loading