feat(forms): support type set in form validators by jeripeierSBB · Pull Request #45793 · angular/angular

jeripeierSBB · 2022-04-28T07:56:54Z

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
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, using Validators.required, Validators.minLength and Validators.maxLength validators don't work with sets because a set has a size property instead of a length property.

What is the new behavior?

Validators.required, Validators.minLength and Validators.maxLength validators are working with sets.

Does this PR introduce a breaking change?

Yes
No

Closes #46101

dylhunn · 2022-04-28T16:40:58Z

I assume this is for custom controls that give a Set when calling .value?

jeripeierSBB · 2022-04-28T17:34:48Z

I assume this is for custom controls that give a Set when calling .value?

yes exactely, eg. const control = new FormControl(new Set(), [Validators.required]).

We often use this in context of a chip input to easy avoid duplicates.

dylhunn · 2022-05-02T18:55:07Z

I don't think this is traditionally a use case we have thought much about, WDYT @AndrewKushnir?

dylhunn · 2022-05-02T22:47:58Z

Thank you for contributing @jeripeierSBB, but we are going to put this on the back burner for now. We currently do not support Set or Map anywhere, and if we decide to do so, we probably want to update every API at once.

Can you open a Feature Request instead (e.g. "Forms should accept Set collections in addition to arrays")? Thanks!

jeripeierSBB · 2022-05-03T05:56:18Z

@dylhunn Thank you for your statement, I will open a feature request. Just for my understanding, can you give me a hint, where at other places in forms as validators the type does matter? Or in other words, where would additional changes be required? Thanks!

angular-automatic-lock-bot · 2022-06-03T16:06:25Z

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

dylhunn · 2022-06-15T20:31:42Z

As discussed in #46101, there is significant community interest in this feature. I will review this PR this week.

2023 edit: oops. Time flies, I'll try to have a look

packages/forms/src/validators.ts

JeanMeche · 2025-01-15T21:30:59Z

We have some legitimate failures in the tests, could you please have a look ?

jeripeierSBB · 2025-01-15T22:06:45Z

We have some legitimate failures in the tests, could you please have a look ?

@JeanMeche Pushed the fix. Sorry, it was a mistake on my part when rebasing.

packages/forms/src/validators.ts

kirjs · 2025-01-16T17:41:27Z

packages/forms/src/validators.ts

No super strong feelings, but I kinda feel that unknown would be the right type here?

I could do so, but then I would need to cast to any or do additional checks when accessing length and size. Would you prefer a change, and if so by casting or introducing IsArray() or instanceOf Set checks?
Performance wise I think only casting would be better.

I don't think casting to any is necessary, woudln't something like this work?
Also see my other comment

