FormArray length validation in NgxSubFormComponent doesn´t propagate validity

[agarzas]

Hi!, firstly thank you for making angular forms better.

I think I found a bug, please look at this stackblitz: https://stackblitz.com/edit/ngx-subform-array-validation

specifically at the child-form.component, I´m using a validation function to validate FormArray length.

protected getFormControls(): Controls<ChildForm> {
        return {
            values: new FormArray([], this.minLengthArray(3)),
        };
    }

    minLengthArray(min: number) {
        return (c: AbstractControl): { [key: string]: any } => {
            if (c.value.length >= min)
                return null;

            return { 'minLengthArray': true };
        }
    }

When I delete an ítem from the array, the root form and subform should be in an invalid state, but the validation error is not propagating to the parent, only the subform is invalid

[maxime1992]

I just investigated this and found the root cause.

This is because for the errors we create an array representing the errors for each entry of the array. So it could be something like this for formGroupErrors (assuming that we've got an array containing 5 values and the only the middle one has an error):

{
  "myFormArray": [
    null,
    null,
    { "someError": true }
    null,
    null,
  ]
}

In the rewrite this has been changed (and is apparently the only breaking change compared to previous behavior). It'd look like the following when using the rewrite:

{
  "myFormArray": {
    2: { "someError": true }
  }
}

Therefore, it's much easier to fix in the new code as we've got an object to which we can simply add a property "formArray" for example which would contain all the errors issued from the validators of the formArray itself + the ones from the items in the array (by their index). Assuming this scenario, it could look like the following:

{
  "myFormArray": {
    "formArray": { "someErrorAtFormArrayLevel": true },
    2: { "someError": true }
  }
}

I'll work on this asap :+1:

[maxime1992]

@agarzas I've tried to make a fix and got it partially working: https://github.com/cloudnc/ngx-sub-form/pull/199

Unfortunately I found an issue and I'm not able to merge just yet. This is not on the top of the priority list so I'll put it on pause for now but I'll try to get back to it at some point :)

If you (or anyone else) have an idea as to why the error is not broadcasted up to the parents on time (only at the next change detection cycle), please feel free to comment on the PR itself or raise a fix :)

Thanks for reporting the issue and providing a repro :pray:! It was much appreciated :)

[anschm]

Today I run into the same issue. I am using the rewrite. I figured out to trigger the broadcast up to the parents the first time to call updateValueAndValidity on the formgroup inside the ngAfterViewInit method. Like:

ngAfterViewInit(): void { this.form.formGroup.updateValueAndValidity(); }

It works as an workaround, but is really ugly. It would be great to have this fixed soon.

[maxime1992]

It works as an workaround, but is really ugly. It would be great to have this fixed soon

I don't have time to look into this. Please raise a PR and I'll try to take a look

[anschm]

Today I did some investigation in this. Its seems that every time we delete or insert a new FormControl into the FormArray we must do formGroup.updateValueAndValidity() and the new valid / invalid status of the form is propagated to the parent correctly. But another problem is that a formGroup which includes a formArray does not propagate the valid / invalid status to the parent the first time or rather the form is initialized. After some investigation on file ngx-sub-form.ts I found the following code starting on line 241:

updateValue$: updateValueAndValidity$.pipe(
  tap(() => {
    formGroup.updateValueAndValidity({ emitEvent: false });
  }),
),

If I understand the code correctly, the formGroup should check the updateValueAndValidity on form initialization. But the emit is suppressed because emitEvent is false. I changed emitEvent to true and it works. Why is emitEvent false? In my opinion we should change this too true.

[maxime1992]

Thanks for the investigation.

It's unfortunately a piece of code where I wished we put a comment next to it because I really can't remember why there's emit event false. Your fix reminded me a of PR I saw recently: https://github.com/cloudnc/ngx-sub-form/pull/218/files

Have you tried running the e2e tests on the app after removing emit event false? Did they pass?

