feat(forms): add error when adding control with dupe name to form

[GrizzlyEnglish]

PR Checklist

Please check if your PR fulfills the following requirements:

• [x] The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
• [x] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)
  • Not sure on if this feature would need documentation other than api reference, if so please point me in the right direction

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?

Currently if a user adds controls to a form that share the same name, their could be unintended consequence due the models setting the controls by name in the form group and having no way to differentiate.

Issue Number: #13993

What is the new behavior?

When a control with a duplicate name is added to a form a runtime error is thrown to notify the user of the possibility of unintended consequences.

Does this PR introduce a breaking change?

• [ ] Yes
• [x] No

I don't believe this would be a breaking change - since surely no one has existing apps with duplicate names.

Other information

I tried avoiding adding any parameters or anything to existing methods in order to make as small of an impact as possible; but, due to the promises and the use case of ngFor renaming controls as they are being deleted I had to add the changes to the control functions in order to determine if it was a rename or if the control was being added.

[GrizzlyEnglish]

My bad on the failed tests, not really sure what is happening 'cause if I run them, they pass.

And I can forcefully make these tests fail by changing the expected header of the forms app

I am absolutely baffled currently, so my bad for having a PR with failed test 'cause like I said they work on my box :D. Probably due to environment more than anything.

[dylhunn]

I have reopened as requested in #13993. Would you like me to review this as-is? @GrizzlyEnglish

[GrizzlyEnglish]

@dylhunn Yes please! Thanks!

[dylhunn]

Great. My main concern on first glance is making sure this a non-breaking change. I'll get back to you soon with more specific comments.

[GrizzlyEnglish]

@dylhunn @AndrewKushnir Apologies for the spam, and the delay. Got this updated based on feedback. Moved from using a promise to ngZone and an exception to console.warn. In doing so I had to update the tests as well. The only downside of this update was it was now triggering dupe names on everything; since, the ngZone triggers after and the directive exists in the collection. Just added a simple check to make sure it wasn't the same control.

Let me know if you see anything else. I appreciate the help!

[dylhunn]

@GrizzlyEnglish Andrew has gone on vacation for the next few weeks. Let me re-review this and find a replacement second approver.

[GrizzlyEnglish]

@dylhunn I am in no rush whatso ever if you choose to wait, I will work whatever bits needed later on. I just would like to try another ticket after this one is completed haha. Thanks!

[dylhunn]

The above issue is really surprising, because the _directives set is immediately initialized with new Set().

Maybe the initializer isn't running for some reason. All the stack traces look like this:

TypeError: Cannot read properties of undefined (reading 'forEach')

at Object.next (forms/src/directives/ng_model.ts?l=258
at SafeSubscriber.__tryOrUnsub (rxjs/src/internal/Subscriber.ts?l=268
at SafeSubscriber.next (rxjs/src/internal/Subscriber.ts?l=210
at Subscriber._next (rxjs/src/internal/Subscriber.ts?l=153
at Subscriber.next (rxjs/src/internal/Subscriber.ts?l=113
at Subject.next (rxjs/src/internal/Subject.ts?l=75
at EventEmitter_.emit (core/src/event_emitter.ts?l=114
at core/src/zone/ng_zone.ts?l=341
at _ZoneDelegate.invoke (zone.js/lib/zone.ts?l=1145
at FakeAsyncTestZoneSpec.onInvoke (zone.js/lib/zone-spec/fake-async-test.ts?l=674
at ProxyZoneSpec.onInvoke (zone.js/lib/zone-spec/proxy.ts?l=134
at _ZoneDelegate.invoke (zone.js/lib/zone.ts?l=1142
at Zone.run (zone.js/lib/zone.ts?l=825
at NgZone.runOutsideAngular (core/src/zone/ng_zone.ts?l=244
at checkStable (core/src/zone/ng_zone.ts?l=341

The common stack frames to all presubmit failures start at FakeAsyncTestZoneSpec.onInvoke, and nearer.

[dylhunn]

A status update on this:

• We need Andrew K's approval once he's back
• We should probably figure out why _directives is sometimes uninitialized
• A small number of internal presubmit failures, hopefully nothing too serious and we can sort them out.

Thanks again for the PR! :)

[GrizzlyEnglish]

@dylhunn @AndrewKushnir Apologies for taking so long on this. Life happened. Question for you two: I rebased like mentioned by circle ci - and now it thinks my latest commit is not a tree. Thoughts on this?

[GrizzlyEnglish]

Ok, rebased again and now that is all fine - but it seems I have gone over the limit

[GrizzlyEnglish]

@dylhunn Sorry to be a pain on this. I am not sure how to fix this. My most recent commit a14f30838a2606d73098635ed3f30828fd25a640 is saying that it is too large; but it is the required symbols.

[jessicajaniuk]

@GrizzlyEnglish You should be able to look at the test failure and update the golden file with the new size. The file is in the root of the repo at goldens/size-tracking/integration-payloads.json. Update the block for forms > uncompressed with the new values. Once you do that, we'll get pinged for size tracking reviews.

[GrizzlyEnglish]

@jessicajaniuk Thank you!

[jessicajaniuk]

@GrizzlyEnglish looks like your fixup commit updated the size to the expected value and not the actual value. The failing size test shows the main uncompressed for forms actual value as 158276. Setting it to that should get it passing.

[GrizzlyEnglish]

@jessicajaniuk Ah my bad. Working and working on this not a good combo :D

[dylhunn]

I or Andrew will look at this shortly, but for now let's target v15.1.

[JeanMeche]

Do you mind rebasing your PR and fix the conflicts ?