Merge pull request #324 from DataDog/watson/DEBUG-5291/fix-enable

Centralize `enable` option handling in the factory

Co-authored-by: watson <thomas.watson@datadoghq.com>

Author: gh-worker-dd-mergequeue-cf854d[bot]

packages/core/src/helpers/options.test.ts

@@ -0,0 +1,102 @@
+// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2019-Present Datadog, Inc.
+
+import { mockLogFn, mockLogger } from '@dd/tests/_jest/helpers/mocks';
+
+import { resetEnableWarnings, resolveEnable } from './options';
+
+beforeEach(() => {
+    jest.clearAllMocks();
+    resetEnableWarnings();
+});
+
+describe('resolveEnable', () => {
+    describe('standard boolean / omitted values', () => {
+        const cases = [
+            {
+                description: 'return false when the config key is undefined',
+                options: {},
+                expected: false,
+            },
+            {
+                description: 'return false when the config key is null',
+                options: { myPlugin: null },
+                expected: false,
+            },
+            {
+                description: 'return true when the config key is a truthy object without enable',
+                options: { myPlugin: { someOther: 'val' } },
+                expected: true,
+            },
+            {
+                description: 'return true when enable is true',
+                options: { myPlugin: { enable: true } },
+                expected: true,
+            },
+            {
+                description: 'return false when enable is false',
+                options: { myPlugin: { enable: false } },
+                expected: false,
+            },
+            {
+                description: 'return true when enable is undefined (object present)',
+                options: { myPlugin: { enable: undefined } },
+                expected: true,
+            },
+        ];
+
+        test.each(cases)('should $description', ({ options, expected }) => {
+            expect(resolveEnable(options, 'myPlugin', mockLogger)).toBe(expected);
+            expect(mockLogFn).not.toHaveBeenCalled();
+        });
+    });
+
+    describe('non-boolean coercion with deprecation warning', () => {
+        const cases = [
+            {
+                description: 'coerce enable: 1 to true and warn',
+                options: { myPlugin: { enable: 1 } },
+                expected: true,
+            },
+            {
+                description: 'coerce enable: 0 to false and warn',
+                options: { myPlugin: { enable: 0 } },
+                expected: false,
+            },
+            {
+                description: 'coerce enable: "true" to true and warn',
+                options: { myPlugin: { enable: 'true' } },
+                expected: true,
+            },
+            {
+                description: 'coerce enable: "" to false and warn',
+                options: { myPlugin: { enable: '' } },
+                expected: false,
+            },
+        ];
+
+        test.each(cases)('should $description', ({ options, expected }) => {
+            expect(resolveEnable(options, 'myPlugin', mockLogger)).toBe(expected);
+            expect(mockLogFn).toHaveBeenCalledTimes(1);
+            expect(mockLogFn).toHaveBeenCalledWith(
+                expect.stringContaining('myPlugin.enable'),
+                'warn',
+            );
+        });
+    });
+
+    describe('warn-once behavior', () => {
+        test('should only warn once per config key across multiple calls', () => {
+            resolveEnable({ myPlugin: { enable: 1 } }, 'myPlugin', mockLogger);
+            resolveEnable({ myPlugin: { enable: 'yes' } }, 'myPlugin', mockLogger);
+            expect(mockLogFn).toHaveBeenCalledTimes(1);
+        });
+
+        test('should warn separately for different config keys', () => {
+            resolveEnable({ pluginA: { enable: 1 } }, 'pluginA', mockLogger);
+            resolveEnable({ pluginB: { enable: 1 } }, 'pluginB', mockLogger);
+            expect(mockLogFn).toHaveBeenCalledTimes(2);
+        });
+    });
+});

packages/core/src/helpers/options.ts

@@ -0,0 +1,52 @@
+// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2019-Present Datadog, Inc.
+
+import type { Logger } from '@dd/core/types';
+
+const warnedKeys = new Set<string>();
+
+/**
+ * Resolve the `enable` value for a plugin config key, emitting a deprecation
+ * warning when the caller passes a non-boolean truthy/falsy value.
+ *
+ * Semantics:
+ *  - Config key absent / undefined / falsy → false (plugin disabled).
+ *  - Config key is a truthy object without an `enable` property → true.
+ *  - Config key is a truthy object with `enable` set → coerce to boolean,
+ *    warning once per key if it isn't already a boolean.
+ */
+export const resolveEnable = <T extends { [K in C]?: unknown }, C extends string>(
+    options: T,
+    configKey: C,
+    log: Logger,
+): boolean => {
+    const pluginConfig = options[configKey];
+
+    if (pluginConfig && typeof pluginConfig === 'object' && 'enable' in pluginConfig) {
+        const value = pluginConfig.enable;
+
+        if (typeof value !== 'boolean' && value !== undefined) {
+            if (!warnedKeys.has(configKey)) {
+                warnedKeys.add(configKey);
+                log.warn(
+                    `\`${configKey}.enable\` should be a boolean, got ${typeof value}. ` +
+                        `Non-boolean values are coerced today but will be rejected in the next major.`,
+                );
+            }
+        }
+
+        if (value !== undefined) {
+            // TODO(next major): drop this coercion and reject non-boolean `enable`
+            // outright. The warning above gives callers one major to migrate.
+            return !!value;
+        }
+    }
+
+    return !!pluginConfig;
+};
+
+/** @internal Exposed only for tests to reset the warn-once set between cases. */
+export const resetEnableWarnings = (): void => {
+    warnedKeys.clear();
+};

packages/factory/src/index.test.ts

@@ -2,14 +2,67 @@
 // This product includes software developed at Datadog (https://www.datadoghq.com/).
 // Copyright 2019-Present Datadog, Inc.
 
