Skip to content

Commit f847e1a

Browse files
watsonwatson
committed
Address review comments
1 parent 5d29c9a commit f847e1a

38 files changed

Lines changed: 145 additions & 863 deletions

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

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
import type { Logger } from '@dd/core/types';
66

7-
import { resetEnableWarnings, resolveEnable, validateEnableStrict } from './options';
7+
import { resetEnableWarnings, resolveEnable } from './options';
88

99
const mockLogger: Logger = {
1010
getLogger: jest.fn(),
@@ -108,25 +108,3 @@ describe('resolveEnable', () => {
108108
});
109109
});
110110
});
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: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
// Copyright 2019-Present Datadog, Inc.
44

55
import type { Logger } from '@dd/core/types';
6-
import chalk from 'chalk';
76

87
const warnedKeys = new Set<string>();
98

@@ -38,26 +37,15 @@ export const resolveEnable = <T extends { [K in C]?: unknown }, C extends string
3837
}
3938

4039
if (value !== undefined) {
40+
// TODO(next major): drop this coercion and reject non-boolean `enable`
41+
// outright. The warning above gives callers one major to migrate.
4142
return !!value;
4243
}
4344
}
4445

4546
return !!pluginConfig;
4647
};
4748

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-
6149
/** @internal Exposed only for tests to reset the warn-once set between cases. */
6250
export const resetEnableWarnings = (): void => {
6351
warnedKeys.clear();

‎packages/factory/src/index.test.ts‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,17 @@
22
// This product includes software developed at Datadog (https://www.datadoghq.com/).
33
// Copyright 2019-Present Datadog, Inc.
44

5+
import type { PluginOptions, Options } from '@dd/core/types';
6+
7+
const invokeFactory = async (opts: Options): Promise<PluginOptions[]> => {
8+
const { buildPluginFactory } = await import('@dd/factory');
9+
const factory = buildPluginFactory({ bundler: {}, version: '1.0.0' });
10+
return factory.raw(opts, { framework: 'esbuild' }) as PluginOptions[];
11+
};
12+
13+
const hasPlugin = (plugins: PluginOptions[], name: string) =>
14+
plugins.some((plugin) => plugin.name.includes(name));
15+
516
describe('Factory', () => {
617
test('Should not throw with no options', async () => {
718
const { buildPluginFactory } = await import('@dd/factory');
@@ -12,4 +23,47 @@ describe('Factory', () => {
1223
factory.vite();
1324
}).not.toThrow();
1425
});
26+
27+
describe('enable gating for user-facing plugins', () => {
28+
// The factory is the single source of truth for `<configKey>.enable`.
29+
// Each user-facing plugin is skipped when its config key is absent or
30+
// explicitly disabled, and included when the config key is present.
31+
32+
test('Should skip a plugin when its config key is absent', async () => {
33+
const plugins = await invokeFactory({ logLevel: 'none' });
34+
expect(hasPlugin(plugins, 'output')).toBe(false);
35+
expect(hasPlugin(plugins, 'metrics')).toBe(false);
36+
expect(hasPlugin(plugins, 'rum')).toBe(false);
37+
});
38+
39+
test('Should include a plugin when its config key is present', async () => {
40+
const plugins = await invokeFactory({ logLevel: 'none', output: {} });
41+
expect(hasPlugin(plugins, 'output')).toBe(true);
42+
});
43+
44+
test('Should skip a plugin when enable: false', async () => {
45+
const plugins = await invokeFactory({
46+
logLevel: 'none',
47+
output: { enable: false },
48+
});
49+
expect(hasPlugin(plugins, 'output')).toBe(false);
50+
});
51+
52+
test('Should include a plugin when enable: true', async () => {
53+
const plugins = await invokeFactory({
54+
logLevel: 'none',
55+
output: { enable: true },
56+
});
57+
expect(hasPlugin(plugins, 'output')).toBe(true);
58+
});
59+
60+
test('Should coerce a non-boolean enable value and still include the plugin', async () => {
61+
const plugins = await invokeFactory({
62+
logLevel: 'none',
63+
// @ts-expect-error - intentional non-boolean to exercise coercion
64+
output: { enable: 1 },
65+
});
66+
expect(hasPlugin(plugins, 'output')).toBe(true);
67+
});
68+
});
1569
});

‎packages/factory/src/index.ts‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import { getContext } from './helpers/context';
3434
import { wrapGetPlugins } from './helpers/wrapPlugins';
3535
import { ALL_ENVS, HOST_NAME } from '@dd/core/constants';
3636
import { notifyOnEnvOverrides } from '@dd/core/helpers/env';
37+
import { resolveEnable } from '@dd/core/helpers/options';
3738
// #imports-injection-marker
3839
import * as apps from '@dd/apps-plugin';
3940
import * as errorTracking from '@dd/error-tracking-plugin';
@@ -160,17 +161,27 @@ export const buildPluginFactory = ({
160161
pluginsToAdd.push(['custom', options.customPlugins]);
161162
}
162163

163-
// Add the customer facing plugins.
164-
pluginsToAdd.push(
164+
// Customer-facing plugins are gated by their `<configKey>.enable` flag.
165+
// Resolving here lets every plugin share the same semantics:
166+
// - config key absent → disabled
167+
// - config key present without `enable` → enabled
168+
// - non-boolean `enable` → coerced, with a single deprecation warning
169+
const userFacingPlugins: [name: string, configKey: string, GetPlugins][] = [
165170
// #configs-injection-marker
166-
['apps', apps.getPlugins],
167-
['error-tracking', errorTracking.getPlugins],
168-
['live-debugger', liveDebugger.getPlugins],
169-
['metrics', metrics.getPlugins],
170-
['output', output.getPlugins],
171-
['rum', rum.getPlugins],
171+
['apps', apps.CONFIG_KEY, apps.getPlugins],
172+
['error-tracking', errorTracking.CONFIG_KEY, errorTracking.getPlugins],
173+
['live-debugger', liveDebugger.CONFIG_KEY, liveDebugger.getPlugins],
174+
['metrics', metrics.CONFIG_KEY, metrics.getPlugins],
175+
['output', output.CONFIG_KEY, output.getPlugins],
176+
['rum', rum.CONFIG_KEY, rum.getPlugins],
172177
// #configs-injection-marker
173-
);
178+
];
179+
180+
for (const [name, configKey, getPlugins] of userFacingPlugins) {
181+
if (resolveEnable(options, configKey, log)) {
182+
pluginsToAdd.push([name, getPlugins]);
183+
}
184+
}
174185

175186
// Initialize all our plugins.
176187
for (const [name, getPlugins] of pluginsToAdd) {

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,7 @@ export type types = {
1818

1919
export const getPlugins: GetPlugins = ({ options, context, bundler }) => {
2020
const log = context.getLogger(PLUGIN_NAME);
21-
const validatedOptions = validateOptions(options, log);
22-
if (!validatedOptions.enable) {
23-
return [];
24-
}
21+
const validatedOptions = validateOptions(options);
2522

2623
if (context.bundler.name !== 'vite') {
2724
log.warn(`The apps plugin only supports Vite; skipping under '${context.bundler.name}'.`);

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,4 +25,7 @@ export type AppsManifest = {
2525
};
2626

2727
// We don't enforce identifier, as it needs to be dynamically computed if absent.
28-
export type AppsOptionsWithDefaults = WithRequired<AppsOptions, 'enable' | 'include' | 'dryRun'>;
28+
export type AppsOptionsWithDefaults = Omit<
29+
WithRequired<AppsOptions, 'include' | 'dryRun'>,
30+
'enable'
31+
>;

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

Lines changed: 11 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -3,90 +3,13 @@
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-
});
226

237
describe('Apps Plugin - validateOptions', () => {
24-
describe('enable flag', () => {
25-
const cases = [
26-
{
27-
description: 'return false when no apps config is provided',
28-
input: {},
29-
expected: false,
30-
},
31-
{
32-
description: 'return true when apps config is an empty object',
33-
input: { apps: {} },
34-
expected: true,
35-
},
36-
{
37-
description: 'respect explicit enable true',
38-
input: { apps: { enable: true } },
39-
expected: true,
40-
},
41-
{
42-
description: 'respect explicit enable false',
43-
input: { apps: { enable: false } },
44-
expected: false,
45-
},
46-
];
47-
48-
test.each(cases)('Should $description', ({ input, expected }) => {
49-
const result = validateOptions(input, mockLogger);
50-
expect(result.enable).toBe(expected);
51-
});
52-
});
53-
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-
848
describe('defaults', () => {
859
test('Should set defaults when nothing is provided', () => {
86-
const result = validateOptions({}, mockLogger);
10+
const result = validateOptions({});
8711
expect(result).toEqual({
8812
dryRun: true,
89-
enable: false,
9013
include: [],
9114
identifier: undefined,
9215
name: undefined,
@@ -96,7 +19,7 @@ describe('Apps Plugin - validateOptions', () => {
9619
test('Should set dryRun to false when DATADOG_APPS_UPLOAD_ASSETS is set', () => {
9720
process.env.DATADOG_APPS_UPLOAD_ASSETS = '1';
9821
try {
99-
const result = validateOptions({ apps: {} }, mockLogger);
22+
const result = validateOptions({ apps: {} });
10023
expect(result.dryRun).toBe(false);
10124
} finally {
10225
delete process.env.DATADOG_APPS_UPLOAD_ASSETS;
@@ -106,7 +29,7 @@ describe('Apps Plugin - validateOptions', () => {
10629
test('Should set dryRun to false when DD_APPS_UPLOAD_ASSETS is set', () => {
10730
process.env.DD_APPS_UPLOAD_ASSETS = '1';
10831
try {
109-
const result = validateOptions({ apps: {} }, mockLogger);
32+
const result = validateOptions({ apps: {} });
11033
expect(result.dryRun).toBe(false);
11134
} finally {
11235
delete process.env.DD_APPS_UPLOAD_ASSETS;
@@ -116,7 +39,7 @@ describe('Apps Plugin - validateOptions', () => {
11639
test('Should respect explicit dryRun over env var', () => {
11740
process.env.DATADOG_APPS_UPLOAD_ASSETS = '1';
11841
try {
119-
const result = validateOptions({ apps: { dryRun: true } }, mockLogger);
42+
const result = validateOptions({ apps: { dryRun: true } });
12043
expect(result.dryRun).toBe(true);
12144
} finally {
12245
delete process.env.DATADOG_APPS_UPLOAD_ASSETS;
@@ -126,21 +49,17 @@ describe('Apps Plugin - validateOptions', () => {
12649

12750
describe('overrides', () => {
12851
test('Should keep provided options and trim identifier', () => {
129-
const result = validateOptions(
130-
{
131-
apps: {
132-
dryRun: true,
133-
enable: true,
134-
include: ['public/**/*', 'dist/**/*'],
135-
identifier: ' my-app ',
136-
},
52+
const result = validateOptions({
53+
apps: {
54+
dryRun: true,
55+
enable: true,
56+
include: ['public/**/*', 'dist/**/*'],
57+
identifier: ' my-app ',
13758
},
138-
mockLogger,
139-
);
59+
});
14060

14161
expect(result).toEqual({
14262
dryRun: true,
143-
enable: true,
14463
include: ['public/**/*', 'dist/**/*'],
14564
identifier: 'my-app',
14665
name: undefined,

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

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

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

98
import { CONFIG_KEY } from './constants';
109
import type { AppsOptions, AppsOptionsWithDefaults } from './types';
1110

12-
export const validateOptions = (options: Options, log: Logger): AppsOptionsWithDefaults => {
11+
export const validateOptions = (options: Options): AppsOptionsWithDefaults => {
1312
const resolvedOptions = (options[CONFIG_KEY] || {}) as AppsOptions;
1413

15-
const validatedOptions: AppsOptionsWithDefaults = {
16-
enable: resolveEnable(options, CONFIG_KEY, log),
14+
return {
1715
include: resolvedOptions.include || [],
1816
dryRun: resolvedOptions.dryRun ?? !getDDEnvValue('APPS_UPLOAD_ASSETS'),
1917
identifier: resolvedOptions.identifier?.trim(),
2018
name: resolvedOptions.name?.trim() || options.metadata?.name?.trim(),
2119
};
22-
23-
return validatedOptions;
2420
};

‎packages/plugins/error-tracking/src/index.test.ts‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,8 @@ const uploadSourcemapsMock = jest.mocked(uploadSourcemaps);
1717

1818
describe('Error Tracking Plugin', () => {
1919
describe('getPlugins', () => {
20-
test('Should not initialize the plugin if not enabled', async () => {
21-
expect(getPlugins(getGetPluginsArg({ errorTracking: { enable: false } }))).toHaveLength(
22-
0,
23-
);
24-
expect(getPlugins(getGetPluginsArg())).toHaveLength(0);
25-
});
26-
27-
test('Should initialize the plugin if enabled', async () => {
28-
expect(
29-
getPlugins(getGetPluginsArg({ errorTracking: { enable: true } })).length,
30-
).toBeGreaterThan(0);
20+
test('Should initialize the plugin', async () => {
21+
expect(getPlugins(getGetPluginsArg({ errorTracking: {} })).length).toBeGreaterThan(0);
3122
});
3223
});
3324

0 commit comments

Comments
 (0)