Skip to content

Commit 722292f

Browse files
mmalerbathePunderWoman
authored andcommitted
refactor(forms): improve typing on min & max (angular#65212)
If we're calling `min` on a path that's guaranteed to be `number` we don't want to make the users validator function handle the `null` or `string` cases. This uncovered an issue in the `SchemaTreePath` type which needed to be fixed by preventing the model type from being distributed over. PR Close angular#65212
1 parent 4f8ab4f commit 722292f

5 files changed

Lines changed: 32 additions & 27 deletions

File tree

goldens/public-api/forms/signals/index.api.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ export class MetadataKey<TValue> {
335335
export const MIN: AggregateMetadataKey<number | undefined, number | undefined>;
336336

337337
// @public
338-
export function min<TPathKind extends PathKind = PathKind.Root>(path: SchemaPath<number | string | null, SchemaPathRules.Supported, TPathKind>, minValue: number | LogicFn<number | string | null, number | undefined, TPathKind>, config?: BaseValidatorConfig<number | string | null, TPathKind>): void;
338+
export function min<TValue extends number | string | null, TPathKind extends PathKind = PathKind.Root>(path: SchemaPath<TValue, SchemaPathRules.Supported, TPathKind>, minValue: number | LogicFn<TValue, number | undefined, TPathKind>, config?: BaseValidatorConfig<TValue, TPathKind>): void;
339339

340340
// @public
341341
export const MIN_LENGTH: AggregateMetadataKey<number | undefined, number | undefined>;
@@ -505,7 +505,7 @@ export namespace SchemaPathRules {
505505
}
506506

507507
// @public
508-
export type SchemaPathTree<TModel, TPathKind extends PathKind = PathKind.Root> = (TModel extends AbstractControl ? CompatSchemaPath<TModel, TPathKind> : SchemaPath<TModel, SchemaPathRules.Supported, TPathKind>) & (TModel extends AbstractControl ? unknown : TModel extends Array<any> ? unknown : TModel extends Record<string, any> ? {
508+
export type SchemaPathTree<TModel, TPathKind extends PathKind = PathKind.Root> = ([TModel] extends [AbstractControl] ? CompatSchemaPath<TModel, TPathKind> : SchemaPath<TModel, SchemaPathRules.Supported, TPathKind>) & (TModel extends AbstractControl ? unknown : TModel extends Array<any> ? unknown : TModel extends Record<string, any> ? {
509509
[K in keyof TModel]: MaybeSchemaPathTree<TModel[K], PathKind.Child>;
510510
} : unknown);
511511

packages/forms/signals/src/api/types.ts

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -405,23 +405,25 @@ export type CompatSchemaPath<
405405
*
406406
* @experimental 21.0.0
407407
*/
408-
export type SchemaPathTree<
409-
TModel,
410-
TPathKind extends PathKind = PathKind.Root,
411-
> = (TModel extends AbstractControl
412-
? CompatSchemaPath<TModel, TPathKind>
413-
: SchemaPath<TModel, SchemaPathRules.Supported, TPathKind>) &
414-
// Subpaths
415-
(TModel extends AbstractControl
416-
? unknown
417-
: // Array paths have no subpaths
418-
TModel extends Array<any>
408+
export type SchemaPathTree<TModel, TPathKind extends PathKind = PathKind.Root> =
409+
// Note: We use `[TModel]` here to avoid distributing over a union type model.
410+
// (e.g. if we have a model of `number | string`, we want a `SchemaPath<number | string>`,
411+
// not a `SchemaPath<number> | SchemaPath<string>`.
412+
// See https://typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types)
413+
([TModel] extends [AbstractControl]
414+
? CompatSchemaPath<TModel, TPathKind>
415+
: SchemaPath<TModel, SchemaPathRules.Supported, TPathKind>) &
416+
// Subpaths
417+
(TModel extends AbstractControl
419418
? unknown
420-
: // Object subfields
421-
TModel extends Record<string, any>
422-
? {[K in keyof TModel]: MaybeSchemaPathTree<TModel[K], PathKind.Child>}
423-
: // Primitive or other type - no subpaths
424-
unknown);
419+
: // Array paths have no subpaths
420+
TModel extends Array<any>
421+
? unknown
422+
: // Object subfields
423+
TModel extends Record<string, any>
424+
? {[K in keyof TModel]: MaybeSchemaPathTree<TModel[K], PathKind.Child>}
425+
: // Primitive or other type - no subpaths
426+
unknown);
425427

426428
/**
427429
* Helper type for defining `FieldPath`. Given a type `TValue` that may include `undefined`, it

packages/forms/signals/src/api/validators/min.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,13 @@ import {BaseValidatorConfig, getOption, isEmpty} from './util';
2929
* @category validation
3030
* @experimental 21.0.0
3131
*/
32-
export function min<TPathKind extends PathKind = PathKind.Root>(
33-
path: SchemaPath<number | string | null, SchemaPathRules.Supported, TPathKind>,
34-
minValue: number | LogicFn<number | string | null, number | undefined, TPathKind>,
35-
config?: BaseValidatorConfig<number | string | null, TPathKind>,
32+
export function min<
33+
TValue extends number | string | null,
34+
TPathKind extends PathKind = PathKind.Root,
35+
>(
36+
path: SchemaPath<TValue, SchemaPathRules.Supported, TPathKind>,
37+
minValue: number | LogicFn<TValue, number | undefined, TPathKind>,
38+
config?: BaseValidatorConfig<TValue, TPathKind>,
3639
) {
3740
const MIN_MEMO = metadata(path, (ctx) =>
3841
computed(() => (typeof minValue === 'number' ? minValue : minValue(ctx))),

packages/forms/signals/test/node/api/validators/min.spec.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ describe('min validator', () => {
5959
(p) => {
6060
min(p.age, 5, {
6161
error: ({value}) => {
62-
return customError({kind: 'special-min', message: value()?.toString()});
62+
return customError({kind: 'special-min', message: value().toString()});
6363
},
6464
});
6565
},
@@ -84,7 +84,7 @@ describe('min validator', () => {
8484
error: ({value}) => {
8585
return {
8686
kind: 'special-min',
87-
message: value()?.toString(),
87+
message: value().toString(),
8888
};
8989
},
9090
});
@@ -130,7 +130,7 @@ describe('min validator', () => {
130130
error: ({value, valueOf}) => {
131131
return valueOf(p.name) === 'disabled'
132132
? []
133-
: customError({kind: 'special-min', message: value()?.toString()});
133+
: customError({kind: 'special-min', message: value().toString()});
134134
},
135135
});
136136
},

packages/forms/signals/test/node/field_node.spec.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import {
1313
applyEach,
1414
customError,
1515
disabled,
16-
SchemaPath,
1716
form,
1817
hidden,
1918
readonly,
@@ -23,6 +22,7 @@ import {
2322
Schema,
2423
schema,
2524
SchemaOrSchemaFn,
25+
SchemaPath,
2626
SchemaPathTree,
2727
validate,
2828
validateTree,
@@ -1228,7 +1228,7 @@ describe('FieldNode', () => {
12281228
});
12291229

12301230
it('should error on resolving predefined schema path that is not part of the form', () => {
1231-
let otherP: SchemaPath<any>;
1231+
let otherP: SchemaPath<string>;
12321232
const s = schema<string>((p) => (otherP = p));
12331233
SchemaImpl.rootCompile(s);
12341234

0 commit comments

Comments
 (0)