♻️ [RUMF-1529] use an enum for experimental features (#2113)

* ♻️ [RUMF-1529] use an enum for experimental features

* 👌 rename updateExperimentalFeatures

* 👌 simplify experimentalFeatures implementation

Let's define the `enabledExperimentalFeature` during module evaluation.
This way, we don't need to worry about it being undefined.

Also, we are now validating `enabledExperimentalFeature` configuration
in `configuration.ts`, so move validation tests there.

* 👌 correctly reset experimentalFeatures

Author: BenoitZugmeyer

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

@@ -1,5 +1,5 @@
 import { createNewEvent, restorePageVisibility, setPageVisibility } from '../../test'
-import { resetExperimentalFeatures, updateExperimentalFeatures } from '../domain/configuration'
+import { resetExperimentalFeatures, addExperimentalFeatures, ExperimentalFeature } from '../domain/configuration'
 import type { Subscription } from '../tools/observable'
 import type { PageExitEvent } from './pageExitObservable'
 import { PageExitReason, createPageExitObservable } from './pageExitObservable'
@@ -20,7 +20,7 @@ describe('createPageExitObservable', () => {
   })
 
   it('notifies when the page fires pagehide if ff pagehide is enabled', () => {
-    updateExperimentalFeatures(['pagehide'])
+    addExperimentalFeatures([ExperimentalFeature.PAGEHIDE])
     onExitSpy = jasmine.createSpy()
     pageExitSubscription = createPageExitObservable().subscribe(onExitSpy)
 

packages/core/src/browser/pageExitObservable.ts

@@ -1,4 +1,4 @@
-import { isExperimentalFeatureEnabled } from '../domain/configuration'
+import { isExperimentalFeatureEnabled, ExperimentalFeature } from '../domain/configuration'
 import { Observable } from '../tools/observable'
 import { includes, noop, objectValues } from '../tools/utils'
 import { addEventListeners, addEventListener, DOM_EVENT } from './addEventListener'
@@ -18,7 +18,7 @@ export interface PageExitEvent {
 
 export function createPageExitObservable(): Observable<PageExitEvent> {
   const observable = new Observable<PageExitEvent>(() => {
-    const pagehideEnabled = isExperimentalFeatureEnabled('pagehide')
+    const pagehideEnabled = isExperimentalFeatureEnabled(ExperimentalFeature.PAGEHIDE)
     const { stop: stopListeners } = addEventListeners(
       window,
       [DOM_EVENT.VISIBILITY_CHANGE, DOM_EVENT.FREEZE, DOM_EVENT.PAGE_HIDE],

packages/core/src/domain/configuration/configuration.spec.ts

@@ -2,18 +2,41 @@ import type { RumEvent } from '../../../../rum-core/src'
 import { display } from '../../tools/display'
 import type { InitConfiguration } from './configuration'
 import { validateAndBuildConfiguration } from './configuration'
-import { isExperimentalFeatureEnabled, updateExperimentalFeatures } from './experimentalFeatures'
+import { ExperimentalFeature, isExperimentalFeatureEnabled, resetExperimentalFeatures } from './experimentalFeatures'
 
 describe('validateAndBuildConfiguration', () => {
   const clientToken = 'some_client_token'
 
-  beforeEach(() => {
-    updateExperimentalFeatures([])
+  afterEach(() => {
+    resetExperimentalFeatures()
   })
 
-  it('updates experimental feature flags', () => {
-    validateAndBuildConfiguration({ clientToken, enableExperimentalFeatures: ['foo'] })
-    expect(isExperimentalFeatureEnabled('foo')).toBeTrue()
+  describe('experimentalFeatures', () => {
+    const TEST_FEATURE_FLAG = 'foo' as ExperimentalFeature
+
+    beforeEach(() => {
+      ;(ExperimentalFeature as any).FOO = TEST_FEATURE_FLAG
+    })
+
+    afterEach(() => {
+      delete (ExperimentalFeature as any).FOO
+    })
+
+    it('updates experimental feature flags', () => {
+      validateAndBuildConfiguration({ clientToken, enableExperimentalFeatures: ['foo'] })
+      expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG)).toBeTrue()
+    })
+
+    it('ignores unknown experimental features', () => {
+      validateAndBuildConfiguration({
+        clientToken,
+        enableExperimentalFeatures: ['bar', undefined as any, null as any, 11 as any],
+      })
+      expect(isExperimentalFeatureEnabled('bar' as any)).toBeFalse()
+      expect(isExperimentalFeatureEnabled(undefined as any)).toBeFalse()
+      expect(isExperimentalFeatureEnabled(null as any)).toBeFalse()
+      expect(isExperimentalFeatureEnabled(11 as any)).toBeFalse()
+    })
   })
 
   describe('validate init configuration', () => {

packages/core/src/domain/configuration/configuration.ts

@@ -2,9 +2,9 @@ import type { CookieOptions } from '../../browser/cookie'
 import { getCurrentSite } from '../../browser/cookie'
 import { catchUserErrors } from '../../tools/catchUserErrors'
 import { display } from '../../tools/display'
-import { assign, isPercentage, ONE_KIBI_BYTE, ONE_SECOND } from '../../tools/utils'
+import { assign, isPercentage, objectHasValue, ONE_KIBI_BYTE, ONE_SECOND } from '../../tools/utils'
 import type { RawTelemetryConfiguration } from '../telemetry'
-import { updateExperimentalFeatures } from './experimentalFeatures'
+import { ExperimentalFeature, addExperimentalFeatures } from './experimentalFeatures'
 import type { TransportConfiguration } from './transportConfiguration'
 import { computeTransportConfiguration } from './transportConfiguration'
 
@@ -112,7 +112,13 @@ export function validateAndBuildConfiguration(initConfiguration: InitConfigurati
   }
 
   // Set the experimental feature flags as early as possible, so we can use them in most places
-  updateExperimentalFeatures(initConfiguration.enableExperimentalFeatures)
+  if (Array.isArray(initConfiguration.enableExperimentalFeatures)) {
+    addExperimentalFeatures(
+      initConfiguration.enableExperimentalFeatures.filter((flag): flag is ExperimentalFeature =>
+        objectHasValue(ExperimentalFeature, flag)
+      )
+    )
+  }
 
   return assign(
     {

packages/core/src/domain/configuration/endpointBuilder.spec.ts

@@ -2,7 +2,7 @@ import type { BuildEnvWindow } from '../../../test'
 import { startsWith } from '../../tools/utils'
 import type { InitConfiguration } from './configuration'
 import { createEndpointBuilder } from './endpointBuilder'
-import { resetExperimentalFeatures, updateExperimentalFeatures } from './experimentalFeatures'
+import { ExperimentalFeature, resetExperimentalFeatures, addExperimentalFeatures } from './experimentalFeatures'
 
 describe('endpointBuilder', () => {
   const clientToken = 'some_client_token'
@@ -126,7 +126,7 @@ describe('endpointBuilder', () => {
     })
 
     it('should contain flush reason when ff collect_flush_reason is enabled', () => {
-      updateExperimentalFeatures(['collect_flush_reason'])
+      addExperimentalFeatures([ExperimentalFeature.COLLECT_FLUSH_REASON])
       expect(createEndpointBuilder(initConfiguration, 'rum', []).build('xhr', 'batch_bytes_limit')).toContain(
         'flush_reason%3Abatch_bytes_limit'
       )

packages/core/src/domain/configuration/endpointBuilder.ts

@@ -4,7 +4,7 @@ import { normalizeUrl } from '../../tools/urlPolyfill'
 import { generateUUID } from '../../tools/utils'
 import type { InitConfiguration } from './configuration'
 import { INTAKE_SITE_AP1, INTAKE_SITE_US1 } from './intakeSites'
-import { isExperimentalFeatureEnabled } from './experimentalFeatures'
+import { ExperimentalFeature, isExperimentalFeatureEnabled } from './experimentalFeatures'
 
 // replaced at build time
 declare const __BUILD_ENV__SDK_VERSION__: string
@@ -104,7 +104,7 @@ function buildEndpointParameters(
   retry: RetryInfo | undefined
 ) {
   const tags = [`sdk_version:${__BUILD_ENV__SDK_VERSION__}`, `api:${api}`].concat(configurationTags)
-  if (flushReason && isExperimentalFeatureEnabled('collect_flush_reason')) {
+  if (flushReason && isExperimentalFeatureEnabled(ExperimentalFeature.COLLECT_FLUSH_REASON)) {
     tags.push(`flush_reason:${flushReason}`)
   }
   if (retry) {

packages/core/src/domain/configuration/experimentalFeatures.spec.ts

@@ -1,39 +1,34 @@
+import type { ExperimentalFeature } from './experimentalFeatures'
 import {
-  updateExperimentalFeatures,
+  addExperimentalFeatures,
   isExperimentalFeatureEnabled,
   resetExperimentalFeatures,
 } from './experimentalFeatures'
 
+const TEST_FEATURE_FLAG_ONE = 'foo' as ExperimentalFeature
+const TEST_FEATURE_FLAG_TWO = 'bar' as ExperimentalFeature
+
 describe('experimentalFeatures', () => {
   afterEach(() => {
     resetExperimentalFeatures()
   })
 
   it('initial state is empty', () => {
-    expect(isExperimentalFeatureEnabled('foo')).toBeFalse()
-    expect(isExperimentalFeatureEnabled('bar')).toBeFalse()
+    expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_ONE)).toBeFalse()
+    expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_TWO)).toBeFalse()
   })
 
   it('should define enabled experimental features', () => {
-    updateExperimentalFeatures(['foo'])
-    expect(isExperimentalFeatureEnabled('foo')).toBeTrue()
-    expect(isExperimentalFeatureEnabled('bar')).toBeFalse()
+    addExperimentalFeatures([TEST_FEATURE_FLAG_ONE])
+    expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_ONE)).toBeTrue()
+    expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_TWO)).toBeFalse()
   })
 
   it('should allow to be shared between products', () => {
-    updateExperimentalFeatures(['foo'])
-    updateExperimentalFeatures(['bar'])
-
-    expect(isExperimentalFeatureEnabled('foo')).toBeTrue()
-    expect(isExperimentalFeatureEnabled('bar')).toBeTrue()
-  })
-
-  it('should support some edge cases', () => {
-    updateExperimentalFeatures(['foo'])
-    updateExperimentalFeatures(undefined)
-    updateExperimentalFeatures([])
-    updateExperimentalFeatures([11 as any])
+    addExperimentalFeatures([TEST_FEATURE_FLAG_ONE])
+    addExperimentalFeatures([TEST_FEATURE_FLAG_TWO])
 
-    expect(isExperimentalFeatureEnabled('foo')).toBeTrue()
+    expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_ONE)).toBeTrue()
+    expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_TWO)).toBeTrue()
   })
 })

packages/core/src/domain/configuration/experimentalFeatures.ts

@@ -3,40 +3,40 @@
  * For NPM setup, this feature flag singleton is shared between RUM and Logs product.
  * This means that an experimental flag set on the RUM product will be set on the Logs product.
  * So keep in mind that in certain configurations, your experimental feature flag may affect other products.
+ *
+ * FORMAT:
+ * All feature flags should be snake_cased
  */
-import { includes } from '../../tools/utils'
-import { display } from '../../tools/display'
-
-let enabledExperimentalFeatures: Set<string> | undefined
-
-export function updateExperimentalFeatures(enabledFeatures: string[] | undefined): void {
-  // Safely handle external data
-  if (!Array.isArray(enabledFeatures)) {
-    return
-  }
+// We want to use a real enum (i.e. not a const enum) here, to be able to check whether an arbitrary
+// string is an expected feature flag
+// eslint-disable-next-line no-restricted-syntax
+export enum ExperimentalFeature {
+  PAGEHIDE = 'pagehide',
+  SHADOW_DOM_DEBUG = 'shadow_dom_debug',
+  FEATURE_FLAGS = 'feature_flags',
+  RESOURCE_DURATIONS = 'resource_durations',
+  RESOURCE_PAGE_STATES = 'resource_page_states',
+  CLICKMAP = 'clickmap',
+  COLLECT_FLUSH_REASON = 'collect_flush_reason',
+  SANITIZE_INPUTS = 'sanitize_inputs',
+}
 
-  if (!enabledExperimentalFeatures) {
-    enabledExperimentalFeatures = new Set(enabledFeatures)
-  }
+const enabledExperimentalFeatures: Set<ExperimentalFeature> = new Set()
 
-  enabledFeatures
-    .filter((flag) => typeof flag === 'string')
-    .forEach((flag: string) => {
-      if (includes(flag, '-')) {
-        display.warn(`please use snake case for '${flag}'`)
-      }
-      enabledExperimentalFeatures!.add(flag)
-    })
+export function addExperimentalFeatures(enabledFeatures: ExperimentalFeature[]): void {
+  enabledFeatures.forEach((flag) => {
+    enabledExperimentalFeatures.add(flag)
+  })
 }
 
-export function isExperimentalFeatureEnabled(featureName: string): boolean {
-  return !!enabledExperimentalFeatures && enabledExperimentalFeatures.has(featureName)
+export function isExperimentalFeatureEnabled(featureName: ExperimentalFeature): boolean {
+  return enabledExperimentalFeatures.has(featureName)
 }
 
 export function resetExperimentalFeatures(): void {
-  enabledExperimentalFeatures = new Set()
+  enabledExperimentalFeatures.clear()
 }
 
-export function getExperimentalFeatures(): Set<string> {
-  return enabledExperimentalFeatures || new Set()
+export function getExperimentalFeatures(): Set<ExperimentalFeature> {
+  return enabledExperimentalFeatures
 }

packages/core/src/domain/configuration/index.ts

@@ -9,8 +9,9 @@ export {
 export { createEndpointBuilder, EndpointBuilder, EndpointType } from './endpointBuilder'
 export {
   isExperimentalFeatureEnabled,
-  updateExperimentalFeatures,
+  addExperimentalFeatures,
   resetExperimentalFeatures,
   getExperimentalFeatures,
+  ExperimentalFeature,
 } from './experimentalFeatures'
 export * from './intakeSites'

packages/core/src/domain/console/consoleObservable.ts

@@ -5,7 +5,7 @@ import { find, jsonStringify } from '../../tools/utils'
 import { ConsoleApiName } from '../../tools/display'
 import { callMonitored } from '../../tools/monitor'
 import { sanitize } from '../../tools/sanitize'
-import { isExperimentalFeatureEnabled } from '../configuration'
+import { ExperimentalFeature, isExperimentalFeatureEnabled } from '../configuration'
 
 export interface ConsoleLog {
   message: string
@@ -70,10 +70,14 @@ function buildConsoleLog(params: unknown[], api: ConsoleApiName, handlingStack:
 
 function formatConsoleParameters(param: unknown) {
   if (typeof param === 'string') {
-    return isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(param) : param
+    return isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS) ? sanitize(param) : param
   }
   if (param instanceof Error) {
     return formatErrorMessage(computeStackTrace(param))
   }
-  return jsonStringify(isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(param) : param, undefined, 2)
+  return jsonStringify(
+    isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS) ? sanitize(param) : param,
+    undefined,
+    2
+  )
 }