Not working two-way data binding in corner cases

KBroichhausen commented 1 year ago

Once again a pull request by me. I have no idea why this patch is working and I really like to discuss that because I don't like to blindly provide patches where I don't know why they are working.

How to reproduce?

In this reprox https://stackblitz.com/edit/angular-ivy-cxvsbz?file=src/app/app.component.ts click Press to change and see that the child object in Non working is not changed to Desc. of child element 2 but in the second form in Working it is changed to Desc. of child element 2.

What's the difference?

The non working schema uses allOf in visibleIf while the second just uses visibleIf without allOf or oneOf

What's the patch doing?

It simply a one liner where I change from subscribe to pipe with disctinctUntilChanged within the binding function for allOf and oneOf. That's the same way as it is done when providing nothing for visibleIf (that's how I get to this solution).

Is this a regression?

No. Even I nearly complete have rewritten the oneOf and allOf binding myself in my last pull request, you can use version 2.8.1 and 2.8.2 in my reprox withouth any change. In 2.8.1 the subscription was done the same way as in the newer 2.8.2 version with my rewrite: https://github.com/guillotinaweb/ngx-schema-form/blob/410b38dda91428dc44ecd3b1c8d60ad1ce829f1e/projects/schema-form/src/lib/model/formproperty.ts#L372-L379

What's happening?

So, I have really no idea why it works with my patch. What I think I understand so far, is that somehow the different subscription overwrites the data in the model when changed. But why and how does this work? See the output where the non working schema output before a change, then after change and then after a timeout of 1 second where it is back in the state before. grafik

KBroichhausen commented 1 year ago

I just realisied that in my full application the two-way data binding is still not working for some values. So, this patch working in the reprox is probably just a side effect and not eliminating the problem.

ebrehault commented 1 year ago

@KBroichhausen in my opinion, the only effect of disctinctUntilChanged here is to ignore some values (the ones being identical to the last value), so it means that, somehow, we emit multiple times the same value and it results in a bad behavior.

Probably the non-working case is not about the value not being changed but changed and then changed bacl to the previous value. Don't you think? (nevertheless I have no idea either why or how 😓)

Regarding your last comment: do you still want to merge this fix (as it is not eliminating the problem entirely)? On my side, I am fine merging it considering it is a very small change that should not make any unwanted side-effect anyway.

KBroichhausen commented 1 year ago

Well, I think using distinctUntilChanged is a good idea at that place because I see calls all over the place without it. Also it is much faster in my full application.

Without distinctUntilChanged and in the minimal example, I can see that in onValueChanges of FormComponent the model is already set back to the initial non-modified state. And I just added some logs to the ControlWidget. Without distinctUntilChanged my change in the model is not populated there but with distinctUntilChanged it is. This happens before onValueChanges. Which imho means that it is somehow manipulated before. Due to the nature of asynchronous code it is really hard to debug and to tell what is going on.

Btw with the distinctUntilChanged fix, I got my application to work with a stupid workaround:

            for (let i = 0; i < 5; i++) {
                this.model = JSON.parse(fileReader.result.toString());
                this.cdr.markForCheck();
                this.cdr.detectChanges();
            }

Where cdr is ChangeDetectorRef. Maybe this is a different problem. It looks like all the visibleIf subscription are removing stuff from the model before it is fully evaluated, that's why I need to load it multiple times. Hence this does not really explain why some fields are not filled, which are always visible, but gets filled with the workaround.

So, technically this could be merged but it's not removing the root cause of this issue. 😉

ebrehault commented 1 year ago

Well, I guess the best approach would be to move all the components to changeDetection: ChangeDetectionStrategy.OnPush so we can have full control.

With the default strategy we are using at the moment, we just rely on Angular to do its best, but here is a typical case where it is not good enough.

Your fix with ChangeDetectorRef sounds totally fine to me (I have to do the same with many external libs quite often)

guillotinaweb / ngx-schema-form