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

FormArray triggers onFormUpdate too early #127

Closed
4typen opened this issue Jan 6, 2020 · 7 comments · Fixed by #129
Closed

FormArray triggers onFormUpdate too early #127

4typen opened this issue Jan 6, 2020 · 7 comments · Fixed by #129
Labels
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 type: RFC/discussion/question This issue is about RFC, discussion or a question

Comments

@4typen
Copy link

4typen commented Jan 6, 2020

Refering to https://github.com/cloudnc/ngx-sub-form#dealing-with-arrays i propably found an issue with the initial building of the FormArray.

The method onFormUpdate(...) triggers when the form gets prefilled by the data of getInitialModel(). So the FormArray crewMembers seems to be updated at this time. Instead you have to use this.formGroup.valueChanges.subscribe(...) after super.ngOnInit() to correctly watch for changes.

@maxime1992
Copy link
Contributor

I've been discussing that with @zakhenry for a while, but I suspect it might be a good idea to deprecate the onFormUpdate method. And that makes another point for it 😅

@4typen are you using it a lot within your app?
Is anyone reading this issue using onFormUpdate heavily and would be against deprecating it?

@maxime1992 maxime1992 added scope: lib Anything related to the library itself state: needs design This feature or fix should be discussed before writing any code type: bug/fix This is a bug or at least needs a fix type: RFC/discussion/question This issue is about RFC, discussion or a question labels Jan 6, 2020
@maxime1992 maxime1992 changed the title FormArray triggers onFormUpdate to early FormArray triggers onFormUpdate too early Jan 6, 2020
@maxime1992 maxime1992 changed the title FormArray triggers onFormUpdate too early FormArray triggers onFormUpdate too early Jan 6, 2020
@4typen
Copy link
Author

4typen commented Jan 6, 2020

@maxime1992 We use it a few times, but we can change it easily i think. So deprecating onFormUpdate is no problem.

maxime1992 added a commit that referenced this issue Jan 8, 2020
If you patch your form and update a few values, this hook will be called multiple times: 1 time for each updated value. Which is not ideal.

Also has a bug here #127

And I've given more details here: #86 (comment)
maxime1992 added a commit that referenced this issue Jan 8, 2020
If you patch your form and update a few values, this hook will be called multiple times: 1 time for each updated value. Which is not ideal.

Also has a bug here #127

And I've given more details here: #86 (comment)
@maxime1992 maxime1992 linked a pull request Feb 11, 2020 that will close this issue
@maxime1992
Copy link
Contributor

https://github.com/cloudnc/ngx-sub-form/releases/tag/v4.0.1 just got release and embed the deprecation of the method :)

#129 has been merged into the next branch so it'll be fully removed on the next major upgrade 👍

@maxime1992 maxime1992 added state: has PR A PR is available for that issue and removed state: needs design This feature or fix should be discussed before writing any code labels Feb 11, 2020
@CyberBotX
Copy link

I hadn't noticed this being here until the 4.0.1 update happened. I've been using onFormUpdate in my own code. What I'm not seeing is what should be used in place of it. My cases for using it have been when I want to have other actions happen as a result of a form value changing.

@maxime1992
Copy link
Contributor

@CyberBotX instead of it you can simply subscribe to the formGroup.valueChanges or formControls.someValue.valueChanges.

BUT, I don't think that this hook was a good idea in the first place. If you end up doing things based on some form values, in a lot of cases it'll probably be something you should do in the parent of that component. Forms should only deal with the form value and not manage side effects IMO. Plus, as the hook is not dealing with observables you'd probably have to use subscribe into it which is a bad idea.

But I may be missing some use cases. Would you mind sharing the code of your components using onFormUpdate? Maybe I'll help you spot something that could be done in a better way or I'll realize I missed something and we shouldn't deprecate it :)

@CyberBotX
Copy link

A specific example I have is here: https://www.cyberbotx.com/angular/MutePrincess/src/app/Components/data-input.component.ts (if you want to browse up in the source tree, feel free to do so) with the fully running app at https://www.cyberbotx.com/MutePrincess.php

In that example, the data input component has a collection of radio inputs, usually when one is selected, nothing special happens in the data input component and the information is passed as-is to the root component. But if the "Other" option is picked, I show a color picker, which is handled differently from all the other colors. Using onFormUpdate was the only thing I could think of at the time. I don't know if subscribing to the valueChanges of the control would be better in this case or not. If you need other examples, I can provide some more.

@maxime1992
Copy link
Contributor

onFormUpdate has been deprecated as explained here and will be removed at some point.

I'd definitely encourage you to listen to valueChanges instead 👍

Closing as the method in question is deprecated

PS:

(if you want to browse up in the source tree, feel free to do so) with the fully running app at https://www.cyberbotx.com/MutePrincess.php

Funny app btw 🙃!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 type: RFC/discussion/question This issue is about RFC, discussion or a question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants