Skip to content

Commit dd508cf

Browse files
watsonwatson
committed
Standardize enable option handling across all plugins
Introduce a shared `resolveEnable` helper in @dd/core that all six user-facing plugins now use to resolve their `enable` config flag. This replaces the two divergent patterns (explicit `??` vs spread- override) with a single greppable call site per plugin. Non-boolean values continue to be coerced for backwards compatibility but now emit a deprecation warning so strict validation can land in the next major. live-debugger retains its existing hard rejection via the companion `validateEnableStrict` helper. Also fixes the misleading `default: true` wording in the output and metrics READMEs, adds the missing `errorTracking.enable` docs section, and authors a full README for the rum plugin (lost in the sourcemaps extraction refactor of 804f917).
1 parent 441c100 commit dd508cf

22 files changed

Lines changed: 788 additions & 57 deletions

File tree

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

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License.
2+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
3+
// Copyright 2019-Present Datadog, Inc.
4+
5+
import type { Logger } from '@dd/core/types';
6+
7+
import { resetEnableWarnings, resolveEnable, validateEnableStrict } from './options';
8+
9+
const mockLogger: Logger = {
10+
getLogger: jest.fn(),
11+
time: jest.fn() as unknown as Logger['time'],
12+
error: jest.fn(),
13+
warn: jest.fn(),
14+
info: jest.fn(),
15+
debug: jest.fn(),
16+
};
17+
18+
beforeEach(() => {
19+
jest.clearAllMocks();
20+
resetEnableWarnings();
21+
});
22+
23+
describe('resolveEnable', () => {
24+
describe('standard boolean / omitted values', () => {
25+
const cases = [
26+
{
27+
description: 'return false when the config key is undefined',
28+
options: {},
29+
expected: false,
30+
},
31+
{
32+
description: 'return false when the config key is null',
33+
options: { myPlugin: null },
34+
expected: false,
35+
},
36+
{
37+
description: 'return true when the config key is a truthy object without enable',
38+
options: { myPlugin: { someOther: 'val' } },
39+
expected: true,
40+
},
41+
{
42+
description: 'return true when enable is true',
43+
options: { myPlugin: { enable: true } },
44+
expected: true,
45+
},
46+
{
47+
description: 'return false when enable is false',
48+
options: { myPlugin: { enable: false } },
49+
expected: false,
50+
},
51+
{
52+
description: 'return true when enable is undefined (object present)',
53+
options: { myPlugin: { enable: undefined } },
54+
expected: true,
55+
},
56+
];
57+
58+
test.each(cases)('should $description', ({ options, expected }) => {
59+
expect(resolveEnable(options, 'myPlugin', mockLogger)).toBe(expected);
60+
expect(mockLogger.warn).not.toHaveBeenCalled();
61+
});
62+
});
63+
64+
describe('non-boolean coercion with deprecation warning', () => {
65+
const cases = [
66+
{
67+
description: 'coerce enable: 1 to true and warn',
68+
options: { myPlugin: { enable: 1 } },
69+
expected: true,
70+
},
71+
{
72+
description: 'coerce enable: 0 to false and warn',
73+
options: { myPlugin: { enable: 0 } },
74+
expected: false,
75+
},
76+
{
77+
description: 'coerce enable: "true" to true and warn',
78+
options: { myPlugin: { enable: 'true' } },
79+
expected: true,
80+
},
81+
{
82+
description: 'coerce enable: "" to false and warn',
83+
options: { myPlugin: { enable: '' } },
84+
expected: false,
85+
},
86+
];
87+
88+
test.each(cases)('should $description', ({ options, expected }) => {
89+
expect(resolveEnable(options, 'myPlugin', mockLogger)).toBe(expected);
90+
expect(mockLogger.warn).toHaveBeenCalledTimes(1);
91+
expect(mockLogger.warn).toHaveBeenCalledWith(
92+
expect.stringContaining('myPlugin.enable'),
93+
);
94+
});
95+
});
96+
97+
describe('warn-once behavior', () => {
98+
test('should only warn once per config key across multiple calls', () => {
99+
resolveEnable({ myPlugin: { enable: 1 } }, 'myPlugin', mockLogger);
100+
resolveEnable({ myPlugin: { enable: 'yes' } }, 'myPlugin', mockLogger);
101+
expect(mockLogger.warn).toHaveBeenCalledTimes(1);
102+
});
103+
104+
test('should warn separately for different config keys', () => {
105+
resolveEnable({ pluginA: { enable: 1 } }, 'pluginA', mockLogger);
106+
resolveEnable({ pluginB: { enable: 1 } }, 'pluginB', mockLogger);
107+
expect(mockLogger.warn).toHaveBeenCalledTimes(2);
108+
});
109+
});
110+
});
111+
112+
describe('validateEnableStrict', () => {
113+
test('should not push an error when enable is a boolean', () => {
114+
const errors: string[] = [];
115+
validateEnableStrict({ enable: true }, errors);
116+
expect(errors).toHaveLength(0);
117+
});
118+
119+
test('should not push an error when enable is undefined', () => {
120+
const errors: string[] = [];
121+
validateEnableStrict({ enable: undefined }, errors);
122+
expect(errors).toHaveLength(0);
123+
});
124+
125+
test('should push an error when enable is a non-boolean', () => {
126+
const errors: string[] = [];
127+
validateEnableStrict({ enable: 'yes' }, errors);
128+
expect(errors).toHaveLength(1);
129+
expect(errors[0]).toContain('enable');
130+
expect(errors[0]).toContain('boolean');
131+
});
132+
});

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License.
2+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
3+
// Copyright 2019-Present Datadog, Inc.
4+
5+
import type { Logger } from '@dd/core/types';
6+
import chalk from 'chalk';
7+
8+
const warnedKeys = new Set<string>();
9+
10+
/**
11+
* Resolve the `enable` value for a plugin config key, emitting a deprecation
12+
* warning when the caller passes a non-boolean truthy/falsy value.
13+
*
14+
* Semantics:
15+
* - Config key absent / undefined / falsy → false (plugin disabled).
16+
* - Config key is a truthy object without an `enable` property → true.
17+
* - Config key is a truthy object with `enable` set → coerce to boolean,
18+
* warning once per key if it isn't already a boolean.
19+
*/
20+
export const resolveEnable = <T extends { [K in C]?: unknown }, C extends string>(
21+
options: T,
22+
configKey: C,
23+
log: Logger,
24+
): boolean => {
25+
const pluginConfig = options[configKey];
26+
27+
if (pluginConfig && typeof pluginConfig === 'object' && 'enable' in pluginConfig) {
28+
const value = (pluginConfig as Record<string, unknown>).enable;
29+
30+
if (typeof value !== 'boolean' && value !== undefined) {
31+
if (!warnedKeys.has(configKey)) {
32+
warnedKeys.add(configKey);
33+
log.warn(
34+
`\`${configKey}.enable\` should be a boolean, got ${typeof value}. ` +
35+
`Non-boolean values are coerced today but will be rejected in the next major.`,
36+
);
37+
}
38+
}
39+
40+
if (value !== undefined) {
41+
return !!value;
42+
}
43+
}
44+
45+
return !!pluginConfig;
46+
};
47+
48+
/**
49+
* Push a strict validation error when `enable` is present but not a boolean.
50+
* Used by plugins that have always rejected non-boolean values (e.g. live-debugger).
51+
*/
52+
export const validateEnableStrict = (
53+
pluginConfig: { enable?: unknown },
54+
errors: string[],
55+
): void => {
56+
if (pluginConfig.enable !== undefined && typeof pluginConfig.enable !== 'boolean') {
57+
errors.push(`${chalk.bold.red('enable')} must be a boolean`);
58+
}
59+
};
60+
61+
/** @internal Exposed only for tests to reset the warn-once set between cases. */
62+
export const resetEnableWarnings = (): void => {
63+
warnedKeys.clear();
64+
};

