Integrate benoit/restore-cookie-store-usage (#3561) into staging-22

Integrated commit sha: fab7030

Co-authored-by: BenoitZugmeyer <[email protected]>

Author: dd-mergequeue[bot]

packages/core/src/browser/addEventListener.spec.ts

@@ -76,7 +76,7 @@ describe('addEventListener', () => {
     const customEventTarget = {
       addEventListener: jasmine.createSpy(),
       removeEventListener: jasmine.createSpy(),
-    } as unknown as EventTarget
+    } as unknown as HTMLElement
 
     const { stop } = addEventListener({ allowUntrustedEvents: false }, customEventTarget, 'change', listener)
     stop()

packages/core/src/browser/browser.types.ts

@@ -53,8 +53,9 @@ export interface WeakRefConstructor {
 
 // Those are native API types that are not official supported by TypeScript yet
 
-// eslint-disable-next-line @typescript-eslint/no-empty-object-type
-export interface CookieStore extends EventTarget {}
+export interface CookieStore extends EventTarget {
+  get(name: string): Promise<unknown>
+}
 
 export interface CookieStoreEventMap {
   change: CookieChangeEvent

packages/rum-core/src/boot/startRum.ts

@@ -266,7 +266,7 @@ export function startRumEventCollection(
   )
 
   const displayContext = startDisplayContext(hooks, configuration)
-  const ciVisibilityContext = startCiVisibilityContext(hooks)
+  const ciVisibilityContext = startCiVisibilityContext(configuration, hooks)
   startSyntheticsContext(hooks)
 
   startRumAssembly(configuration, lifeCycle, hooks, reportError)

packages/rum-core/src/browser/cookieObservable.spec.ts

@@ -2,6 +2,8 @@ import type { Subscription } from '@datadog/browser-core'
 import { ONE_MINUTE, deleteCookie, setCookie } from '@datadog/browser-core'
 import type { Clock } from '@datadog/browser-core/test'
 import { mockClock } from '@datadog/browser-core/test'
+import { mockRumConfiguration } from '../../test'
+import type { CookieStoreWindow } from './cookieObservable'
 import { WATCH_COOKIE_INTERVAL_DELAY, createCookieObservable } from './cookieObservable'
 
 const COOKIE_NAME = 'cookie_name'
@@ -25,20 +27,47 @@ describe('cookieObservable', () => {
     clock.cleanup()
   })
 
-  it('should notify observers on cookie change', (done) => {
-    const observable = createCookieObservable(COOKIE_NAME)
+  it('should notify observers on cookie change', async () => {
+    const observable = createCookieObservable(mockRumConfiguration(), COOKIE_NAME)
 
-    subscription = observable.subscribe((cookieChange) => {
-      expect(cookieChange).toEqual('foo')
-
-      done()
+    const cookieChangePromise = new Promise((resolve) => {
+      subscription = observable.subscribe(resolve)
     })
+
+    // When writing a cookie just after subscribing to the cookieStore 'change' event, the 'change'
+    // event is sometimes not triggered, making this test case flaky.
+    // To work around this, we get some random cookie from the cookieStore. This adds enough delay
+    // to ensure that the 'change' event is triggered when we write our cookie.
+    // This was reported here: https://issues.chromium.org/issues/420405275
+    const cookieStore = (window as CookieStoreWindow).cookieStore
+    if (cookieStore) {
+      // Wait for the cookieStore to be ready
+      await cookieStore.get('some_cookie_name')
+    }
+
+    setCookie(COOKIE_NAME, 'foo', COOKIE_DURATION)
+    clock.tick(WATCH_COOKIE_INTERVAL_DELAY)
+
+    const cookieChange = await cookieChangePromise
+    expect(cookieChange).toEqual('foo')
+  })
+
+  it('should notify observers on cookie change when cookieStore is not supported', () => {
+    Object.defineProperty(window, 'cookieStore', { get: () => undefined, configurable: true })
+    const observable = createCookieObservable(mockRumConfiguration(), COOKIE_NAME)
+
+    let cookieChange: string | undefined
+    subscription = observable.subscribe((change) => (cookieChange = change))
+
     setCookie(COOKIE_NAME, 'foo', COOKIE_DURATION)
     clock.tick(WATCH_COOKIE_INTERVAL_DELAY)
+
+    expect(cookieChange).toEqual('foo')
   })
 
-  it('should not notify observers on cookie change when the cookie value has not changed', () => {
-    const observable = createCookieObservable(COOKIE_NAME)
+  it('should not notify observers on cookie change when the cookie value as not changed when cookieStore is not supported', () => {
+    Object.defineProperty(window, 'cookieStore', { get: () => undefined, configurable: true })
+    const observable = createCookieObservable(mockRumConfiguration(), COOKIE_NAME)
 
     setCookie(COOKIE_NAME, 'foo', COOKIE_DURATION)
 

packages/rum-core/src/browser/cookieObservable.ts

@@ -1,23 +1,54 @@
-import type { CookieStore } from '@datadog/browser-core'
-import { setInterval, clearInterval, Observable, ONE_SECOND, findCommaSeparatedValue } from '@datadog/browser-core'
+import type { Configuration, CookieStore } from '@datadog/browser-core'
+import {
+  setInterval,
+  clearInterval,
+  Observable,
+  addEventListener,
+  ONE_SECOND,
+  findCommaSeparatedValue,
+  DOM_EVENT,
+} from '@datadog/browser-core'
 
 export interface CookieStoreWindow extends Window {
   cookieStore?: CookieStore
 }
 
 export type CookieObservable = ReturnType<typeof createCookieObservable>
 
-export const WATCH_COOKIE_INTERVAL_DELAY = ONE_SECOND
+export function createCookieObservable(configuration: Configuration, cookieName: string) {
+  const detectCookieChangeStrategy = (window as CookieStoreWindow).cookieStore
+    ? listenToCookieStoreChange(configuration)
+    : watchCookieFallback
 
-export function createCookieObservable(cookieName: string) {
   return new Observable<string | undefined>((observable) =>
-    // NOTE: we don't use cookiestore.addEventListner('change', handler) as it seems to me more prone to bugs
-    // and cause our tests to be flaky (and probably in production as well)
-    watchCookieStrategy(cookieName, (event) => observable.notify(event))
+    detectCookieChangeStrategy(cookieName, (event) => observable.notify(event))
   )
 }
 
-function watchCookieStrategy(cookieName: string, callback: (event: string | undefined) => void) {
+function listenToCookieStoreChange(configuration: Configuration) {
+  return (cookieName: string, callback: (event: string | undefined) => void) => {
+    const listener = addEventListener(
+      configuration,
+      (window as CookieStoreWindow).cookieStore!,
+      DOM_EVENT.CHANGE,
+      (event) => {
+        // Based on our experimentation, we're assuming that entries for the same cookie cannot be in both the 'changed' and 'deleted' arrays.
+        // However, due to ambiguity in the specification, we asked for clarification: https://github.com/WICG/cookie-store/issues/226
+        const changeEvent =
+          event.changed.find((event) => event.name === cookieName) ||
+          event.deleted.find((event) => event.name === cookieName)
+        if (changeEvent) {
+          callback(changeEvent.value)
+        }
+      }
+    )
+    return listener.stop
+  }
+}
+
+export const WATCH_COOKIE_INTERVAL_DELAY = ONE_SECOND
+
+function watchCookieFallback(cookieName: string, callback: (event: string | undefined) => void) {
   const previousCookieValue = findCommaSeparatedValue(document.cookie, cookieName)
   const watchCookieIntervalId = setInterval(() => {
     const cookieValue = findCommaSeparatedValue(document.cookie, cookieName)
@@ -26,5 +57,7 @@ function watchCookieStrategy(cookieName: string, callback: (event: string | unde
     }
   }, WATCH_COOKIE_INTERVAL_DELAY)
 
-  return () => clearInterval(watchCookieIntervalId)
+  return () => {
+    clearInterval(watchCookieIntervalId)
+  }
 }

packages/rum-core/src/domain/contexts/ciVisibilityContext.spec.ts

@@ -1,4 +1,4 @@
-import type { RelativeTime } from '@datadog/browser-core'
+import type { Configuration, RelativeTime } from '@datadog/browser-core'
 import { HookNames, Observable } from '@datadog/browser-core'
 import { mockCiVisibilityValues } from '../../../test'
 import type { CookieObservable } from '../../browser/cookieObservable'
@@ -24,7 +24,7 @@ describe('startCiVisibilityContext', () => {
   describe('assemble hook', () => {
     it('should set ci visibility context defined by Cypress global variables', () => {
       mockCiVisibilityValues('trace_id_value')
-      ;({ stop: stopCiVisibility } = startCiVisibilityContext(hooks, cookieObservable))
+      ;({ stop: stopCiVisibility } = startCiVisibilityContext({} as Configuration, hooks, cookieObservable))
 
       const defaultRumEventAttributes = hooks.triggerHook(HookNames.Assemble, {
         eventType: 'view',
@@ -44,7 +44,7 @@ describe('startCiVisibilityContext', () => {
 
     it('should add the ci visibility context defined by global cookie', () => {
       mockCiVisibilityValues('trace_id_value', 'cookies')
-      ;({ stop: stopCiVisibility } = startCiVisibilityContext(hooks, cookieObservable))
+      ;({ stop: stopCiVisibility } = startCiVisibilityContext({} as Configuration, hooks, cookieObservable))
 
       const defaultRumEventAttributes = hooks.triggerHook(HookNames.Assemble, {
         eventType: 'view',
@@ -64,7 +64,7 @@ describe('startCiVisibilityContext', () => {
 
     it('should update the ci visibility context when global cookie is updated', () => {
       mockCiVisibilityValues('trace_id_value', 'cookies')
-      ;({ stop: stopCiVisibility } = startCiVisibilityContext(hooks, cookieObservable))
+      ;({ stop: stopCiVisibility } = startCiVisibilityContext({} as Configuration, hooks, cookieObservable))
       cookieObservable.notify('trace_id_value_updated')
 
       const defaultRumEventAttributes = hooks.triggerHook(HookNames.Assemble, {
@@ -85,7 +85,7 @@ describe('startCiVisibilityContext', () => {
 
     it('should not set ci visibility context if the Cypress global variable is undefined', () => {
       mockCiVisibilityValues(undefined)
-      ;({ stop: stopCiVisibility } = startCiVisibilityContext(hooks, cookieObservable))
+      ;({ stop: stopCiVisibility } = startCiVisibilityContext({} as Configuration, hooks, cookieObservable))
 
       const defaultRumEventAttributes = hooks.triggerHook(HookNames.Assemble, {
         eventType: 'view',
@@ -97,7 +97,7 @@ describe('startCiVisibilityContext', () => {
 
     it('should not set ci visibility context if it is not a string', () => {
       mockCiVisibilityValues({ key: 'value' })
-      ;({ stop: stopCiVisibility } = startCiVisibilityContext(hooks, cookieObservable))
+      ;({ stop: stopCiVisibility } = startCiVisibilityContext({} as Configuration, hooks, cookieObservable))
 
       const defaultRumEventAttributes = hooks.triggerHook(HookNames.Assemble, {
         eventType: 'view',

packages/rum-core/src/domain/contexts/ciVisibilityContext.ts

@@ -1,4 +1,5 @@
 import { getInitCookie, HookNames, SKIPPED } from '@datadog/browser-core'
+import type { Configuration } from '@datadog/browser-core'
 import { createCookieObservable } from '../../browser/cookieObservable'
 import { SessionType } from '../rumSessionManager'
 import type { DefaultRumEventAttributes, Hooks } from '../hooks'
@@ -14,8 +15,9 @@ export interface CiTestWindow extends Window {
 export type CiVisibilityContext = ReturnType<typeof startCiVisibilityContext>
 
 export function startCiVisibilityContext(
+  configuration: Configuration,
   hooks: Hooks,
-  cookieObservable = createCookieObservable(CI_VISIBILITY_TEST_ID_COOKIE_NAME)
+  cookieObservable = createCookieObservable(configuration, CI_VISIBILITY_TEST_ID_COOKIE_NAME)
 ) {
   let testExecutionId =
     getInitCookie(CI_VISIBILITY_TEST_ID_COOKIE_NAME) || (window as CiTestWindow).Cypress?.env('traceId')