KillerCodeMonkey / ngx-quill

Angular (>=2) components for the Quill Rich Text Editor
MIT License
1.78k stars 260 forks source link

Reactive Form Locks Up #1045

Closed KerickHowlett closed 4 years ago

KerickHowlett commented 4 years ago

I'm trying to create a complex form that uses child components for part of the forms; however, when I try to use patchValue or setValue on a ReactiveForm property that's NOT the editor, my browser just locks up. There's something creating an infinite loop or something like it.

Parent Component

public ngOnInit(): void {

        this._form = this.formBuilder.group( { updateOn: 'blur' } );

        this.subscription = this.form.valueChanges.subscribe( ( newData: any ) => console.dir( newData ) );

    }

Editor Component (Child One):

public ngOnInit(): void {
        const excerptValidators: Array<Validators> = [ Validators.required, Validators.minLength( 1 ), Validators.maxLength( this.maxLength ) ];
        this.parent.form.addControl( 'writing', this.formBuilder.group( { excerpt: [ '', { validators: excerptValidators, updateOn: 'blur' } ] } ) );
    }

Sibling Component (Child Two):

constructor(
        private formBuilder: FormBuilder,
        @Host() private parent: FormGroupDirective
    ) { }

    public ngOnInit(): void {
        this.parent.form.addControl( 'source', this.formBuilder.group( { type: [ '', { validators: [ Validators.required ], updateOn: 'blur' } ] } ) );
    }

    public ngAfterContentInit(): void {
        this.parent.form.get( 'source' ).patchValue( { type: Source.Book } );
    }
KerickHowlett commented 4 years ago

NOTE: Both the editor and sibling component use the same constructor arguments.

KerickHowlett commented 4 years ago

Second Note: I tried to use the following block of code to provide a default value (the goal I'm trying to achieve when I ran into this problem), but the outcome was the same as I mentioned.

this.parent.form.addControl( 'source', this.formBuilder.group( { type: [ 'book', { validators: [ Validators.required ], updateOn: 'blur' } ] } ) );
KillerCodeMonkey commented 4 years ago

could you provide a stackblitz or something like that.

so in general reactive forms are working fine.

there is even an example in my demo repo:

https://killercodemonkey.github.io/ngx-quill-example/ https://github.com/KillerCodeMonkey/ngx-quill-example

search for "Reactive Forms and patch value"

KerickHowlett commented 4 years ago

There's too much of my code to try to put in StackBlitz.

Regardless, this problem goes away as soon as I comment out the editor, so the problem has to be stemming from there.

Do your examples involve passing the form data between parent and child components?

KillerCodeMonkey commented 4 years ago

nope... but it can not be that difficult at all.

but without a real example i can not help you... it would be just guessing. If you browser goes in an endless loop you should check your js dev console of your browser.

Maybe there is just an undefined variable or what ever in your template.

KerickHowlett commented 4 years ago

There are the code samples above, but here's my GitHub if that helps.

I'm new to FormBuilder, as I'm used to using ngModels; however, I'm trying to learn a new means of going about it. Plus, I think it'll work better for what I am doing.

KillerCodeMonkey commented 4 years ago

there is no onTextChange output for quill-editor. it is onContentChanged: https://github.com/KillerCodeMonkey/ngx-quill#outputs

KerickHowlett commented 4 years ago

Oh, that's right! I forgot. I had switched over from using ngPrime to using yours. (I thought I got rid of that.) I'll give that a try and see if that works.

KerickHowlett commented 4 years ago

I just applied the change and checked... that wasn't it. It's still locking up my browser. Every time I boot it up, the fan on my computer kicks into gear and my tab locks up. It won't even let me open the console, or if it's already open, the console won't populate with anything at all — not even the element inspector.

KillerCodeMonkey commented 4 years ago

in your case i would just create a new clean project with ng-cli. add there only a form + a form control in a sub component.

to reproduce it.

just as less logic as possible.

KerickHowlett commented 4 years ago

With all do respect, but that's not gonna fully capture the whole situation. Besides, the problem occurs when I try to set a default value in the sibling component.

KillerCodeMonkey commented 4 years ago

but then you can not expect from my side 😉. Because i will not build something on my own to reproduce it. And i think it is really easy to rebuild it in a new clean project.

KerickHowlett commented 4 years ago

That makes no sense. When a scientist wants to re-examine something, they need to do so with all the elements present at the time of the occurrence.

But I can do that. Sorry, I don't mean to sound difficult. I don't talk to other develops often through GitHub, so I'm not fully vetted in the etiquette.

Please reopen the ticket, and I'll try to provide it when I can.

KillerCodeMonkey commented 4 years ago

even scientist try to rebuild systems in an encapsulated way to avoid falsy result because of outer influences 😉

in general you can not expect help for a really really specific problem you have by just describing what is happening. like you see.. you even did not used the correct hooks of the component and so on.

And you can not expect that a maintainer of an Open source project will be sit down for hours guessing and trying to build something to reproduce the problem or making a code review of a whole part of your project... in most cases those people have a fultime job or other things to do.

The idea of building a minimal reproducable example is to show that the bug has nothing to do with some other parts and is part of this lib/project. So the initial work lays by the issue creator and not the maintainer.

i mean you want to get help for a really specific problem you have.

And by building a stackblitz or minimal repo you will need to rethink and check your code again.

KerickHowlett commented 4 years ago

Admittedly, those are fair points.

I'll try to get it built tomorrow-- er, later today. It's almost 2AM here.

KillerCodeMonkey commented 4 years ago

oookay...

in general if i can give you a hint.

remove everything that is not needed in first place -> just a formgroup with form controls + quill editor. so even the custom toolbar of the editor or validators are not really interesting, so something really simple should be the result. 1 module, 1 parent component, 2 child components.

KerickHowlett commented 4 years ago

Sorry that it took me so long to get this done, but here's the StackBlitz link.

IMPORTANT NOTE: I had to comment the entire app.component.html file because it was still running into that bug, and uncommenting it will lock up your tab and will jumpstart your computer's fan.

KillerCodeMonkey commented 4 years ago

i played around and the problem only occurs when you use your multi select component... everything is working only with the editor... even if i comment out the editor and only the multi select is in there... it fails.

So i think the problem is your multiselect not the editor.

KerickHowlett commented 4 years ago

It also works when you comment out the editor.

KillerCodeMonkey commented 4 years ago

nope editor is working for me :)