‎packages/plugins/apps/README.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,12 @@ Setting the `apps.dryRun` configuration will override any value set in the envir
5454

5555
### apps.enable
5656

57-
> default: `true` when an `apps` config block is present
57+
> default: `true` when an `apps` config block is present, `false` otherwise.
5858
5959
Enable or disable the plugin without removing its configuration.
6060

61+
Must be a boolean. Non-boolean values are coerced today but will be rejected in a future major release.
62+
6163
### apps.include
6264

6365
> default: `[]`

‎packages/plugins/apps/src/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ export type types = {
3131
export const getPlugins: GetPlugins = ({ options, context, bundler }) => {
3232
const log = context.getLogger(PLUGIN_NAME);
3333
let toThrow: Error | undefined;
34-
const validatedOptions = validateOptions(options);
34+
const validatedOptions = validateOptions(options, log);
3535
if (!validatedOptions.enable) {
3636
return [];
3737
}

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

Lines changed: 61 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,22 @@
33
// Copyright 2019-Present Datadog, Inc.
44

55
import { validateOptions } from '@dd/apps-plugin/validate';
6+
import { resetEnableWarnings } from '@dd/core/helpers/options';
7+
import type { Logger } from '@dd/core/types';
8+
9+
const mockLogger: Logger = {
10+
getLogger: jest.fn(() => mockLogger),
11+
time: jest.fn() as unknown as Logger['time'],
12+
error: jest.fn(),
13+
warn: jest.fn(),
14+
info: jest.fn(),
15+
debug: jest.fn(),
16+
};
17+
18+
beforeEach(() => {
19+
jest.clearAllMocks();
20+
resetEnableWarnings();
21+
});
622

723
describe('Apps Plugin - validateOptions', () => {
824
describe('enable flag', () => {
@@ -30,14 +46,44 @@ describe('Apps Plugin - validateOptions', () => {
3046
];
3147

3248
test.each(cases)('Should $description', ({ input, expected }) => {
33-
const result = validateOptions(input);
49+
const result = validateOptions(input, mockLogger);
3450
expect(result.enable).toBe(expected);
3551
});
3652
});
3753

54+
describe('enable deprecation warning for non-boolean values', () => {
55+
const cases = [
56+
{
57+
description: 'coerce enable: 1 to true and warn',
58+
input: { apps: { enable: 1 } },
59+
expected: true,
60+
},
61+
{
62+
description: 'coerce enable: 0 to false and warn',
63+
input: { apps: { enable: 0 } },
64+
expected: false,
65+
},
66+
{
67+
description: 'coerce enable: "true" to true and warn',
68+
input: { apps: { enable: 'true' } },
69+
expected: true,
70+
},
71+
];
72+
73+
test.each(cases)('Should $description', ({ input, expected }) => {
74+
const result = validateOptions(
75+
input as unknown as Parameters<typeof validateOptions>[0],
76+
mockLogger,
77+
);
78+
expect(result.enable).toBe(expected);
79+
expect(mockLogger.warn).toHaveBeenCalledTimes(1);
80+
expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('apps.enable'));
81+
});
82+
});
83+
3884
describe('defaults', () => {
3985
test('Should set defaults when nothing is provided', () => {
40-
const result = validateOptions({});
86+
const result = validateOptions({}, mockLogger);
4187
expect(result).toEqual({
4288
backendDir: 'backend',
4389
dryRun: true,
@@ -51,7 +97,7 @@ describe('Apps Plugin - validateOptions', () => {
5197
test('Should set dryRun to false when DATADOG_APPS_UPLOAD_ASSETS is set', () => {
5298
process.env.DATADOG_APPS_UPLOAD_ASSETS = '1';
5399
try {
54-
const result = validateOptions({ apps: {} });
100+
const result = validateOptions({ apps: {} }, mockLogger);
55101
expect(result.dryRun).toBe(false);
56102
} finally {
57103
delete process.env.DATADOG_APPS_UPLOAD_ASSETS;
@@ -61,7 +107,7 @@ describe('Apps Plugin - validateOptions', () => {
61107
test('Should set dryRun to false when DD_APPS_UPLOAD_ASSETS is set', () => {
62108
process.env.DD_APPS_UPLOAD_ASSETS = '1';
63109
try {
64-
const result = validateOptions({ apps: {} });
110+
const result = validateOptions({ apps: {} }, mockLogger);
65111
expect(result.dryRun).toBe(false);
66112
} finally {
67113
delete process.env.DD_APPS_UPLOAD_ASSETS;
@@ -71,7 +117,7 @@ describe('Apps Plugin - validateOptions', () => {
71117
test('Should respect explicit dryRun over env var', () => {
72118
process.env.DATADOG_APPS_UPLOAD_ASSETS = '1';
73119
try {
74-
const result = validateOptions({ apps: { dryRun: true } });
120+
const result = validateOptions({ apps: { dryRun: true } }, mockLogger);
75121
expect(result.dryRun).toBe(true);
76122
} finally {
77123
delete process.env.DATADOG_APPS_UPLOAD_ASSETS;
@@ -81,14 +127,17 @@ describe('Apps Plugin - validateOptions', () => {
81127

82128
describe('overrides', () => {
83129
test('Should keep provided options and trim identifier', () => {
84-
const result = validateOptions({
85-
apps: {
86-
dryRun: true,
87-
enable: true,
88-
include: ['public/**/*', 'dist/**/*'],
89-
identifier: ' my-app ',
130+
const result = validateOptions(
131+
{
132+
apps: {
133+
dryRun: true,
134+
enable: true,
135+
include: ['public/**/*', 'dist/**/*'],
136+
identifier: ' my-app ',
137+
},
90138
},
91-
});
139+
mockLogger,
140+
);
92141

93142
expect(result).toEqual({
94143
backendDir: 'backend',

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,17 @@
33
// Copyright 2019-Present Datadog, Inc.
44

55
import { getDDEnvValue } from '@dd/core/helpers/env';
6-
import type { Options } from '@dd/core/types';
6+
import { resolveEnable } from '@dd/core/helpers/options';
7+
import type { Logger, Options } from '@dd/core/types';
78

89
import { CONFIG_KEY } from './constants';
910
import type { AppsOptions, AppsOptionsWithDefaults } from './types';
1011

11-
export const validateOptions = (options: Options): AppsOptionsWithDefaults => {
12+
export const validateOptions = (options: Options, log: Logger): AppsOptionsWithDefaults => {
1213
const resolvedOptions = (options[CONFIG_KEY] || {}) as AppsOptions;
13-
const enable = resolvedOptions.enable ?? !!options[CONFIG_KEY];
1414

1515
const validatedOptions: AppsOptionsWithDefaults = {
16-
enable,
16+
enable: resolveEnable(options, CONFIG_KEY, log),
1717
include: resolvedOptions.include || [],
1818
dryRun: resolvedOptions.dryRun ?? !getDDEnvValue('APPS_UPLOAD_ASSETS'),
1919
identifier: resolvedOptions.identifier?.trim(),

‎packages/plugins/error-tracking/README.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ Interact with Error Tracking directly from your build system.
1010

1111
<!-- #toc -->
1212
- [Configuration](#configuration)
13+
- [errorTracking.enable](#errortrackingenable)
1314
- [Sourcemaps Upload](#sourcemaps-upload)
1415
- [errorTracking.sourcemaps.bailOnError](#errortrackingsourcemapsbailonerror)
1516
- [errorTracking.sourcemaps.dryRun](#errortrackingsourcemapsdryrun)
@@ -35,6 +36,14 @@ errorTracking?: {
3536
}
3637
```
3738

39+
### errorTracking.enable
40+
41+
> default: `true` when an `errorTracking` config block is present, `false` otherwise.
42+
43+
Enable or disable the plugin without removing its configuration.
44+
45+
Must be a boolean. Non-boolean values are coerced today but will be rejected in a future major release.
46+
3847
## Sourcemaps Upload
3948

4049
Upload JavaScript sourcemaps to Datadog to un-minify your errors.

0 commit comments

Comments
 (0)