feat(forms): add updateOn option to FormBuilder #24599

LayZeeDK · 2018-06-20T18:48:48Z

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
[x] 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?

Issue Number: #19163

What is the new behavior?

Support updateOn option in FormBuilder methods array, control and group.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Special care has been taken to keep backwards compatibility with the extra object containing optional validator and asyncValidator properties in the FormBuilder#group method so as not to create breaking changes.

Unit tests have been massaged from the form control spec.

LayZeeDK · 2018-06-20T20:24:18Z

Let me know how to proceed from here.

kara

Thanks for the PR! It's looking great, just one comment (also needs a rebase).

kara · 2018-07-31T18:09:23Z

packages/forms/src/form_builder.ts

-import {AbstractControl, FormArray, FormControl, FormGroup} from './model';
+import {AbstractControl, AbstractControlOptions, FormArray, FormControl, FormGroup, FormHooks} from './model';
+
+export interface GroupExtra {


Comment here about what this is? For the name, I think OldFormBuilderOptions is more descriptive.

Seeing that we are now at version 6.1 and that no more minor releases are planned before version 7, do you have any thoughts on adding a deprecation notice to these (soon to become) legacy group options?

Hmm, that's a good point. I think yes, but let's do that in a follow-up PR since we need to design a a full deprecation plan.

JoostK · 2018-07-31T20:44:51Z

packages/forms/src/form_builder.ts

+    if (isGroupExtra(extraOrOpts)) {
+      validators = extraOrOpts.validator != null ? extraOrOpts.validator : null;
+      asyncValidators = extraOrOpts.asyncValidator != null ? extraOrOpts.asyncValidator : null;
+    } else if (extraOrOpts != null) {


This doesn't seem right. If isGroupExtra is satisfied, the updateOn property will not be set. Instead of the GroupExtra interface I would try to either lift on the usage of AbstractControlOptions ~~(which is not public last time I checked) or introduce a public facing interface with all fields being optional.~~ It is public so we should be fine with using it. @kara thoughts?

I think it is right as is. We don't want to keep supporting the GroupExtra API, so I'd prefer not to add options to it.

Agreed, but AbstractControlOptions is fully type-compatible with FormExtra so there shouldn't be a need for the new interface to be added at all. The bug I described above is real.

I don't think it's type compatible. asyncValidator vs asyncValidators and validator vs validators.

My bad. I did not spot the additional s.

LayZeeDK · 2018-08-21T19:41:00Z

Hi @kara,

I renamed the legacy options interface and added a description as per your comment.

To my best ability I have rebased on upstream/master as well.

kara · 2018-08-21T22:36:28Z

@LayZeeDK Can you take a look at the failing CI test? Looks related to the change: https://travis-ci.org/angular/angular/jobs/418858316.

Also you'll want to lint with gulp format to get the linting check green.

LayZeeDK · 2018-10-02T18:53:40Z

@kara You are right. I did not realize that the documentaton example was coupled to an end-to-end test. It should be fixed now, but honestly I have no idea since I am having trouble running the tests on Windows.

LayZeeDK · 2018-10-03T03:54:44Z

It seems that all that is left is to accept a new golden file for the public API guard.

kara · 2018-10-09T22:06:10Z

@LayZeeDK Yes, the last thing needed to get the tests green is to generate a new golden file. You can do so with:

bazel run //tools/public_api_guard:forms_forms_api_bin.accept

LayZeeDK · 2018-10-11T18:13:53Z

What am I doing wrong?

**/angular
$ bazel run //tools/public_api_guard:forms_forms_api_bin.accept
ERROR: Skipping '//tools/public_api_guard:forms_forms_api_bin.accept': no such target '//tools/public_api_guard:forms_forms_api_bin.accept': target 'forms_forms_api_bin.accept' not declared in package 'tools/public_api_guard' (did you mean 'forms_forms_api.accept'?) defined by **/angular/tools/public_api_guard/BUILD.bazel
WARNING: Target pattern parsing failed.
ERROR: no such target '//tools/public_api_guard:forms_forms_api_bin.accept': target 'forms_forms_api_bin.accept' not declared in package 'tools/public_api_guard' (did you mean 'forms_forms_api.accept'?) defined by **/angular/tools/public_api_guard/BUILD.bazel
INFO: Elapsed time: 0,193s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
FAILED: Build did NOT complete successfully (0 packages loaded)

$ bazel version
Build label: 0.17.2
Build target: bazel-out/x64_windows-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri Sep 21 10:31:06 2018 (1537525866)
Build timestamp: 1537525866
Build timestamp as int: 1537525866

LayZeeDK · 2018-10-11T18:27:40Z

Alright, the command seems to be bazel run //tools/public_api_guard:forms_forms_api.accept (without _bin) but fails with this

ERROR: **/_bazel_**/go3n4uez/external/rxjs/BUILD.bazel:7:1: Compiling TypeScript (devmode) @rxjs//:lib failed (Exit 1)
external/rxjs/internal/scheduler/AsyncAction.ts:82:12 - error TS1345: An expression of type 'void' cannot be tested for truthiness

82     return clearInterval(id) && undefined || undefined;
              ~~~~~~~~~~~~~~~~~
external/rxjs/internal/scheduler/AsyncAction.ts:82:12 - error TS1345: An expression of type 'void' cannot be tested for truthiness

82     return clearInterval(id) && undefined || undefined;
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I will try rebasing again and then generate a new golden file.

LayZeeDK · 2018-10-11T19:58:07Z

Please help me out here

$ bazel run //tools/public_api_guard:forms_forms_api.accept
WARNING: Overflow when watching local filesystem for changes... temporarily falling back to manually checking files for changes
ERROR: /mnt/d/**/angular/packages/forms/BUILD.bazel:21:1: no such package '@angular_deps//': yarn_install failed: yarn install v1.9.2
info If you think this is a bug, please open a bug report with the information provided in "/home/lars/.cache/bazel/_bazel_lars/c49625842b1d24dd37283e182df182e8/external/angular_deps/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
 (error An unexpected error occurred: "EACCES: permission denied, scandir '/home/lars/.config/yarn/link'".
) and referenced by '//packages/forms:npm_package'
ERROR: Analysis of target '//tools/public_api_guard:forms_forms_api.accept' failed; build aborted: no such package '@angular_deps//': yarn_install failed: yarn install v1.9.2
info If you think this is a bug, please open a bug report with the information provided in "/home/lars/.cache/bazel/_bazel_lars/c49625842b1d24dd37283e182df182e8/external/angular_deps/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
 (error An unexpected error occurred: "EACCES: permission denied, scandir '/home/lars/.config/yarn/link'".
)
INFO: Elapsed time: 1.157s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
FAILED: Build did NOT complete successfully (0 packages loaded)

kara · 2018-10-11T23:10:38Z

@LayZeeDK Hmm, I'm not sure what's going on there. Maybe a Windows issue with Bazel? (I'm on a Mac, and the command seems to work as expected). I ran the command for you and added the result in a fixup commit so it wouldn't mess with your authorship.

kara · 2018-10-11T23:11:50Z

@IgorMinar Assigning to you for public API approval.

LayZeeDK · 2018-10-12T05:48:19Z

I'm not sure what's going on there. Maybe a Windows issue with Bazel?

I tried it both on Bash, MSYS2, and Ubuntu@Windows Subsystem for Linux.

Thank you for helping me out, @kara! I spent more time wrestling Git, testing tools, and build tools than adding code and tests 😅

IgorMinar

LGTM for api but let's wait until we branch off 7.0.x before we merge this.

LayZeeDK · 2018-10-14T06:31:18Z

@kara Remember a deprecation plan for the legacy form group options in the FormBuilder
#24599 (comment)

kara · 2018-10-16T22:47:29Z

presubmit

kara · 2018-10-16T23:55:13Z

packages/forms/src/form_builder.ts

+ */
+export interface LegacyFormGroupOptions {
+  asyncValidator?: AsyncValidatorFn|AsyncValidatorFn[]|null;
+  validator?: ValidatorFn|ValidatorFn[]|null;


@LayZeeDK Some internal Google tests failed because this new type is more restrictive than it was before. Previously we were technically typing the validator and asyncValidator properties as any, so typing as ValidatorFn is a breaking change for any projects that have strict function types turned on and are using a subtype in their validator function type (e.g. (group: FormGroup) => ValidationErrors|null instead of (control: AbstractControl) => ValidationErrors|null or ValidatorFn).

Technically those types are unsafe and should be updated to match ValidatorFn (which is why the strict function type option will catch it), but I don't think it makes
sense to create a breaking change for a legacy API that we plan to deprecate.

So let's relax it a bit to {validator?: Function|Function[]|null}, etc?

Actually, after chatting with @IgorMinar , I think we need to remove the LegacyFormGroupOptions type altogether and keep the type exactly the same to avoid breaking changes. When we move to AbstractControlOptions, we can remove it.

LayZeeDK · 2018-10-17T09:54:33Z

@kara I need your help to modify the golden file for the public API guard again. Thank you! 🙂

JohnyDaison · 2018-10-22T15:47:47Z

So does this mean this option will only be available in angular 8, or will it also make it into 7?

LayZeeDK · 2018-10-22T18:36:16Z

So does this mean this option will only be available in angular 8, or will it also make it into 7?

Hopefully 7.1 based on @IgorMinar's comment.

kara · 2018-10-25T22:26:01Z

New presubmit here: http://test/OCL:218760407:BASE:218760432:1540506327680:d2e3abe9

@JohnyDaison @LayZeeDK Should be in 7.1, yes.

PR Close angular#24599

LaljiG · 2018-11-26T18:20:15Z

Any firm idea which release this (updateOn via FB group method) will be released in?

LayZeeDK · 2018-11-27T13:13:37Z

Any firm idea which release this (updateOn via FB group method) will be released in?

It was released in @angular/forms@7.1.0.

PR Close angular#24599

angular-automatic-lock-bot · 2019-09-14T08:51:56Z

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 Jun 20, 2018

mhevery added the area: forms label Jul 12, 2018

mhevery requested a review from kara July 12, 2018 22:53

kara reviewed Jul 31, 2018

View reviewed changes

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 31, 2018

kara assigned LayZeeDK Jul 31, 2018

kara added feature Issue that requests a new feature target: major This PR is targeted for the next major release labels Jul 31, 2018

JoostK reviewed Jul 31, 2018

View reviewed changes

LayZeeDK mentioned this pull request Oct 8, 2018

Update FormBuilder#control() to support AbstractControlOptions as 2nd parameter #19163

Closed

kara unassigned LayZeeDK Oct 11, 2018

kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 11, 2018

kara requested a review from IgorMinar October 11, 2018 23:11

IgorMinar approved these changes Oct 13, 2018

View reviewed changes

IgorMinar added the state: blocked label Oct 13, 2018

kara added the target: patch This PR is targeted for the next patch release label Oct 16, 2018

kara removed action: review The PR is still awaiting reviews from at least one requested reviewer state: blocked target: major This PR is targeted for the next major release labels Oct 16, 2018

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 16, 2018

kara suggested changes Oct 17, 2018

View reviewed changes

feat(forms): add updateOn option to FormBuilder

01f5580

kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 25, 2018

ngbot bot added this to the needsTriage milestone Oct 25, 2018

kara added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 25, 2018

kara approved these changes Oct 25, 2018

View reviewed changes

kara added the action: merge The PR is ready for merge by the caretaker label Oct 25, 2018

fixup! feat(forms): add updateOn option to FormBuilder

13c7523

kara closed this in e9e804f Nov 1, 2018

sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018

feat(forms): add updateOn option to FormBuilder (angular#24599)

631833a

PR Close angular#24599

m3hm3tk mentioned this pull request Nov 5, 2018

{ updateOn: 'submit' } doesn't work when using with FormBuilder's group method #20176

Closed

cexbrayat mentioned this pull request Nov 7, 2018

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

Closed

14 tasks

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

feat(forms): add updateOn option to FormBuilder (angular#24599)

a8872f4

PR Close angular#24599

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

feat(forms): add updateOn option to FormBuilder #24599

feat(forms): add updateOn option to FormBuilder #24599

LayZeeDK commented Jun 20, 2018 •

edited

Loading

LayZeeDK commented Jun 20, 2018

kara left a comment •

edited

Loading

kara Jul 31, 2018

LayZeeDK Aug 1, 2018 •

edited

Loading

kara Aug 1, 2018

JoostK Jul 31, 2018 •

edited

Loading

kara Jul 31, 2018

JoostK Jul 31, 2018

kara Jul 31, 2018

JoostK Jul 31, 2018

LayZeeDK commented Aug 21, 2018 •

edited

Loading

kara commented Aug 21, 2018

LayZeeDK commented Oct 2, 2018 •

edited

Loading

LayZeeDK commented Oct 3, 2018

kara commented Oct 9, 2018

LayZeeDK commented Oct 11, 2018 •

edited

Loading

LayZeeDK commented Oct 11, 2018 •

edited

Loading

LayZeeDK commented Oct 11, 2018 •

edited

Loading

kara commented Oct 11, 2018 •

edited

Loading

kara commented Oct 11, 2018

LayZeeDK commented Oct 12, 2018 •

edited

Loading

IgorMinar left a comment

LayZeeDK commented Oct 14, 2018

kara commented Oct 16, 2018

kara Oct 16, 2018 •

edited

Loading

kara Oct 17, 2018 •

edited

Loading

LayZeeDK commented Oct 17, 2018

JohnyDaison commented Oct 22, 2018

LayZeeDK commented Oct 22, 2018 •

edited

Loading

kara commented Oct 25, 2018

LaljiG commented Nov 26, 2018

LayZeeDK commented Nov 27, 2018

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

feat(forms): add updateOn option to FormBuilder #24599

feat(forms): add updateOn option to FormBuilder #24599

Conversation

LayZeeDK commented Jun 20, 2018 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

LayZeeDK commented Jun 20, 2018

kara left a comment • edited Loading

Choose a reason for hiding this comment

kara Jul 31, 2018

Choose a reason for hiding this comment

LayZeeDK Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

kara Aug 1, 2018

Choose a reason for hiding this comment

JoostK Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

kara Jul 31, 2018

Choose a reason for hiding this comment

JoostK Jul 31, 2018

Choose a reason for hiding this comment

kara Jul 31, 2018

Choose a reason for hiding this comment

JoostK Jul 31, 2018

Choose a reason for hiding this comment

LayZeeDK commented Aug 21, 2018 • edited Loading

kara commented Aug 21, 2018

LayZeeDK commented Oct 2, 2018 • edited Loading

LayZeeDK commented Oct 3, 2018

kara commented Oct 9, 2018

LayZeeDK commented Oct 11, 2018 • edited Loading

LayZeeDK commented Oct 11, 2018 • edited Loading

LayZeeDK commented Oct 11, 2018 • edited Loading

kara commented Oct 11, 2018 • edited Loading

kara commented Oct 11, 2018

LayZeeDK commented Oct 12, 2018 • edited Loading

IgorMinar left a comment

Choose a reason for hiding this comment

LayZeeDK commented Oct 14, 2018

kara commented Oct 16, 2018

kara Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

kara Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

LayZeeDK commented Oct 17, 2018

JohnyDaison commented Oct 22, 2018

LayZeeDK commented Oct 22, 2018 • edited Loading

kara commented Oct 25, 2018

LaljiG commented Nov 26, 2018

LayZeeDK commented Nov 27, 2018

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

LayZeeDK commented Jun 20, 2018 •

edited

Loading

kara left a comment •

edited

Loading

LayZeeDK Aug 1, 2018 •

edited

Loading

JoostK Jul 31, 2018 •

edited

Loading

LayZeeDK commented Aug 21, 2018 •

edited

Loading

LayZeeDK commented Oct 2, 2018 •

edited

Loading

LayZeeDK commented Oct 11, 2018 •

edited

Loading

LayZeeDK commented Oct 11, 2018 •

edited

Loading

LayZeeDK commented Oct 11, 2018 •

edited

Loading

kara commented Oct 11, 2018 •

edited

Loading

LayZeeDK commented Oct 12, 2018 •

edited

Loading

kara Oct 16, 2018 •

edited

Loading

kara Oct 17, 2018 •

edited

Loading

LayZeeDK commented Oct 22, 2018 •

edited

Loading