if((Array.isArray(value) || typeof value === 'string') && typeof value.length === 'number'){ ```

Type unknown finally introduced with 1f6d76e

packages/forms/src/validators.ts

kirjs · 2025-01-21T18:50:02Z

Hey @jeripeierSBB,
I've had some discussions with teammates regarding this PR, and it's not super clear, how it aligns with the bigger picture of forms supporting sets in general, and whether it'd have repercussions for other parts of the forms.

I'm going to bring this up in the next team meeting, to thoroughly discuss this.

I know there's been a lot of back and forth on this PR, and appreciate your patience, hopefully I'll have a final answer for you within the next couple of weeks.

kirjs · 2025-01-28T19:23:58Z

Hey @jeripeierSBB, just chatted with the team, let's merge this in!
i'm going to follow up on the code review right now.

packages/forms/src/validators.ts

Previously, using `Validators.required`, `Validators.minLength` and `Validators.maxLength` validators don't work with sets because a set has the `size` property instead of the `length` property. This change enables the validators to be working with sets.

kirjs · 2025-02-04T00:07:26Z

Ok, this should be good to merge, thanks a lot for addressing all the feedback

alxhub · 2025-02-05T16:59:58Z

This PR was merged into the repository by commit fa0c3e3.

The changes were merged into the following branches: main

Previously, using `Validators.required`, `Validators.minLength` and `Validators.maxLength` validators don't work with sets because a set has the `size` property instead of the `length` property. This change enables the validators to be working with sets. PR Close angular#45793

angular-automatic-lock-bot · 2025-03-08T01:42:39Z

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

pullapprove bot requested a review from dylhunn April 28, 2022 07:57

jeripeierSBB force-pushed the support-set-in-validators branch 2 times, most recently from 771d20f to 123c25d Compare April 28, 2022 09:06

dylhunn added the area: forms label Apr 28, 2022

ngbot bot added this to the Backlog milestone Apr 28, 2022

dylhunn closed this May 2, 2022

jeripeierSBB mentioned this pull request May 23, 2022

Forms should accept Set collections in addition to arrays #46101

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 3, 2022

dylhunn reopened this Jun 15, 2022

angular unlocked this conversation Jun 15, 2022

dylhunn added feature Label used to distinguish feature request from other issues action: review The PR is still awaiting reviews from at least one requested reviewer state: confirmed forms: validators target: minor This PR is targeted for the next minor release labels Jun 15, 2022

JeanMeche reviewed Jan 30, 2023

View reviewed changes

packages/forms/src/validators.ts Outdated Show resolved Hide resolved

packages/forms/src/validators.ts Outdated Show resolved Hide resolved

jeripeierSBB force-pushed the support-set-in-validators branch from 123c25d to 7660bdb Compare February 1, 2023 08:30

angular-robot bot added the detected: feature PR contains a feature commit label Feb 1, 2023

jeripeierSBB requested review from JeanMeche and removed request for dylhunn February 1, 2023 08:35

pullapprove bot requested review from atscott and dylhunn and removed request for JeanMeche February 1, 2023 08:35

JeanMeche requested a review from kirjs January 15, 2025 21:02

kirjs approved these changes Jan 16, 2025

View reviewed changes

jeripeierSBB force-pushed the support-set-in-validators branch from 95a50a6 to b0cb669 Compare January 16, 2025 19:35

jeripeierSBB requested a review from kirjs January 17, 2025 21:02

kirjs requested changes Jan 28, 2025

View reviewed changes

packages/forms/src/validators.ts Outdated Show resolved Hide resolved

packages/forms/src/validators.ts Outdated Show resolved Hide resolved

jeripeierSBB added 7 commits January 28, 2025 23:43

fixup! feat(forms): support type set in form validators

a2fb485

fixup! feat(forms): support type set in form validators

b8347c6

fixup! feat(forms): support type set in form validators

64065a1

fixup! feat(forms): support type set in form validators

e82a0ef

fixup! feat(forms): support type set in form validators

65432b7

fixup! feat(forms): support type set in form validators

1f6d76e

jeripeierSBB force-pushed the support-set-in-validators branch from b0cb669 to 1f6d76e Compare January 28, 2025 22:46

jeripeierSBB requested a review from kirjs January 28, 2025 22:52

kirjs added the action: presubmit The PR is in need of a google3 presubmit label Feb 3, 2025

kirjs approved these changes Feb 3, 2025

View reviewed changes

kirjs added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Feb 3, 2025

alxhub closed this in fa0c3e3 Feb 5, 2025

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 8, 2025

Conversation

jeripeierSBB commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

dylhunn commented Apr 28, 2022

Uh oh!

jeripeierSBB commented Apr 28, 2022

Uh oh!

dylhunn commented May 2, 2022

Uh oh!

dylhunn commented May 2, 2022

Uh oh!

jeripeierSBB commented May 3, 2022

Uh oh!

angular-automatic-lock-bot bot commented Jun 3, 2022

Uh oh!

dylhunn commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeanMeche commented Jan 15, 2025

Uh oh!

jeripeierSBB commented Jan 15, 2025

Uh oh!

Uh oh!

kirjs Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

jeripeierSBB Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

kirjs Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeripeierSBB Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kirjs commented Jan 21, 2025

Uh oh!

kirjs commented Jan 28, 2025

Uh oh!

Uh oh!

Uh oh!

kirjs commented Feb 4, 2025

Uh oh!

alxhub commented Feb 5, 2025

Uh oh!

angular-automatic-lock-bot bot commented Mar 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jeripeierSBB commented Apr 28, 2022 •

edited

Loading

dylhunn commented Jun 15, 2022 •

edited

Loading

kirjs Jan 21, 2025 •

edited

Loading