feat(forms): make reactive forms strongly typed

[KostyaTretyak]

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?

• Feature

What is the current behavior?

Weak typechecking for reactive forms.

Issue Number: #13721

Does this PR introduce a breaking change?

• Yes
• No

[KostyaTretyak]

@AndrewKushnir, please tell me why you set "flag: breaking change"?

[KostyaTretyak]

Good news! After sleep, I see why "flag: breaking change" is set here =). This is not a runtime breaking change, but only by type.

[AndrewKushnir]

Hi @KostyaTretyak, thanks for creating this PR! We will review it and provide the feedback.

please tell me why you set "flag: breaking change"?

I've added the "flag: breaking change" flag since there are changes to the public API that may not be backwards-compatible. For example this change to the status field type from string -> enum with the set of specific values looks potentially backwards-incompatible (there might be more cases, I can provide additional info after detailed review). The "breaking change" flag is an indication that the changes should be processed in a certain way (special release notes, running a full test suite in Google codebase, etc) and be included into the major Angular versions.

Thank you.

[KostyaTretyak]

1. I follow Submitting a Pull Request (PR) and Running Tests Locally.
2. I found that ci/circleci: components-repo-unit-tests I can't fix because this error is about angular/components repo
3. It seems that the following errors are actually the same error (with same commant yarn --cwd aio e2e --configuration=ci):

• ci/circleci: test_aio
• ci/circleci: test_aio_local
• ci/circleci: test_aio_local_viewengine

I tried to run yarn --cwd aio e2e --configuration = ci and yarn --cwd aio e2e, but my tests failed with the following error:

yarn run v1.22.4
$ yarn check-env && yarn update-webdriver
$ yarn ~~check-env
$ node scripts/check-environment
$ yarn aio-check-local
$ node tools/ng-packages-installer check .
$ node ../scripts/webdriver-manager-update.js
error Command "webdriver-manager" not found.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
[webdriver-manager-update.js] Call to 'yarn webdriver-manager update --gecko=false --standalone=false --versions.chrome 80.0.3987.0' failed with error code 1
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

If anyone knows how to fix this, please tell me how.

[KostyaTretyak]

It remains to fix one error. This error, as far as I understand, should be fixed on the angular/components repo.

[jonkoops]

@KostyaTretyak If you need any help with this PR I am willing to help out. Perhaps my PR for supporting a generic type for ControlValueAccessor can be incorporated here (see: #31801)?

[SchnWalter]

I tried to run yarn --cwd aio e2e --configuration = ci and yarn --cwd aio e2e, but my tests failed with the following error:
...
error Command "webdriver-manager" not found.

@KostyaTretyak, it looks like you also need to run yarn install in the aio directory to get around that error. yarn install in the root directory doesn't seem to be sufficient.

# start with a clean slate, if you want to.
# rm -rf aio/node_modules
yarn --cwd aio install

But then you run into other issues, so you need to also run:

yarn --cwd aio setup-local

And only then, the yarn --cwd aio e2e --configuration=ci passes.

Checkout the "Developer tasks" section in aio/README.md. Maybe these aio specific things should also be mentioned somewhere in the "Running Tests Locally" section of docs/DEVELOPER.md, it would be nice to just follow that section and have all tests running locally. You can follow #33591 to get updates when the docs are going to be updated. // cc: @aikidave (sorry for bugging you twice, I only later realized that I should add a comment onto the linked story)

[AndrewKushnir]

Thanks for the updates in this PR @KostyaTretyak and thanks @SchnWalter for providing feedback
👍

As Kara mentioned in #13721 (comment), we'll start looking into Form type improvements soon (we don't have an exact ETA at this moment). Meanwhile I've collected all types-related PRs in the Forms type improvements Milestone on Github, so we can take them (including comments) into account.

I still want to add form status type and form validation type to the form module. But in the following PR, not in current PR. Although, if the Angular team says it's better to do it in the current PR, then I'm ready to do it. The same goes for ControlValueAccessor.

I think it makes sense to have multiple individual PRs targeting subsets of Forms package, so it's easier to review, test and merge (in master branch as well as in Google codebase internally).

We'll provide more updates as soon as we have them.

Thank you.

[JohnYoungers]

Due to the HTML binding issues noted in issue #36405, it would be nice if the typings on FormGroup and FormArray included the type of control itself.

I.e. instead of:

FormArray - controls: AbstractControl<Item>[];
FormGroup - controls: { [key in keyof T]: AbstractControl<T[key]>; };

The generics were something like: <Item, Control extends AbstractControl<Item>>

If possible I think that would resolve a lot of work-arounds currently needed, especially since it appears angular 10 is more aggressive with strictTemplates

[KostyaTretyak]

My opinion on this can be read in this discussion.

[AndrewKushnir]

Hi @KostyaTretyak,

Quick update: while this task is in our Backlog at this moment (i.e. we have not started active work on it) I ran tests in Google's codebase with the changes from this PR. As we expected, it's breaking a relatively big number of apps, so landing this PR with the current set of changes would be difficult (since that would require adapting apps code to this change). This also indicates that landing this PR on NPM (as a part of a major release) would require Forms users to adapt their apps during the upgrade as well. This is expected for some breaking changes (where creating a schematic is not feasible or a breaking change is affecting a small portion of the codebase and the fix is straightforward), but we always try to minimize the impact of breaking changes to provide a smooth upgrade experience.

The changes in this PR are very useful and I'd like to propose splitting this PR into a set of smaller PRs and try to land them in the following sequence (each group might have more than one PR):

• Backwards-compatible changes. These changes should not require any work from apps authors during the upgrade.
• Breaking changes but with limited impact and straightforward fix(es). Good example of these changes can be found in PR fix(forms): include null in return types of .parent of abstract control #32671.
• Breaking changes with high impact affecting a large portion of the codebase and/or where the fix is not straightforward. For these changes we should perform an assessment to see how we can land them best to ensure smooth upgrades.

Please let me know your opinion on how the changes in this PR can be split and whether we can extract some backwards-compatible changes into separate PR(s).

Thank you.

[KostyaTretyak]

Unfortunately, I will not be able to take the time to improve this Pull Request in the near future.

[angular-automatic-lock-bot]

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.}