Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to have validatorFN for FormControl inside getFormControls() ? #82

Closed
andreElrico opened this issue Jul 24, 2019 · 9 comments
Closed
Assignees
Labels
flag: breaking change This feature or fix will require a breaking change released on @feat-rewrite released scope: lib Anything related to the library itself state: has PR A PR is available for that issue type: bug/fix This is a bug or at least needs a fix

Comments

@andreElrico
Copy link

andreElrico commented Jul 24, 2019

I know there is getFormGroupControlOptions but this is validation for the entire formGroup right? (as indicated by (formGroup) => {} ) - FormGroup - level.

How can I use validation on a FormControl level?

Here in my stackblitz im using the same validatorFN validatorWithDeps in a normal angular way and inside getFormControls() the ngx-sub-form way. How can I yield the same results? For ngx-sub-form its undefined. (because of scoping ...)

STACKBLITZ

Steps to reproduce:

  • open browser-console
  • type stuff into top field ("Enter you email")
  • type stuff into field with "12345" in it.
  • For top field the validator can resolve the values. For ngx-sub-form it cant.
@maxime1992 maxime1992 added the type: RFC/discussion/question This issue is about RFC, discussion or a question label Sep 1, 2019
@maxime1992
Copy link
Contributor

maxime1992 commented Nov 11, 2019

Hi @andreElrico,

I took some time this evening to investigate. Thanks for the stackblitz repro it helped a lot.

If you look into this comment: microsoft/TypeScript#1617 (comment)

You'll notice that:

The order of initialization is:

The base class initialized properties are initialized
The base class constructor runs
The derived class initialized properties are initialized
The derived class constructor runs

As of today, the creation of the formGroup is happening in the constructor and therefore, the getFormControls method from the sub class is called when the properties of the sub class are not initialized yet.

public formGroup: TypedFormGroup<FormInterface> = (new FormGroup(
this._getFormControls(),
this.getFormGroupControlOptions() as AbstractControlOptions,
) as unknown) as TypedFormGroup<FormInterface>;

I'm working on a fix but I suspect it might be a (small) breaking change.

EDIT:

I've got the fix working just need to edit some tests

maxime1992 added a commit that referenced this issue Nov 12, 2019
BREAKING CHANGE: If you have components that are extending one of the classes of ngx-sub-form AND that have an `ngOnInit` hook, they should call `super.ngOnInit()`

This closes #82
@maxime1992 maxime1992 added flag: breaking change This feature or fix will require a breaking change scope: lib Anything related to the library itself state: has PR A PR is available for that issue type: bug/fix This is a bug or at least needs a fix and removed type: RFC/discussion/question This issue is about RFC, discussion or a question labels Nov 12, 2019
@maxime1992 maxime1992 self-assigned this Nov 12, 2019
@maxime1992
Copy link
Contributor

Small update that I forgot to post here: this is on hold as I've found a few annoying things in my fix 😓

CF #113 (comment)

@alex87
Copy link

alex87 commented Feb 3, 2020

Hi @maxime1992 we are also facing a similar issue where we need to use validators that are not available until ngOnInit() is called.

It is quite a pressing issue for us and I was wondering if you have an estimation on when you might be able to resolve this?

If you don't have time we would be happy to help if you can explain the issues!

@maxime1992
Copy link
Contributor

maxime1992 commented Feb 3, 2020

Hi @alex87. As you can see here: #113, I've been working on a fix but unfortunately I was quite unhappy with the outcome as it'd mean:

  • We'd then have to think of calling super.ngOnInit() if overriding that hook in our components (and there's no way to make sure it's type safe for ex)

  • A lot of properties like formGroup, formGroupErrors, formGroupControls would not be available as soon as the class is created. Only after ngOnInit which IMO is very error prone especially for people who don't have strict null checks turned on...

So I'm not willing to ship this MR anymore and would like to come up with a better fix. If you refer to my previous comment, here's of the order of execution works: microsoft/TypeScript#1617 (comment)

It is quite a pressing issue for us and I was wondering if you have an estimation on when you might be able to resolve this?

No estimation as I'm not sure how to fix this properly. Do you have any idea? I'd gladly accept some help.

@maxime1992 maxime1992 reopened this Feb 3, 2020
@maxime1992
Copy link
Contributor

☝️ Wooops, fail clicked. Reopened.

@alex87
Copy link

alex87 commented Mar 10, 2020

Hi @maxime1992 sorry for the delay in getting back to you, got busy!

We found a workaround for this specific issue by applying validators in ngOnInit using the setValidators function on the formControls. Its not perfect but works around the issue for now.

Regarding your fix and calling super.ngOnInit() this already has to be done for ngx-sub-form no? https://github.com/cloudnc/ngx-sub-form#angular-hooks

We have a similar issue with other internal components and I've been thinking it might be possible to enforce in typescript by adding some kind of decorator/tslint rule to the base function. Unless this gets solved soon: microsoft/TypeScript#21388

@maxime1992
Copy link
Contributor

Discussed with @zakhenry:

We will move the creation of the formGroup to ngOnInit

maxime1992 added a commit that referenced this issue Jun 15, 2020
This is a major architecture change which is brought without any breaking change 😄!

We've split up the code base in 2: Old one and new one.
The old one hasn't moved at all but is now deprecated (not removed yet!).
You can keep using the old one for a bit and have a smooth/incremental update to use the new API.

Few changes that you have to note with the new API:
- Only works with Angular 9 or more
- The app needs to have Ivy activated (this is because we use `ɵmarkDirty` internally. If it ever gets removed we'll probably have to ask to provide the `ChangeDetectorRef` but we were able to around this for now!)
- We got rid of inheritance 🙌
- Form errors on a FormArray are now an object instead of an array. Previously the array contained null values on all the fields without any error. It's now an object containing only the ones with errors and you can access them using the index

Please start upgrading to the new API as soon as possible as we stop supporting the old API as of today and will remove it in a near release.

This closes #171 for the major architectural changes and also the following issues as a result:
- closes #82
- closes #86
- closes #93
- closes #133
- closes #143
- closes #144
- closes #149
- closes #160
- closes #168
@maxime1992
Copy link
Contributor

🎉 This issue has been resolved in version 5.2.0-feat-rewrite.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

zakhenry pushed a commit that referenced this issue Oct 23, 2020
This is a major architecture change which is brought without any breaking change 😄!

We've split up the code base in 2: Old one and new one.
The old one hasn't moved at all but is now deprecated (not removed yet!).
You can keep using the old one for a bit and have a smooth/incremental update to use the new API.

Few changes that you have to note with the new API:
- Only works with Angular 9 or more
- The app needs to have Ivy activated (this is because we use `ɵmarkDirty` internally. If it ever gets removed we'll probably have to ask to provide the `ChangeDetectorRef` but we were able to around this for now!)
- We got rid of inheritance 🙌
- Form errors on a FormArray are now an object instead of an array. Previously the array contained null values on all the fields without any error. It's now an object containing only the ones with errors and you can access them using the index

Please start upgrading to the new API as soon as possible as we stop supporting the old API as of today and will remove it in a near release.

This closes #171 for the major architectural changes and also the following issues as a result:
- closes #82
- closes #86
- closes #93
- closes #133
- closes #143
- closes #144
- closes #149
- closes #160
- closes #168
@github-actions
Copy link

🎉 This issue has been resolved in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: breaking change This feature or fix will require a breaking change released on @feat-rewrite released scope: lib Anything related to the library itself state: has PR A PR is available for that issue type: bug/fix This is a bug or at least needs a fix
Projects
None yet
Development

No branches or pull requests

3 participants