+import type { PluginOptions, Options } from '@dd/core/types';
+import { buildPluginFactory } from '@dd/factory';
+
+const invokeFactory = (opts: Options): PluginOptions[] => {
+    const factory = buildPluginFactory({ bundler: {}, version: '1.0.0' });
+    return factory.raw(opts, { framework: 'esbuild' }) as PluginOptions[];
+};
+
+const hasPlugin = (plugins: PluginOptions[], name: string) =>
+    plugins.some((plugin) => plugin.name.includes(name));
+
 describe('Factory', () => {
-    test('Should not throw with no options', async () => {
-        const { buildPluginFactory } = await import('@dd/factory');
+    test('Should not throw with no options', () => {
         expect(() => {
             const factory = buildPluginFactory({ bundler: {}, version: '1.0.0' });
             // Vite could call the factory without options.
             // @ts-expect-error - We are testing the factory without options.
             factory.vite();
         }).not.toThrow();
     });
+
+    describe('enable gating for user-facing plugins', () => {
+        // The factory is the single source of truth for `<configKey>.enable`.
+        // Each user-facing plugin is skipped when its config key is absent or
+        // explicitly disabled, and included when the config key is present.
+
+        test('Should skip a plugin when its config key is absent', () => {
+            const plugins = invokeFactory({ logLevel: 'none' });
+            expect(hasPlugin(plugins, 'output')).toBe(false);
+            expect(hasPlugin(plugins, 'metrics')).toBe(false);
+            expect(hasPlugin(plugins, 'rum')).toBe(false);
+        });
+
+        test('Should include a plugin when its config key is present', () => {
+            const plugins = invokeFactory({ logLevel: 'none', output: {} });
+            expect(hasPlugin(plugins, 'output')).toBe(true);
+        });
+
+        test('Should skip a plugin when enable: false', () => {
+            const plugins = invokeFactory({
+                logLevel: 'none',
+                output: { enable: false },
+            });
+            expect(hasPlugin(plugins, 'output')).toBe(false);
+        });
+
+        test('Should include a plugin when enable: true', () => {
+            const plugins = invokeFactory({
+                logLevel: 'none',
+                output: { enable: true },
+            });
+            expect(hasPlugin(plugins, 'output')).toBe(true);
+        });
+
+        test('Should coerce a non-boolean enable value and still include the plugin', () => {
+            const plugins = invokeFactory({
+                logLevel: 'none',
+                // @ts-expect-error - intentional non-boolean to exercise coercion
+                output: { enable: 1 },
+            });
+            expect(hasPlugin(plugins, 'output')).toBe(true);
+        });
+    });
 });

packages/factory/src/index.ts

@@ -34,6 +34,7 @@ import { getContext } from './helpers/context';
 import { wrapGetPlugins } from './helpers/wrapPlugins';
 import { ALL_ENVS, HOST_NAME } from '@dd/core/constants';
 import { notifyOnEnvOverrides } from '@dd/core/helpers/env';
+import { resolveEnable } from '@dd/core/helpers/options';
 // #imports-injection-marker
 import * as apps from '@dd/apps-plugin';
 import * as errorTracking from '@dd/error-tracking-plugin';
@@ -160,17 +161,27 @@ export const buildPluginFactory = ({
             pluginsToAdd.push(['custom', options.customPlugins]);
         }
 
-        // Add the customer facing plugins.
-        pluginsToAdd.push(
+        // Customer-facing plugins are gated by their `<configKey>.enable` flag.
+        // Resolving here lets every plugin share the same semantics:
+        //   - config key absent → disabled
+        //   - config key present without `enable` → enabled
+        //   - non-boolean `enable` → coerced, with a single deprecation warning
+        const userFacingPlugins: [name: string, configKey: string, GetPlugins][] = [
             // #configs-injection-marker
-            ['apps', apps.getPlugins],
-            ['error-tracking', errorTracking.getPlugins],
-            ['live-debugger', liveDebugger.getPlugins],
-            ['metrics', metrics.getPlugins],
-            ['output', output.getPlugins],
-            ['rum', rum.getPlugins],
+            ['apps', apps.CONFIG_KEY, apps.getPlugins],
+            ['error-tracking', errorTracking.CONFIG_KEY, errorTracking.getPlugins],
+            ['live-debugger', liveDebugger.CONFIG_KEY, liveDebugger.getPlugins],
+            ['metrics', metrics.CONFIG_KEY, metrics.getPlugins],
+            ['output', output.CONFIG_KEY, output.getPlugins],
+            ['rum', rum.CONFIG_KEY, rum.getPlugins],
             // #configs-injection-marker
-        );
+        ];
+
+        for (const [name, configKey, getPlugins] of userFacingPlugins) {
+            if (resolveEnable(options, configKey, log)) {
+                pluginsToAdd.push([name, getPlugins]);
+            }
+        }
 
         // Initialize all our plugins.
         for (const [name, getPlugins] of pluginsToAdd) {

packages/plugins/apps/README.md

@@ -54,10 +54,12 @@ Setting the `apps.dryRun` configuration will override any value set in the envir
 
 ### apps.enable
 
-> default: `true` when an `apps` config block is present
+> default: `true` when an `apps` config block is present, `false` otherwise.
 
 Enable or disable the plugin without removing its configuration.
 
+Must be a boolean. Non-boolean values are coerced today but will be rejected in a future major release.
+
 ### apps.include
 
 > default: `[]`

packages/plugins/apps/src/index.ts

@@ -19,9 +19,6 @@ export type types = {
 export const getPlugins: GetPlugins = ({ options, context, bundler }) => {
     const log = context.getLogger(PLUGIN_NAME);
     const validatedOptions = validateOptions(options);
-    if (!validatedOptions.enable) {
-        return [];
-    }
 
     if (context.bundler.name !== 'vite') {
         log.warn(`The apps plugin only supports Vite; skipping under '${context.bundler.name}'.`);

packages/plugins/apps/src/types.ts

@@ -25,4 +25,7 @@ export type AppsManifest = {
 };
 
 // We don't enforce identifier, as it needs to be dynamically computed if absent.
-export type AppsOptionsWithDefaults = WithRequired<AppsOptions, 'enable' | 'include' | 'dryRun'>;
+export type AppsOptionsWithDefaults = Omit<
+    WithRequired<AppsOptions, 'include' | 'dryRun'>,
+    'enable'
+>;

packages/plugins/apps/src/validate.test.ts

@@ -5,42 +5,11 @@
 import { validateOptions } from '@dd/apps-plugin/validate';
 
 describe('Apps Plugin - validateOptions', () => {
-    describe('enable flag', () => {
-        const cases = [
-            {
-                description: 'return false when no apps config is provided',
-                input: {},
-                expected: false,
-            },
-            {
-                description: 'return true when apps config is an empty object',
-                input: { apps: {} },
-                expected: true,
-            },
-            {
-                description: 'respect explicit enable true',
-                input: { apps: { enable: true } },
-                expected: true,
-            },
-            {
-                description: 'respect explicit enable false',
-                input: { apps: { enable: false } },
-                expected: false,
-            },
-        ];
-
-        test.each(cases)('Should $description', ({ input, expected }) => {
-            const result = validateOptions(input);
-            expect(result.enable).toBe(expected);
-        });
-    });
-
     describe('defaults', () => {
         test('Should set defaults when nothing is provided', () => {
             const result = validateOptions({});
             expect(result).toEqual({
                 dryRun: true,
-                enable: false,
                 include: [],
                 identifier: undefined,
                 name: undefined,
@@ -91,7 +60,6 @@ describe('Apps Plugin - validateOptions', () => {
 
             expect(result).toEqual({
                 dryRun: true,
-                enable: true,
                 include: ['public/**/*', 'dist/**/*'],
                 identifier: 'my-app',
                 name: undefined,

packages/plugins/apps/src/validate.ts

@@ -10,15 +10,11 @@ import type { AppsOptions, AppsOptionsWithDefaults } from './types';
 
 export const validateOptions = (options: Options): AppsOptionsWithDefaults => {
     const resolvedOptions = (options[CONFIG_KEY] || {}) as AppsOptions;
-    const enable = resolvedOptions.enable ?? !!options[CONFIG_KEY];
 
-    const validatedOptions: AppsOptionsWithDefaults = {
-        enable,
+    return {
         include: resolvedOptions.include || [],
         dryRun: resolvedOptions.dryRun ?? !getDDEnvValue('APPS_UPLOAD_ASSETS'),
         identifier: resolvedOptions.identifier?.trim(),
         name: resolvedOptions.name?.trim() || options.metadata?.name?.trim(),
     };
-
-    return validatedOptions;
 };

packages/plugins/error-tracking/README.md

@@ -10,6 +10,7 @@ Interact with Error Tracking directly from your build system.
 
 <!-- #toc -->
 -   [Configuration](#configuration)
+    -   [errorTracking.enable](#errortrackingenable)
 -   [Sourcemaps Upload](#sourcemaps-upload)
     -   [errorTracking.sourcemaps.bailOnError](#errortrackingsourcemapsbailonerror)
     -   [errorTracking.sourcemaps.dryRun](#errortrackingsourcemapsdryrun)
@@ -35,6 +36,14 @@ errorTracking?: {
 }
 ```
 
+### errorTracking.enable
+
+> default: `true` when an `errorTracking` config block is present, `false` otherwise.
+
+Enable or disable the plugin without removing its configuration.
+
+Must be a boolean. Non-boolean values are coerced today but will be rejected in a future major release.
+
 ## Sourcemaps Upload
 
 Upload JavaScript sourcemaps to Datadog to un-minify your errors.