Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit c3789e4

Browse files
fix(@angular/cli): apply default to array types
This commit fixes an issue where the `default` option was not being applied to `array` type options in yargs. This seemingly minor change required refactoring in some tests, which revealed that a `.coerce` validation was incorrectly throwing an error on failure. The validation logic was moved to a `.check` to ensure proper error handling and prevent unexpected failures.
1 parent 6c64560 commit c3789e4

File tree

2 files changed

+113
-123
lines changed

2 files changed

+113
-123
lines changed

‎packages/angular/cli/src/command-builder/utilities/json-schema.ts‎

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,33 @@ export interface Option extends YargsOptions {
5151
itemValueType?: 'string';
5252
}
5353

54+
function checkStringMap(keyValuePairOptions: Set<string>, args: Arguments): boolean {
55+
for (const key of keyValuePairOptions) {
56+
const value = args[key];
57+
if (!Array.isArray(value)) {
58+
// Value has been parsed.
59+
continue;
60+
}
61+
62+
for (const pair of value) {
63+
if (pair === undefined) {
64+
continue;
65+
}
66+
67+
if (!pair.includes('=')) {
68+
throw new Error(
69+
`Invalid value for argument: ${key}, Given: '${pair}', Expected key=value pair`,
70+
);
71+
}
72+
}
73+
}
74+
75+
return true;
76+
}
77+
5478
function coerceToStringMap(
55-
dashedName: string,
5679
value: (string | undefined)[],
57-
): Record<string, string> | Promise<never> {
80+
): Record<string, string> | (string|undefined)[] {
5881
const stringMap: Record<string, string> = {};
5982
for (const pair of value) {
6083
// This happens when the flag isn't passed at all.
@@ -64,18 +87,12 @@ function coerceToStringMap(
6487

6588
const eqIdx = pair.indexOf('=');
6689
if (eqIdx === -1) {
67-
// TODO: Remove workaround once yargs properly handles thrown errors from coerce.
68-
// Right now these sometimes end up as uncaught exceptions instead of proper validation
69-
// errors with usage output.
70-
return Promise.reject(
71-
new Error(
72-
`Invalid value for argument: ${dashedName}, Given: '${pair}', Expected key=value pair`,
73-
),
74-
);
90+
// In the case it is not valid skip processing this option and handle the error in `checkStringMap`
91+
return value;
7592
}
93+
7694
const key = pair.slice(0, eqIdx);
77-
const value = pair.slice(eqIdx + 1);
78-
stringMap[key] = value;
95+
stringMap[key] = pair.slice(eqIdx + 1);
7996
}
8097

8198
return stringMap;
@@ -184,6 +201,7 @@ export async function parseJsonSchemaToOptions(
184201
if (current.default !== undefined) {
185202
switch (types[0]) {
186203
case 'string':
204+
case 'array':
187205
if (typeof current.default == 'string') {
188206
defaultValue = current.default;
189207
}
@@ -308,7 +326,7 @@ export function addSchemaOptionsToCommand<T>(
308326
}
309327

310328
if (itemValueType) {
311-
keyValuePairOptions.add(name);
329+
keyValuePairOptions.add(dashedName);
312330
}
313331

314332
const sharedOptions: YargsOptions & PositionalOptions = {
@@ -317,7 +335,7 @@ export function addSchemaOptionsToCommand<T>(
317335
description,
318336
deprecated,
319337
choices,
320-
coerce: itemValueType ? coerceToStringMap.bind(null,dashedName) : undefined,
338+
coerce: itemValueType ? coerceToStringMap : undefined,
321339
// This should only be done when `--help` is used otherwise default will override options set in angular.json.
322340
...(includeDefaultValues ? { default: defaultVal } : {}),
323341
};
@@ -341,6 +359,11 @@ export function addSchemaOptionsToCommand<T>(
341359
}
342360
}
343361

362+
// Valid key/value options
363+
if (keyValuePairOptions.size) {
364+
localYargs.check(checkStringMap.bind(null, keyValuePairOptions), false);
365+
}
366+
344367
// Handle options which have been defined in the schema with `no` prefix.
345368
if (booleanOptionsWithNoPrefix.size) {
346369
localYargs.middleware((options: Arguments) => {

‎packages/angular/cli/src/command-builder/utilities/json-schema_spec.ts‎

Lines changed: 76 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -6,95 +6,61 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import { json,schema } from '@angular-devkit/core';
9+
import { schema } from '@angular-devkit/core';
1010
import yargs from 'yargs';
1111

1212
import { addSchemaOptionsToCommand, parseJsonSchemaToOptions } from './json-schema';
1313

14-
const YError = (() => {
15-
try {
16-
const y = yargs().strict().fail(false).exitProcess(false).parse(['--forced-failure']);
17-
} catch (e) {
18-
if (!(e instanceof Error)) {
19-
throw new Error('Unexpected non-Error thrown');
20-
}
21-
22-
return e.constructor as typeof Error;
23-
}
24-
throw new Error('Expected parse to fail');
25-
})();
26-
27-
interface ParseFunction {
28-
(argv: string[]): unknown;
29-
}
30-
31-
function withParseForSchema(
32-
jsonSchema: json.JsonObject,
33-
{
34-
interactive = true,
35-
includeDefaultValues = true,
36-
}: { interactive?: boolean; includeDefaultValues?: boolean } = {},
37-
): ParseFunction {
38-
let actualParse: ParseFunction = () => {
39-
throw new Error('Called before init');
40-
};
41-
const parse: ParseFunction = (args) => {
42-
return actualParse(args);
43-
};
44-
45-
beforeEach(async () => {
46-
const registry = new schema.CoreSchemaRegistry();
47-
const options = await parseJsonSchemaToOptions(registry, jsonSchema, interactive);
48-
49-
actualParse = async (args: string[]) => {
50-
// Create a fresh yargs for each call. The yargs object is stateful and
51-
// calling .parse multiple times on the same instance isn't safe.
52-
const localYargs = yargs().exitProcess(false).strict().fail(false);
53-
addSchemaOptionsToCommand(localYargs, options, includeDefaultValues);
54-
14+
describe('parseJsonSchemaToOptions', () => {
15+
describe('without required fields in schema', () => {
16+
const parse = async (args: string[]) => {
5517
// Yargs only exposes the parse errors as proper errors when using the
5618
// callback syntax. This unwraps that ugly workaround so tests can just
5719
// use simple .toThrow/.toEqual assertions.
5820
return localYargs.parseAsync(args);
5921
};
60-
});
61-
62-
return parse;
63-
}
6422

65-
describe('parseJsonSchemaToOptions', () => {
66-
describe('without required fields in schema', () => {
67-
const parse = withParseForSchema({
68-
'type': 'object',
69-
'properties': {
70-
'maxSize': {
71-
'type': 'number',
72-
},
73-
'ssr': {
74-
'type': 'string',
75-
'enum': ['always', 'surprise-me', 'never'],
76-
},
77-
'arrayWithChoices': {
78-
'type': 'array',
79-
'items': {
80-
'type': 'string',
81-
'enum': ['always', 'never'],
23+
let localYargs: yargs.Argv<unknown>;
24+
beforeEach(async () => {
25+
// Create a fresh yargs for each call. The yargs object is stateful and
26+
// calling .parse multiple times on the same instance isn't safe.
27+
localYargs = yargs().exitProcess(false).strict().fail(false).wrap(1_000);
28+
const jsonSchema = {
29+
'type': 'object',
30+
'properties': {
31+
'maxSize': {
32+
'type': 'number',
8233
},
83-
},
84-
'extendable': {
85-
'type': 'object',
86-
'properties': {},
87-
'additionalProperties': {
34+
'ssr': {
8835
'type': 'string',
36+
'enum': ['always', 'surprise-me', 'never'],
8937
},
90-
},
91-
'someDefine': {
92-
'type': 'object',
93-
'additionalProperties': {
94-
'type': 'string',
38+
'arrayWithChoices': {
39+
'type': 'array',
40+
'default': 'default-array',
41+
'items': {
42+
'type': 'string',
43+
'enum': ['always', 'never', 'default-array'],
44+
},
45+
},
46+
'extendable': {
47+
'type': 'object',
48+
'properties': {},
49+
'additionalProperties': {
50+
'type': 'string',
51+
},
52+
},
53+
'someDefine': {
54+
'type': 'object',
55+
'additionalProperties': {
56+
'type': 'string',
57+
},
9558
},
9659
},
97-
},
60+
};
61+
const registry = new schema.CoreSchemaRegistry();
62+
const options = await parseJsonSchemaToOptions(registry, jsonSchema, false);
63+
addSchemaOptionsToCommand(localYargs, options, true);
9864
});
9965

10066
describe('type=number', () => {
@@ -123,6 +89,10 @@ describe('parseJsonSchemaToOptions', () => {
12389
/Argument:array-with-choices,Given:"yes",Choices:/,
12490
);
12591
});
92+
93+
it('should add default value to help', async () => {
94+
expect(await localYargs.getHelp()).toContain('[default: "default-array"]');
95+
});
12696
});
12797

12898
describe('type=string, enum', () => {
@@ -150,11 +120,9 @@ describe('parseJsonSchemaToOptions', () => {
150120

151121
it('rejects invalid values for string maps', async () => {
152122
await expectAsync(parse(['--some-define', 'foo'])).toBeRejectedWithError(
153-
YError,
154123
/Invalidvalueforargument:some-define,Given:'foo',Expectedkey=valuepair/,
155124
);
156125
await expectAsync(parse(['--some-define', '42'])).toBeRejectedWithError(
157-
YError,
158126
/Invalidvalueforargument:some-define,Given:'42',Expectedkey=valuepair/,
159127
);
160128
});
@@ -187,43 +155,42 @@ describe('parseJsonSchemaToOptions', () => {
187155

188156
describe('with required positional argument', () => {
189157
it('marks the required argument as required', async () => {
190-
const jsonSchema = JSON.parse(`
191-
{
192-
"$id": "FakeSchema",
193-
"title": "Fake Schema",
194-
"type": "object",
195-
"required": ["a"],
196-
"properties": {
197-
"b": {
198-
"type": "string",
199-
"description": "b.",
200-
"$default": {
201-
"$source": "argv",
202-
"index": 1
203-
}
158+
const jsonSchema = {
159+
'$id': 'FakeSchema',
160+
'title': 'Fake Schema',
161+
'type': 'object',
162+
'required': ['a'],
163+
'properties': {
164+
'b': {
165+
'type': 'string',
166+
'description': 'b.',
167+
'$default': {
168+
'$source': 'argv',
169+
'index': 1,
170+
},
204171
},
205-
"a": {
206-
"type": "string",
207-
"description": "a.",
208-
"$default": {
209-
"$source": "argv",
210-
"index": 0
211-
}
172+
'a': {
173+
'type': 'string',
174+
'description': 'a.',
175+
'$default': {
176+
'$source': 'argv',
177+
'index': 0,
178+
},
212179
},
213-
"optC": {
214-
"type": "string",
215-
"description": "optC"
180+
'optC': {
181+
'type': 'string',
182+
'description': 'optC',
216183
},
217-
"optA": {
218-
"type": "string",
219-
"description": "optA"
184+
'optA': {
185+
'type': 'string',
186+
'description': 'optA',
187+
},
188+
'optB': {
189+
'type': 'string',
190+
'description': 'optB',
220191
},
221-
"optB": {
222-
"type": "string",
223-
"description": "optB"
224-
}
225-
}
226-
}`) as json.JsonObject;
192+
},
193+
};
227194
const registry = new schema.CoreSchemaRegistry();
228195
const options = await parseJsonSchemaToOptions(registry, jsonSchema, /* interactive= */ true);
229196

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /