fix(forms): typed argument for FormBuilder group #26985

cexbrayat · 2018-11-07T19:59:58Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

The refactored group method of FormBuilder introduced by #24599 allows two types of options but the signature doesn't show it.

What is the new behavior?

It adds a stricter type to the refactored group method.

Does this PR introduce a breaking change?

Yes
No

Other information

cc @kara

IgorMinar

@kara can you take a look too please? Thanks

IgorMinar · 2018-11-08T09:15:34Z

packages/forms/src/form_builder.ts

+        validators = legacyOrOpts.validators != null ? legacyOrOpts.validators : null;
+        asyncValidators =
+            legacyOrOpts.asyncValidators != null ? legacyOrOpts.asyncValidators : null;
+        updateOn = legacyOrOpts.updateOn != null ? legacyOrOpts.updateOn : undefined;


Why do we need all the ternary operator checks? Couldn't we use object destructuring instead? It seems that we are doing a lot of work no obvious reason.

To support the legacy options of shape

{ asyncValidator?: AsyncValidatorFn | AsyncValidatorFn[] | null; validator?: ValidatorFn | ValidatorFn[] | null; }

as well as AbstractControlOptions with similar but slightly differently named properties (notice the ending s)

{ asyncValidators?: AsyncValidatorFn | AsyncValidatorFn[] | null; validators?: ValidatorFn | ValidatorFn[] | null; updateOn?: 'change' | 'blur' | 'submit'; }

IgorMinar · 2018-11-08T09:16:19Z

packages/forms/src/form_builder.ts

-      FormGroup {
+  group(
+      controlsConfig: {[key: string]: any},
+      legacyOrOpts: AbstractControlOptions|{[key: string]: any}|null = null): FormGroup {


I'm now realizing that this param name is very confusing. I think we should just call it options and document that it has two shapes one of which is deprecated.

Works for me

IgorMinar · 2018-11-08T09:17:27Z

tools/public_api_guard/forms/forms.d.ts

@@ -205,7 +205,7 @@ export declare class FormBuilder {
    control(formState: any, validatorOrOpts?: ValidatorFn | ValidatorFn[] | AbstractControlOptions | null, asyncValidator?: AsyncValidatorFn | AsyncValidatorFn[] | null): FormControl;
    group(controlsConfig: {
        [key: string]: any;
-    }, legacyOrOpts?: {
+      }, legacyOrOpts?: AbstractControlOptions | {


I like this. Change. At minimum from docs perspective it makes the api cleaner.

Yeah, in terms of types, there isn't really a difference, but I do think this is helpful documentation.

@IgorMinar Should legacyOrOpts be replaced with options here, too?

LayZeeDK · 2018-11-12T09:24:07Z

@kara Note that type declarations were previously made less strict because of internal Google tests.

kara

LGTM (once Igor's comments are addressed)

kara · 2018-11-21T00:02:36Z

packages/forms/src/form_builder.ts

-      FormGroup {
+  group(
+      controlsConfig: {[key: string]: any},
+      legacyOrOpts: AbstractControlOptions|{[key: string]: any}|null = null): FormGroup {


Works for me

cexbrayat · 2018-11-21T08:27:29Z

@kara Thank you for your review. I renamed legacyOrOpts to options as requested by @IgorMinar and amended the commit.

skreborn

Just casual nitpicking following up on the legacyOrOpts -> options change.

skreborn · 2018-11-21T09:12:00Z

packages/forms/src/form_builder.ts

-      updateOn = legacyOrOpts.updateOn != null ? legacyOrOpts.updateOn : undefined;
+    if (options != null) {
+      if (isAbstractControlOptions(options)) {
+        // `legacyOrOpts` are `AbstractControlOptions`


legacyOrOpts should also be replaced in the comment here since it was renamed to options.

Right, thanks for catching that. I fixed.

skreborn · 2018-11-21T09:12:17Z

packages/forms/src/form_builder.ts

+        asyncValidators = options.asyncValidators != null ? options.asyncValidators : null;
+        updateOn = options.updateOn != null ? options.updateOn : undefined;
+      } else {
+        // `legacyOrOpts` are legacy form group options


Same as above.

skreborn · 2018-11-21T09:13:35Z

tools/public_api_guard/forms/forms.d.ts

@@ -205,7 +205,7 @@ export declare class FormBuilder {
    control(formState: any, validatorOrOpts?: ValidatorFn | ValidatorFn[] | AbstractControlOptions | null, asyncValidator?: AsyncValidatorFn | AsyncValidatorFn[] | null): FormControl;
    group(controlsConfig: {
        [key: string]: any;
-    }, legacyOrOpts?: {
+      }, legacyOrOpts?: AbstractControlOptions | {


@IgorMinar Should legacyOrOpts be replaced with options here, too?

LayZeeDK · 2018-11-21T10:35:10Z

Please note my comment and make sure that you don't recreate the issue that broke your internal Google tests, @kara and @IgorMinar
#26985 (comment)

This is the reason types were made less strict and correct.

kara · 2018-11-21T18:53:24Z

@LayZeeDK This PR does not actually make any meaningful type changes because AbstractControlOptions is a subtype of {[key:string]: any} type and does not replace it in the signature. It's mostly being added for documentation purposes.

kara · 2018-11-21T18:53:37Z

presubmit

kara

LGTM

kara · 2018-11-21T18:57:55Z

@IgorMinar Can you take another look?

LayZeeDK · 2018-11-22T06:38:00Z

So Angular 7.1.0 landed. This will be in which version now? 7.2.0? 7.1.x?

IgorMinar

Lgtm. Thanks for being patient!

PR Close angular#26985

angular-automatic-lock-bot · 2019-09-14T10:21:26Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Nov 7, 2018

kara added the area: forms label Nov 7, 2018

ngbot bot added this to the needsTriage milestone Nov 7, 2018

cexbrayat force-pushed the fix/type-safe-group branch from f22fba3 to 1d25da2 Compare November 7, 2018 23:51

cexbrayat mentioned this pull request Nov 7, 2018

docs(forms): update description for FormBuilder.group #26970

Closed

IgorMinar reviewed Nov 8, 2018

View reviewed changes

IgorMinar requested a review from kara November 8, 2018 09:19

kara approved these changes Nov 21, 2018

View reviewed changes

kara assigned cexbrayat Nov 21, 2018

kara added the action: cleanup label Nov 21, 2018

cexbrayat force-pushed the fix/type-safe-group branch from 1d25da2 to fac1fdb Compare November 21, 2018 08:24

skreborn suggested changes Nov 21, 2018

View reviewed changes

cexbrayat force-pushed the fix/type-safe-group branch from fac1fdb to 00feb89 Compare November 21, 2018 09:42

fix(forms): typed argument for FormBuilder group

60824ff

cexbrayat force-pushed the fix/type-safe-group branch from 00feb89 to 60824ff Compare November 21, 2018 09:52

kara approved these changes Nov 21, 2018

View reviewed changes

kara assigned IgorMinar and unassigned cexbrayat Nov 21, 2018

kara added action: review target: major and removed action: cleanup labels Nov 21, 2018

IgorMinar approved these changes Dec 7, 2018

View reviewed changes

IgorMinar added the action: merge label Dec 7, 2018

IgorMinar removed the action: review label Dec 7, 2018

alxhub closed this in b0c7561 Dec 7, 2018

ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

fix(forms): typed argument for FormBuilder group (angular#26985)

ee50f1e

PR Close angular#26985

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(forms): typed argument for FormBuilder group #26985

fix(forms): typed argument for FormBuilder group #26985

cexbrayat commented Nov 7, 2018

IgorMinar left a comment

IgorMinar Nov 8, 2018

LayZeeDK Nov 12, 2018

IgorMinar Nov 8, 2018

kara Nov 21, 2018

IgorMinar Nov 8, 2018

kara Nov 21, 2018

skreborn Nov 21, 2018

LayZeeDK commented Nov 12, 2018 •

edited

Loading

kara left a comment •

edited

Loading

kara Nov 21, 2018

cexbrayat commented Nov 21, 2018

skreborn left a comment

skreborn Nov 21, 2018

cexbrayat Nov 21, 2018

skreborn Nov 21, 2018

skreborn Nov 21, 2018

LayZeeDK commented Nov 21, 2018

kara commented Nov 21, 2018 •

edited

Loading

kara commented Nov 21, 2018

kara left a comment

kara commented Nov 21, 2018

LayZeeDK commented Nov 22, 2018 •

edited

Loading

IgorMinar left a comment

angular-automatic-lock-bot bot commented Sep 14, 2019

fix(forms): typed argument for FormBuilder group #26985

fix(forms): typed argument for FormBuilder group #26985

Conversation

cexbrayat commented Nov 7, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LayZeeDK commented Nov 12, 2018 • edited Loading

kara left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cexbrayat commented Nov 21, 2018

skreborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LayZeeDK commented Nov 21, 2018

kara commented Nov 21, 2018 • edited Loading

kara commented Nov 21, 2018

kara left a comment

Choose a reason for hiding this comment

kara commented Nov 21, 2018

LayZeeDK commented Nov 22, 2018 • edited Loading

IgorMinar left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 14, 2019

LayZeeDK commented Nov 12, 2018 •

edited

Loading

kara left a comment •

edited

Loading

kara commented Nov 21, 2018 •

edited

Loading

LayZeeDK commented Nov 22, 2018 •

edited

Loading