We've lost a whole bunch of unit tests that we used to have before the big bang rewrite... so that's not gonna help. If the e2e tests pass I may try running that change on our internal codebase using ngx sub form on which we also have some e2e tests hoping that if there's something breaking there it'd be caught

[anschm]

I dont know how to run the tests in your project. I dont use Karma and cypress. I tried, but I get to much errors on startup.

I did the change in my fork and run the e2e test from my own project. All tests passed. Can you please verify this in your application and do the change. It would be great to have this fixed soon.

[maxime1992]

You need to serve the app

yarn demo:start

and launch the e2e tests in parallel

yarn demo:test:e2e:watch

[anschm]

Now I did run the e2e tests without my change. It seems your tests are not stable because there are a lot AssertionErrors. Please check the e2e tests by your self. I don't know whats wrong with the tests. 6 passed and 8 tests with AssertionErrors and this without my chance.

[anschm]

Any update on this?

[maxime1992]

It seems your tests are not stable because there are a lot AssertionErrors. Please check the e2e tests by your self. I don't know whats wrong with the tests. 6 passed and 8 tests with AssertionErrors and this without my chance.

I just had a go from master branch, launched them 3 times, no issue at all

run 1	run 2	run 3

[anschm]

Ok, nice. And now whats happening if you run the tests with my change?

[maxime1992]

@anschm maybe try this branch, I just made a PR to upgrade cypress from v4 to v10, hopefully it may help with the tests being flaky on your side? https://github.com/cloudnc/ngx-sub-form/pull/258/

[maxime1992]

whats happening if you run the tests with my change?

Apparently nested errors are broken:

[anschm]

Ok, any ideas why this happens after the change? Did you any investigation on this?

[maxime1992]

I'm afraid not, I haven't spent any time on this. After doing that I made a few MR to update ngx sub form to latest angular, cypress and remove the deprecated API. Didn't get to that and don't think I'll have any free time to work on that any time soon so if you feel like digging into it yourself please go ahead and raise a PR. I'm happy discussing any findings you may have here if needed too

[anschm]

I dont know whats wrong with the e2e tests or maybe my system, but I dont get the e2e tests green. I get: Timed out retrying after 4000ms: expected [ Array(6) ] to deeply equal [ Array(6) ]

Any idea why this happens?

[maxime1992]

Interesting. If you open the devtool and click on the red line in cypress with the error, you'll get in the console of the browser the exact diff and why they're not the same. I do wonder. Are you on windows? Could it be a line return or anything like that breaking :thinking: ?

I guess if you post what's displayed in the console we can figure it out

[anschm]

I figured it out. Its a locale problem. The angular default locale is 'en-US' and all numbers you are displaying with the decimal pipe are rendered with a comma like $ 45,000,000. But inside the expected date set of listings you are expecting a number with dots like 45.000.000. You should set the LOCALE_ID to a fix locale you ecpect. For example set the locale fix to 'de' and then it works.

[anschm]

I check the e2e with my change and you are right the test should display the (nested) errors from the form fails. I think it fails because the validations errors from the nested array are now shown correctly. The crewMembers FormArray has a length validator. This validation error isn't shown without my change. Now the validation error comes up. In my opinion thats correct and the e2e test should corrected.

[anschm]

Whats your opinion on this? If you have the same opinion like me so I would provide a PR.

[maxime1992]

Can you push where you're at currently and raise a PR even as WIP? I'll try to take a look locally so I better grasp what's going on there :)

[anschm]

Could you give me permissions to push my branch and raise a PR? I finished and fix the bug.

[maxime1992]

Can you fork and raise a PR instead please? :) (and nice one for the fix, thanks for looking into it!)

[anschm]

PR s now available: https://github.com/cloudnc/ngx-sub-form/pull/263

[anschm]

Any progress on that?

[maxime1992]

I've got a bunch of work to focus on so not sure I'll be able to before end of week

I do keep it in mind and will try to have a look worst case scenario during the weekend