New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into
base: master
from

Conversation

@LayZeeDK
Contributor

LayZeeDK commented Jun 20, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

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.

@googlebot googlebot added the cla: yes label Jun 20, 2018

@LayZeeDK

This comment has been minimized.

Contributor

LayZeeDK commented Jun 20, 2018

Let me know how to proceed from here.

@mhevery mhevery added the comp: forms label Jul 12, 2018

@mhevery mhevery requested a review from kara Jul 12, 2018

@kara

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

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

This comment has been minimized.

@kara

kara Jul 31, 2018

Contributor

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

This comment has been minimized.

@LayZeeDK

LayZeeDK Aug 1, 2018

Contributor

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?

This comment has been minimized.

@kara

kara Aug 1, 2018

Contributor

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.

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

This comment has been minimized.

@JoostK

JoostK Jul 31, 2018

Contributor

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?

This comment has been minimized.

@kara

kara Jul 31, 2018

Contributor

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.

This comment has been minimized.

@JoostK

JoostK Jul 31, 2018

Contributor

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.

This comment has been minimized.

@kara

kara Jul 31, 2018

Contributor

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

This comment has been minimized.

@JoostK

JoostK Jul 31, 2018

Contributor

My bad. I did not spot the additional s.

@LayZeeDK

This comment has been minimized.

Contributor

LayZeeDK commented Aug 21, 2018

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

This comment has been minimized.

Contributor

kara commented Aug 21, 2018

@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

This comment has been minimized.

Contributor

LayZeeDK commented Oct 2, 2018

@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

This comment has been minimized.

Contributor

LayZeeDK commented Oct 3, 2018

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

@kara

This comment has been minimized.

Contributor

kara commented Oct 9, 2018

@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

This comment has been minimized.

Contributor

LayZeeDK commented Oct 11, 2018

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

This comment has been minimized.

Contributor

LayZeeDK commented Oct 11, 2018

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

This comment has been minimized.

Contributor

LayZeeDK commented Oct 11, 2018

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

This comment has been minimized.

Contributor

kara commented Oct 11, 2018

@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 kara requested a review from IgorMinar Oct 11, 2018

@kara

This comment has been minimized.

Contributor

kara commented Oct 11, 2018

@IgorMinar Assigning to you for public API approval.

@LayZeeDK

This comment has been minimized.

Contributor

LayZeeDK commented Oct 12, 2018

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

This comment has been minimized.

Contributor

LayZeeDK commented Oct 14, 2018

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

@kara

This comment has been minimized.

Contributor

kara commented Oct 16, 2018

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

This comment has been minimized.

@kara

kara Oct 17, 2018

Contributor

@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?

This comment has been minimized.

@kara

kara Oct 17, 2018

Contributor

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

This comment has been minimized.

Contributor

LayZeeDK commented Oct 17, 2018

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

@JohnyDaison

This comment has been minimized.

JohnyDaison commented Oct 22, 2018

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

@LayZeeDK

This comment has been minimized.

Contributor

LayZeeDK commented Oct 22, 2018

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.

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

@kara

This comment has been minimized.

Contributor

kara commented Oct 25, 2018

@kara

kara approved these changes Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment