Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
feat(forms): support type set in form validators #45793
Conversation
jeripeierSBB
force-pushed
the
support-set-in-validators
branch
2 times, most recently
from
771d20f to
123c25d
Compare
|
I assume this is for custom controls that give a |
yes exactely, eg. We often use this in context of a chip input to easy avoid duplicates. |
|
I don't think this is traditionally a use case we have thought much about, WDYT @AndrewKushnir? |
|
Thank you for contributing @jeripeierSBB, but we are going to put this on the back burner for now. We currently do not support Can you open a Feature Request instead (e.g. "Forms should accept Set collections in addition to arrays")? Thanks! |
|
@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! |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
|
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 |
dylhunn
added
feature
jeripeierSBB
force-pushed
the
support-set-in-validators
branch
from
123c25d to
7660bdb
Compare
pullapprove
bot
requested review from
atscott and
dylhunn
and removed request for
JeanMeche
|
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
Outdated
| * Return null else. | ||
| * @param value Either an array, set or undefined. | ||
| */ | ||
| function lengthOrSize(value: any): number | null { |
There was a problem hiding this comment.
No super strong feelings, but I kinda feel that unknown would be the right type here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'){
```
There was a problem hiding this comment.
Type unknown finally introduced with 1f6d76e
jeripeierSBB
force-pushed
the
support-set-in-validators
branch
from
95a50a6 to
b0cb669
Compare
|
Hey @jeripeierSBB, 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. |
|
Hey @jeripeierSBB, just chatted with the team, let's merge this in! |
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.
jeripeierSBB
force-pushed
the
support-set-in-validators
branch
from
b0cb669 to
1f6d76e
Compare
kirjs
added
action: merge
|
Ok, this should be good to merge, thanks a lot for addressing all the feedback |
|
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
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, using
Validators.required,Validators.minLengthandValidators.maxLengthvalidators don't work with sets because a set has asizeproperty instead of alengthproperty.What is the new behavior?
Validators.required,Validators.minLengthandValidators.maxLengthvalidators are working with sets.Does this PR introduce a breaking change?
Closes #46101