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.

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

Add function setConsent() to set end user consent state for web apps in Firebase Analytics #6376

Merged
merged 13 commits into from
Jun 29, 2022

Conversation

dwyfrequency
Copy link
Contributor

Sets the applicable end user consent state for this web app across all gtag references once Firebase Analytics is initialized.

@changeset-bot
Copy link

changeset-bot bot commented Jun 22, 2022

🦋 Changeset detected

Latest commit: 0d0bd61

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/analytics Minor
@firebase/analytics-compat Patch
firebase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 22, 2022

Size Report 1

Affected Products

  • @firebase/analytics

    TypeBase (69e2ee0)Merge (7a0a633)Diff
    browser19.6 kB20.1 kB+509 B (+2.6%)
    esm524.2 kB24.7 kB+509 B (+2.1%)
    main25.4 kB26.0 kB+574 B (+2.3%)
    module19.6 kB20.1 kB+509 B (+2.6%)
  • @firebase/database

    TypeBase (69e2ee0)Merge (7a0a633)Diff
    browser249 kB248 kB-404 B (-0.2%)
    esm5277 kB276 kB-427 B (-0.2%)
    main282 kB282 kB-427 B (-0.2%)
    module249 kB248 kB-404 B (-0.2%)
  • @firebase/database-compat/standalone

    TypeBase (69e2ee0)Merge (7a0a633)Diff
    main371 kB371 kB-427 B (-0.1%)
  • @firebase/util

    TypeBase (69e2ee0)Merge (7a0a633)Diff
    browser20.7 kB21.0 kB+258 B (+1.2%)
    esm522.0 kB22.3 kB+302 B (+1.4%)
    main26.9 kB27.3 kB+404 B (+1.5%)
    module20.7 kB21.0 kB+258 B (+1.2%)
  • bundle

    TypeBase (69e2ee0)Merge (7a0a633)Diff
    analytics (logEvent)41.8 kB41.8 kB+47 B (+0.1%)
    database (Append to a list of data)145 kB145 kB-332 B (-0.2%)
    database (Filtering data)144 kB144 kB-332 B (-0.2%)
    database (Listen for child events)160 kB160 kB-332 B (-0.2%)
    database (Listen for value events + Detach listeners)160 kB160 kB-332 B (-0.2%)
    database (Listen for value events)160 kB160 kB-332 B (-0.2%)
    database (Read data once)157 kB156 kB-332 B (-0.2%)
    database (Save data as transactions)162 kB162 kB-332 B (-0.2%)
    database (Sort data)146 kB145 kB-332 B (-0.2%)
    database (Write data)144 kB144 kB-332 B (-0.2%)
  • firebase

    TypeBase (69e2ee0)Merge (7a0a633)Diff
    firebase-analytics-compat.js25.8 kB25.8 kB+38 B (+0.1%)
    firebase-analytics.js113 kB115 kB+1.60 kB (+1.4%)
    firebase-auth-react-native.js497 kB498 kB+1.04 kB (+0.2%)
    firebase-compat.js794 kB794 kB-268 B (-0.0%)
    firebase-database-compat.js166 kB166 kB-306 B (-0.2%)
    firebase-database.js607 kB606 kB-692 B (-0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/2vH6Yo2oC8.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 22, 2022

Size Analysis Report 1

This report is too large (347,240 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/T975b50ie2.html

@google-oss-bot google-oss-bot added the doc-changes PRs that affect docs label Jun 22, 2022
packages/analytics/src/helpers.ts Outdated Show resolved Hide resolved
/* eslint-enable camelcase */

/** Whether a particular consent type has been granted or denied. */
Copy link
Contributor

Choose a reason for hiding this comment

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

You can ask @egilmorez for a more definitive opinion but I guess maybe "should be" instead of "has been" since in this context we can only set it, not read the current state. Also needs a @public tag.

packages/analytics/src/public-types.ts Show resolved Hide resolved
packages/analytics/src/functions.ts Show resolved Hide resolved
packages/analytics/src/api.ts Outdated Show resolved Hide resolved
@@ -288,4 +288,14 @@ export interface EventParams {
page_path?: string;
[key: string]: unknown;
}

/** Maps the applicable end user consent state. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest: "Consent status settings for each consent type." or similar, @egilmorez may have suggestions. Also add @public tag.

packages/analytics/src/public-types.ts Show resolved Hide resolved
@dwyfrequency dwyfrequency changed the title Add initial draft of setConsent logic Add function setConsent() to set end user consent state for web apps in Firebase Analytics Jun 27, 2022
@dwyfrequency dwyfrequency marked this pull request as ready for review June 27, 2022 18:49
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Doc strings LGTM, thanks!

packages/analytics/src/helpers.test.ts Show resolved Hide resolved
ad_storage?: ConsentStatusString;
// Enables storage, such as cookies, related to analytics (for example, visit duration)
analytics_storage?: ConsentStatusString;
// Enables storage that supports the functionality of the website or app such as language settings
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the generated analytics.api.md, it looks like api-documenter still thinks these are undocumented. I think api-documenter may need comments to be in the /** */ format.

* Use the {@link ConsentSettings} to specify individual consent type values. By default consent
* types are set to "granted".
* @public
* @param consentSettings Maps the applicable end user consent state for gtag.js.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I just ran build on this branch and noticed warnings from api-documenter that consentSettings needs to be followed by a hyphen here, and also customParams on line 236, which I think was from a previous PR.

Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

LGTM, just address the param tsdoc comment before merging.

@dwyfrequency dwyfrequency merged commit 1d3a34d into master Jun 29, 2022
@dwyfrequency dwyfrequency deleted the setConsent branch June 29, 2022 17:35
@google-oss-bot google-oss-bot mentioned this pull request Jul 6, 2022
@google-oss-bot google-oss-bot mentioned this pull request Jul 7, 2022
@firebase firebase locked and limited conversation to collaborators Jul 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
doc-changes PRs that affect docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants