fix(forms): incorrectly keeping track of ngModel with ngFor inside a form by crisbeto · Pull Request #40459 · angular/angular

crisbeto · 2021-01-16T11:24:29Z

When an NgModel is created within a form, it receives an NgControl based on its name, but the control doesn't get swapped out if the name changes. This can lead to problems if the NgModel is part of an ngFor, because the name can change based on its position in the list and a new control can be defined with the same name, leading us to having multiple directives pointing to the same control. For example, if we start off with a list like :

[0, 1, 2]; -> [NgModel(0), NgModel(1), NgModel(2)]

Then we remove the second item:

[0, 2]; -> [NgModel(0), NgModel(2)]

And finally, if we decide to add an item to the end of the list, we'll already have a control for
index 2, causing the list to look like:

[0, 2, 3]; -> [NgModel(0), NgModel(2), NgModel(2)]

These changes fix the issue by removing the old control when the name of the directive changes.

Fixes #38465.
Fixes #37920.

packages/forms/src/directives/ng_form.ts

packages/forms/src/directives/ng_model.ts

AndrewKushnir

Thanks @crisbeto, the change looks good, just left a few comments. Thank you.

packages/forms/test/template_integration_spec.ts

packages/forms/src/directives/ng_model.ts

packages/forms/src/directives/ng_form.ts

AndrewKushnir · 2021-01-17T00:29:51Z

Presubmit (to see if there is any impact on apps in Google's codebase).

AndrewKushnir · 2021-01-17T20:33:48Z

@crisbeto presubmit indicated a few targets that are affected. I didn't have a chance to perform an investigation yet, but I'd like to share the error message in case it's helpful:

        - Uncaught Error: Uncaught (in promise): TypeError: Cannot read property 'registerControl' of null
        TypeError: Cannot read property 'registerControl' of null
            at packages/forms/src/directives/ng_form.ts?l=191
            at ZoneDelegate.invoke (packages/zone.js/lib/zone.ts?l=1148)
            at FakeAsyncTestZoneSpec.onInvoke (packages/zone.js/lib/zone-spec/fake-async-test.ts?l=674)
            at ZoneDelegate.invoke (packages/zone.js/lib/zone.ts?l=1145)
            at Object.onInvoke (packages/core/src/zone/ng_zone.ts?l=364)
            ...
            at ZoneDelegate.invokeTask (packages/zone.js/lib/zone.ts?l=1182)

From the stack trace it's unclear what triggered the error (more investigation is needed).

AndrewKushnir · 2021-01-17T20:46:42Z

packages/forms/test/template_integration_spec.ts

I was thinking of the following scenario:

Suggested change

group = {};

items: {value: string}[] = [];

items: {value: string}[] = [];

group = {items: this.items};

i.e. put items inside the group and test how the fix works with ngModelGroup which would act as a parent instead of NgForm.

crisbeto · 2021-01-17T21:02:09Z

Thank you for the info. If I had to guess, I'd say that the error comes from this call, although I'm not exactly sure how it can happen yet.

dylhunn · 2022-02-04T00:56:10Z

@crisbeto Now that you can run the presubmit, would you be interested in revisiting this for 14? This would be a nice fix!

crisbeto · 2022-02-04T07:35:23Z

Yes, I just need to finish getting some other work submitted first.

…form When an `NgModel` is created within a `form`, it receives an `NgControl` based on its `name`, but the control doesn't get swapped out if the name changes. This can lead to problems if the `NgModel` is part of an `ngFor`, because the name can change based on its position in the list and a new control can be defined with the same name, leading us to having multiple directives pointing to the same control. For example, if we start off with a list like : ``` [0, 1, 2]; -> [NgModel(0), NgModel(1), NgModel(2)] ``` Then we remove the second item: ``` [0, 2]; -> [NgModel(0), NgModel(2)] ``` And finally, if we decide to add an item to the end of the list, we'll already have a control for index 2, causing the list to look like: ``` [0, 2, 3]; -> [NgModel(0), NgModel(2), NgModel(2)] ``` These changes fix the issue by removing the old control when the `name` of the directive changes. Fixes angular#38465. Fixes angular#37920.

crisbeto · 2022-02-04T16:12:44Z

@AndrewKushnir @dylhunn I just ran a presubmit and it passed, except for a handful of already failing targets. Maybe the error mentioned above isn't a problem anymore?

AndrewKushnir · 2022-02-04T17:28:39Z

I just ran a presubmit and it passed, except for a handful of already failing targets. Maybe the error mentioned above isn't a problem anymore?

It was likely a target (or a set of targets) from the global run (TGP). Could you please try running it as well?

dylhunn · 2022-02-04T19:10:46Z

Choo choo, all aboard the TAP train! 🚂 I'll post back with the global presubmit results.

dylhunn · 2022-02-05T22:22:52Z

TAP train rerun looks good: link

crisbeto · 2022-02-06T08:15:15Z

I ran my own global presubmit as well. It has a bunch of breakages, but they're unrelated and they show up in other test runs in other projects as well.

dylhunn · 2022-02-07T16:54:43Z

I ran my own global presubmit as well. It has a bunch of breakages, but they're unrelated and they show up in other test runs in other projects as well.

Indeed, my TGP was clean also -- the only breakages pertain to another PR and were fixed elsewhere.

dylhunn

reviewed-for: fw-forms

dylhunn · 2022-02-07T21:16:13Z

This PR was merged into the repository by commit ebf2fc5.

…form (#40459) When an `NgModel` is created within a `form`, it receives an `NgControl` based on its `name`, but the control doesn't get swapped out if the name changes. This can lead to problems if the `NgModel` is part of an `ngFor`, because the name can change based on its position in the list and a new control can be defined with the same name, leading us to having multiple directives pointing to the same control. For example, if we start off with a list like : ``` [0, 1, 2]; -> [NgModel(0), NgModel(1), NgModel(2)] ``` Then we remove the second item: ``` [0, 2]; -> [NgModel(0), NgModel(2)] ``` And finally, if we decide to add an item to the end of the list, we'll already have a control for index 2, causing the list to look like: ``` [0, 2, 3]; -> [NgModel(0), NgModel(2), NgModel(2)] ``` These changes fix the issue by removing the old control when the `name` of the directive changes. Fixes #38465. Fixes #37920. PR Close #40459

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fanimations/13.2.1/13.2.2) | | [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fcommon/13.2.1/13.2.2) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/13.2.1/13.2.2) | | [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/13.2.1/13.2.2) | | [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fcore/13.2.1/13.2.2) | | [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fforms/13.2.1/13.2.2) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/13.2.1/13.2.2) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/13.2.1/13.2.2) | | [@angular/router](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2frouter/13.2.1/13.2.2) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v13.2.2`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1322-2022-02-08) [Compare Source](angular/angular@13.2.1...13.2.2) ##### compiler | Commit | Type | Description | | -- | -- | -- | | [37af6abb49](angular/angular@37af6ab) | fix | allow banana-in-a-box bindings to end with non-null assertion ([#37809](angular/angular#37809)) | ##### forms | Commit | Type | Description | | -- | -- | -- | | [b75e90f809](angular/angular@b75e90f) | fix | incorrectly keeping track of ngModel with ngFor inside a form ([#40459](angular/angular#40459)) | ##### http | Commit | Type | Description | | -- | -- | -- | | [3fae6637e7](angular/angular@3fae663) | perf | remove IE special status handling ([#44354](angular/angular#44354)) | ##### upgrade | Commit | Type | Description | | -- | -- | -- | | [b9aab0c87b](angular/angular@b9aab0c) | fix | Do not trigger duplicate navigation events from Angular Router ([#43441](angular/angular#43441)) | #### Special Thanks Alan Agius, Alan Cohen, Andrew Kushnir, Andrew Scott, Daniel Díaz, Dario Piotrowicz, Doug Parker, Jayson Acosta, Joey Perrott, JoostK, Kristiyan Kostadinov, Olivier Capuozzo, Ramzan, Shai Reznik, TANMAY SRIVASTAVA, dario-piotrowicz, iRealNirmal, jhonyeduardo, mgechev and zuckjet  </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1160 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

angular-automatic-lock-bot · 2022-03-10T16:11:52Z

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

google-cla bot added the cla: yes label Jan 16, 2021

crisbeto commented Jan 16, 2021

View reviewed changes

packages/forms/src/directives/ng_form.ts Outdated Show resolved Hide resolved

crisbeto marked this pull request as ready for review January 16, 2021 11:58

pullapprove bot requested a review from AndrewKushnir January 16, 2021 11:58

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms target: patch This PR is targeted for the next patch release type: bug/fix labels Jan 16, 2021

ngbot bot modified the milestone: Backlog Jan 16, 2021

crisbeto commented Jan 16, 2021

View reviewed changes

packages/forms/src/directives/ng_model.ts Outdated Show resolved Hide resolved

AndrewKushnir reviewed Jan 17, 2021

View reviewed changes

packages/forms/test/template_integration_spec.ts Outdated Show resolved Hide resolved

packages/forms/src/directives/ng_model.ts Outdated Show resolved Hide resolved

packages/forms/src/directives/ng_form.ts Outdated Show resolved Hide resolved

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jan 17, 2021

crisbeto force-pushed the 38465/forms-ng-model-ng-for branch from 84ef983 to 659375b Compare January 17, 2021 09:44

AndrewKushnir added the state: blocked label Jan 17, 2021

AndrewKushnir reviewed Jan 17, 2021

View reviewed changes

crisbeto force-pushed the 38465/forms-ng-model-ng-for branch from 659375b to 6285806 Compare January 23, 2021 09:54

crisbeto force-pushed the 38465/forms-ng-model-ng-for branch from 6285806 to 05b6316 Compare February 27, 2021 10:57

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label May 6, 2021

dylhunn modified the milestones: Backlog, v14-candidates Feb 4, 2022

crisbeto force-pushed the 38465/forms-ng-model-ng-for branch from 05b6316 to 59887c2 Compare February 4, 2022 14:45

dylhunn approved these changes Feb 7, 2022

View reviewed changes

crisbeto added action: merge The PR is ready for merge by the caretaker and removed state: blocked action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 7, 2022

dylhunn closed this in ebf2fc5 Feb 7, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 10, 2022

-             group = {};
-             items: {value: string}[] = [];
+             items: {value: string}[] = [];
+             group = {items: this.items};

Conversation

crisbeto commented Jan 16, 2021

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Jan 17, 2021

Uh oh!

AndrewKushnir commented Jan 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKushnir Jan 17, 2021

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Jan 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylhunn commented Feb 4, 2022

Uh oh!

crisbeto commented Feb 4, 2022

Uh oh!

crisbeto commented Feb 4, 2022

Uh oh!

AndrewKushnir commented Feb 4, 2022

Uh oh!

dylhunn commented Feb 4, 2022

Uh oh!

dylhunn commented Feb 5, 2022

Uh oh!

crisbeto commented Feb 6, 2022

Uh oh!

dylhunn commented Feb 7, 2022

Uh oh!

dylhunn left a comment

Choose a reason for hiding this comment

Uh oh!

dylhunn commented Feb 7, 2022

Uh oh!

angular-automatic-lock-bot bot commented Mar 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AndrewKushnir commented Jan 17, 2021 •

edited

Loading

crisbeto commented Jan 17, 2021 •

edited

Loading