From d8c5ad9dff06e42656504657fdd27e6a67b875e3 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 31 Aug 2023 11:43:16 -0500 Subject: [PATCH] Fix consent bugs (#941) --- .changeset/curvy-ravens-confess.md | 7 ++ .changeset/funny-brooms-smash.md | 6 + .../pages/index-consent.html | 106 ++++++++++++------ packages/consent/consent-tools/README.md | 4 + packages/consent/consent-tools/package.json | 8 ++ .../src/domain/consent-changed.ts | 17 ++- .../src/domain/create-wrapper.ts | 26 ++--- .../src/domain/get-initialized-analytics.ts | 25 +++++ packages/consent/consent-tools/src/index.ts | 1 + .../consent-tools/src/types/settings.ts | 11 +- .../consent-tools/src/types/wrapper.ts | 8 +- .../consent-wrapper-onetrust/README.md | 2 +- .../consent-wrapper-onetrust/package.json | 12 +- .../src/domain/__tests__/wrapper.test.ts | 12 +- .../src/domain/wrapper.ts | 27 +++-- yarn.lock | 10 ++ 16 files changed, 209 insertions(+), 73 deletions(-) create mode 100644 .changeset/curvy-ravens-confess.md create mode 100644 .changeset/funny-brooms-smash.md create mode 100644 packages/consent/consent-tools/src/domain/get-initialized-analytics.ts diff --git a/.changeset/curvy-ravens-confess.md b/.changeset/curvy-ravens-confess.md new file mode 100644 index 000000000..40d53e5aa --- /dev/null +++ b/.changeset/curvy-ravens-confess.md @@ -0,0 +1,7 @@ +--- +'@segment/analytics-consent-tools': patch +'@segment/analytics-consent-wrapper-onetrust': patch +--- + +- Fix `onConsentChanged` not firing in snippet environment due to to stale analytics reference. +- Register `onConsentChanged` early in the wrapper initialization sequence so it can catch consent changed events that occur before analytics is loaded. diff --git a/.changeset/funny-brooms-smash.md b/.changeset/funny-brooms-smash.md new file mode 100644 index 000000000..5fc39e52f --- /dev/null +++ b/.changeset/funny-brooms-smash.md @@ -0,0 +1,6 @@ +--- +'@segment/analytics-consent-tools': minor +'@segment/analytics-consent-wrapper-onetrust': minor +--- + +Tighten up analytics types diff --git a/examples/standalone-playground/pages/index-consent.html b/examples/standalone-playground/pages/index-consent.html index 2c908fc0e..c6a998fb6 100644 --- a/examples/standalone-playground/pages/index-consent.html +++ b/examples/standalone-playground/pages/index-consent.html @@ -1,34 +1,42 @@ - +
+ + + + + + + + - + + diff --git a/packages/consent/consent-tools/README.md b/packages/consent/consent-tools/README.md index 3461491fa..caea7561f 100644 --- a/packages/consent/consent-tools/README.md +++ b/packages/consent/consent-tools/README.md @@ -70,6 +70,10 @@ window.analytics.load('= **1.53.1**. Note: If your library depends on this library, you should have the appropriate peer dependency declaration. See our `package.json` for an example. + ## Development 1. Build this package + all dependencies diff --git a/packages/consent/consent-tools/package.json b/packages/consent/consent-tools/package.json index fb3048b20..7d8898c73 100644 --- a/packages/consent/consent-tools/package.json +++ b/packages/consent/consent-tools/package.json @@ -25,6 +25,14 @@ "concurrently": "yarn run -T concurrently --raw", "jest": "yarn run -T jest" }, + "peerDependencies": { + "@segment/analytics-next": ">=1.53.1" + }, + "peerDependenciesMeta": { + "@segment/analytics-next": { + "optional": true + } + }, "devDependencies": { "@internal/config": "workspace:^", "@internal/test-helpers": "workspace:^", diff --git a/packages/consent/consent-tools/src/domain/consent-changed.ts b/packages/consent/consent-tools/src/domain/consent-changed.ts index 5a67a707d..6e17738aa 100644 --- a/packages/consent/consent-tools/src/domain/consent-changed.ts +++ b/packages/consent/consent-tools/src/domain/consent-changed.ts @@ -1,4 +1,6 @@ import { AnyAnalytics, Categories } from '../types' +import { getInitializedAnalytics } from './get-initialized-analytics' +import { validateCategories } from './validation' /** * Dispatch an event that looks like: @@ -20,7 +22,7 @@ export const sendConsentChangedEvent = ( analytics: AnyAnalytics, categories: Categories ): void => { - analytics.track( + getInitializedAnalytics(analytics).track( CONSENT_CHANGED_EVENT, undefined, createConsentChangedCtxDto(categories) @@ -34,3 +36,16 @@ const createConsentChangedCtxDto = (categories: Categories) => ({ categoryPreferences: categories, }, }) + +export const validateAndSendConsentChangedEvent = ( + analytics: AnyAnalytics, + categories: Categories +) => { + try { + validateCategories(categories) + sendConsentChangedEvent(analytics, categories) + } catch (err) { + // Not sure if there's a better way to handle this, but this makes testing a bit easier. + console.error(err) + } +} diff --git a/packages/consent/consent-tools/src/domain/create-wrapper.ts b/packages/consent/consent-tools/src/domain/create-wrapper.ts index caa619a67..97a417fcc 100644 --- a/packages/consent/consent-tools/src/domain/create-wrapper.ts +++ b/packages/consent/consent-tools/src/domain/create-wrapper.ts @@ -15,9 +15,11 @@ import { createConsentStampingMiddleware } from './consent-stamping' import { pipe, pick, uniq } from '../utils' import { AbortLoadError, LoadContext } from './load-cancellation' import { ValidationError } from './validation/validation-error' -import { sendConsentChangedEvent } from './consent-changed' +import { validateAndSendConsentChangedEvent } from './consent-changed' -export const createWrapper: CreateWrapper = (createWrapperOptions) => { +export const createWrapper = ( + ...[createWrapperOptions]: Parameters> +): ReturnType> => { validateSettings(createWrapperOptions) const { @@ -31,8 +33,15 @@ export const createWrapper: CreateWrapper = (createWrapperOptions) => { registerOnConsentChanged, } = createWrapperOptions - return (analytics) => { + return (analytics: Analytics) => { validateAnalyticsInstance(analytics) + + // Call this function as early as possible. OnConsentChanged events can happen before .load is called. + registerOnConsentChanged?.((categories) => + // whenever consent changes, dispatch a new event with the latest consent information + validateAndSendConsentChangedEvent(analytics, categories) + ) + const ogLoad = analytics.load const loadWithConsent: AnyAnalytics['load'] = async ( @@ -125,17 +134,6 @@ export const createWrapper: CreateWrapper = (createWrapperOptions) => { createConsentStampingMiddleware(getValidCategoriesForConsentStamping) ) - // whenever consent changes, dispatch a new event with the latest consent information - registerOnConsentChanged?.((categories) => { - try { - validateCategories(categories) - sendConsentChangedEvent(analytics, categories) - } catch (err) { - // Not sure if there's a better way to handle this, but this makes testing a bit easier. - console.error(err) - } - }) - const updateCDNSettings: InitOptions['updateCDNSettings'] = ( cdnSettings ) => { diff --git a/packages/consent/consent-tools/src/domain/get-initialized-analytics.ts b/packages/consent/consent-tools/src/domain/get-initialized-analytics.ts new file mode 100644 index 000000000..0a445ed79 --- /dev/null +++ b/packages/consent/consent-tools/src/domain/get-initialized-analytics.ts @@ -0,0 +1,25 @@ +import { AnyAnalytics } from '../types' + +/** + * There is a known bug for people who attempt to to wrap the library: the analytics reference does not get updated when the analytics.js library loads. + * Thus, we need to proxy events to the global reference instead. + * + * There is a universal fix here: however, many users may not have updated it: + * https://github.com/segmentio/snippet/commit/081faba8abab0b2c3ec840b685c4ff6d6cccf79c + */ +export const getInitializedAnalytics = ( + analytics: AnyAnalytics +): AnyAnalytics => { + const isSnippetUser = Array.isArray(analytics) + if (isSnippetUser) { + const opts = (analytics as any)._loadOptions ?? {} + const globalAnalytics = (window as any)[ + opts?.globalAnalyticsKey ?? 'analytics' + ] + if ((globalAnalytics as any).initialized) { + return globalAnalytics + } + } + + return analytics +} diff --git a/packages/consent/consent-tools/src/index.ts b/packages/consent/consent-tools/src/index.ts index 640515d47..6019d8e91 100644 --- a/packages/consent/consent-tools/src/index.ts +++ b/packages/consent/consent-tools/src/index.ts @@ -11,5 +11,6 @@ export type { CreateWrapperSettings, IntegrationCategoryMappings, Categories, + RegisterOnConsentChangedFunction, AnyAnalytics, } from './types' diff --git a/packages/consent/consent-tools/src/types/settings.ts b/packages/consent/consent-tools/src/types/settings.ts index 9b535be1e..0d637a879 100644 --- a/packages/consent/consent-tools/src/types/settings.ts +++ b/packages/consent/consent-tools/src/types/settings.ts @@ -5,6 +5,10 @@ import type { CDNSettingsRemotePlugin, } from './wrapper' +export type RegisterOnConsentChangedFunction = ( + categoriesChangedCb: (categories: Categories) => void +) => void + /** * Consent wrapper function configuration */ @@ -33,7 +37,8 @@ export interface CreateWrapperSettings { * #### Note: The callback requires the categories to be in the shape of { "C0001": true, "C0002": false }, so some normalization may be needed. * @example * ```ts - * (categoriesChangedCb) => { + * async (categoriesChangedCb) => { + * await resolveWhen(() => window.MyCMP !== undefined, 500) * window.MyCMP.OnConsentChanged((event.detail) => categoriesChangedCb(normalizeCategories(event.detail)) * } * @@ -52,9 +57,7 @@ export interface CreateWrapperSettings { * .. * ``` */ - registerOnConsentChanged?: ( - categoriesChangedCb: (categories: Categories) => void - ) => void + registerOnConsentChanged?: RegisterOnConsentChangedFunction /** * This permanently disables any consent requirement (i.e device mode gating, event pref stamping). diff --git a/packages/consent/consent-tools/src/types/wrapper.ts b/packages/consent/consent-tools/src/types/wrapper.ts index f1186caa5..edb646026 100644 --- a/packages/consent/consent-tools/src/types/wrapper.ts +++ b/packages/consent/consent-tools/src/types/wrapper.ts @@ -22,7 +22,7 @@ export interface InitOptions { */ export interface AnyAnalytics { addSourceMiddleware(...args: any[]): any - on(event: 'initialize', callback: (settings: CDNSettings) => void): void + on(event: 'initialize', callback: (cdnSettings: CDNSettings) => void): void track(event: string, properties?: unknown, ...args: any[]): void /** @@ -41,14 +41,16 @@ export interface AnyAnalytics { **/ // Why type this as 'object' rather than 'AnyAnalytics'? IMO, the chance of a false positive is much higher than the chance that someone will pass in an object that is not an analytics instance. // We have an assertion function that throws an error if the analytics instance is not compatible. -export type Wrapper = ( +export type Wrapper = ( analyticsInstance: Analytics ) => Analytics /** * Create a function which wraps analytics instances to add consent management. */ -export type CreateWrapper = (settings: CreateWrapperSettings) => Wrapper +export type CreateWrapper = ( + settings: CreateWrapperSettings +) => Wrapper export interface Categories { [category: string]: boolean diff --git a/packages/consent/consent-wrapper-onetrust/README.md b/packages/consent/consent-wrapper-onetrust/README.md index 4f2bdecc5..cd31350b7 100644 --- a/packages/consent/consent-wrapper-onetrust/README.md +++ b/packages/consent/consent-wrapper-onetrust/README.md @@ -72,7 +72,7 @@ withOneTrust(analytics).load({ writeKey: ' }) > - +