only the multi selects breaks it

KerickHowlett commented 4 years ago

For some reason the problem goes away when I comment out the editor in my actual app. I don't know why it's not working here.

KillerCodeMonkey commented 4 years ago

the problem is because how you try to nest a formgroup for your multi select in your parent form.

KillerCodeMonkey commented 4 years ago

i just changed it a little bit: https://stackblitz.com/edit/reactor-form-test-jmnzbu?file=src%2Fapp%2Fmulti-select%2Fmulti-select.component.html

KerickHowlett commented 4 years ago

Yes, but now the form isn't passing the data to the parent. It's not triggering the observable when you click on the buttons, anyway.

Also, here's a screenshot of it working with me just commenting out the editor component.

Screenshot

KillerCodeMonkey commented 4 years ago

yeah but then i guess it is because of your generall structure you build up. Manipulating the parent component and adding controls/groups there.

KillerCodeMonkey commented 4 years ago

and i played around a little bit more and knocked it down to your .patchValue in the multi select component

when i do not patch the value it renders without problems. https://stackblitz.com/edit/reactor-form-test-ev4w5p?file=src%2Fapp%2Fapp.component.html

but in general i do not know how your update configuration with "onBlur" plays in there and so on.

KerickHowlett commented 4 years ago

Trying to add in a default value is why I came here hoping you could solve the bug in the first place. The reason I tried to use patchValue was because simply adding it into the multi-select's form controller didn't work, and I thought doing it during view init would work.

My original thought was the way I had broken down my form's child components, too, but I couldn't find anything that stuck out to me as the immediate problem.

I also tried deleting the onBlur update properties, and the problem remains.

KerickHowlett commented 4 years ago

Okay, I did some digging into ngx-quill, and while I still can't figure out the exact cause, but I was able to narrow it down the code that's causing the memory leak. I don't know why it's causing it, but I narrowed down the two lines in the code that's triggering it.

In your quill.service.file, when I comment out these two lines of code:

const quillImport = await import( 'quill' );
this.quill = ( quillImport.default ? quillImport.default : quillImport ) as any;

Obviously, the code no longer works when I comment these out, but it seems to be where the memory leak's coming from.

It's possible that it could be a coincidence, but I've gotten as far as I could without your insight.

KerickHowlett commented 4 years ago

Okay. I don't know why this works, but I finally fixed it — or at least I found a workaround.

Whatever the actual problem may be, I feel the genuine problem is QuillJS itself, because I switched over to PrimeNG's editor and it still didn't work.

PrimeNG's editor is near identical to ngx-quill in almost every way — right down to using QuillJS. The only actual difference I could see was that it didn't have onFocus() or onBlur() outputs, and it called the onContentChange() output onTextChange(). Furthermore, the memory leaked stop whenever I commented out the QuillJS import when attempting to debug the ngx-quill library.

In case anyone else comes across this same problem, I want to make sure they can come here to find the answer: The workaround is to set the ChangeDetectionStrategy for your form components to OnPush. That way it'll only update the components only once unless told to so otherwise; thus avoiding the infinite memory leaking loop of doom.

The component annotation will need to look something like this:

@Component( {
    selector: 'form-group-component',
    templateUrl: './form-group.component.html',
    styleUrls: [ './form-group.component.scss' ],
    changeDetection: ChangeDetectionStrategy.OnPush, // <---- THIS LINE OF CODE HERE!!!
    viewProviders: [ { provide: ControlContainer, useExisting: FormGroupDirective } ],
} )
export class FormGroupComponent {}
KillerCodeMonkey commented 4 years ago

then i would guess, creating the formcontrol and setting the default value is triggering multiple times so the quill editor is rerendered multiple times before it is initialized correctly the first time.

KerickHowlett commented 4 years ago

I thought, too, but you had a counter put in place where it would only run that block of code if the Quill Editor didn't already exist and if the counter equalled one.

KillerCodeMonkey commented 4 years ago

but what happens, when the rerendering happens that fast, that the service in not initialized. SO maybe it would try to load it again and again ...

KerickHowlett commented 4 years ago

That makes sense. I saw you had it on a Promise, which would make it asynchronous and could lead to such a thing occurring.

My problem now is that it won't trigger the visual change in my Angular Material child components, leaving my default value highlighted as selected even when I've selected another option. Although, that's a different discussion that's out of scope for this thread.

In either case, I'm just glad I could find a workaround that'll no longer lead to my computer sounding like it's about to self-destruct.

KillerCodeMonkey commented 4 years ago

:D.

In general why do you initialize your components that why? Because in general you should not directly access parent properties.

if you know you will have an editor and your selection in the form. just add everything in the parent class to the formgroup + default values.

an then pass the control as an input property to the child components.

maybe this would reduce the complexity a little bit.

KerickHowlett commented 4 years ago

I'm assuming you mean the @Host() annotation in the constructor?

The idea is to make each component as "dumb" as possible and be unaware of one another. Someone taught me you should design your architecture by creating components around individual features, so that all you have to do to remove a feature is delete the component, and that's it. You're done!

I will admit that using FormGroup and FormControl is 100% new to me, and trying to break them down into nested child components is proving to be a difficult challenge, too. I'm using this opportunity to learn all this. Sadly, I can't find too many good examples of this on Google.

Let me know if you ever find any. :)

KillerCodeMonkey commented 4 years ago

@KerickHowlett then you could create your custom form controls as the quill-editor component is one :), where you can pass a form control.

or you could just work with inputs and outputs instead of the nested form groups. Just pass the the values/default value in and inside of your child components provide an output to inform the parent component.

it would be much easier to understand and to read. and you eliminate changing the parent the children should not know anything about if they are really dumb.

In your case the children need a formgroup and you expect it is there.

KerickHowlett commented 4 years ago

I think I understand what you're saying. (It's super late here, because I'm up way too late programming again, so I'm having to read what you said ten times to just comprehend half of it. XD )

Although, I was thinking I should restructure my form, anyway. For some different reasons than the ones you listed, but if I understand you correctly, you also bring up some valid points.

I had tried to use Inputs and Outputs in the beginning, but that didn't work out. If I recall correctly, I kept running into problems like infinite loops and the form not updating onBlur. I'll figure it out, though.

Before that person enlightened me, I would over complicate everything — making everything into an intermingled mess of spaghetti code and tightly meshed smart components. I'm trying to learn from my mistakes and not go down that road again. :p

KerickHowlett commented 4 years ago

Also, when it comes to the ngx-quill component, there are also some other things I was including to affect things around the editor itself. For example, a means to limit the max number of characters.

Although, now I say that, while it wasn't a PrimeNG feature, I hadn't even checked to see if the means were already built in to max out the number of characters a user can type into the field.

I don't mean a validation, but rather stop any additional input into the text field when it's hit the max number of characters (not counting the injected tags and such). That's why I'm using the onContentChange() output; however, I still have to do some work in not counting the said tags.