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

Emit value when writeValue(null|undefined) + defaultValues defined #152

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ntziolis
Copy link
Contributor

@ntziolis ntziolis commented Mar 1, 2020

This PR addresses a bug where data inside a sub-form does not matching the data in the control in the parent form until additional changes are being made to the sub-form (for example via user input).

As outlined in #86 there are situations where we should be emitting from inside writeValue:

  • When a reset() is triggered on the outer form and there are default values specified in the sub form
  • The value inside the sub-form is no longer the same as the value that was passed into writeValue
  • Hence in this case the sub form should emit to the outer form
  • The same is true when the sub-form is initialized with null or undefined and has default values

@ntziolis ntziolis changed the title [Please Ignore] fix(writeValue): emit value changes if input and internal value would be different Emit value when writeValue(null|undefined) + defaultValues defined Mar 13, 2020
@ntziolis
Copy link
Contributor Author

@maxime1992 This is ready for review by now.

@maxime1992
Copy link
Contributor

This LGTM but I'm also afraid I might be missing something.

I'm pretty sure that we did set to off for a good reason and either it may be dangerous to turn it back on for some cases or we may need to do that for other cases.

@zakhenry any idea on your side?

@ntziolis
Copy link
Contributor Author

@maxime1992 Disabling it was/is the correct thing todo. As the intend is that as the same value that is passed in shouldn't cause an event to be bubbled back up to the parent form.

However due to

  • (passed-in + default value) != passed-in

this is a case where we should emit as otherwise the internal state of the sub form is different than the value (for the sub form) in the parent form.

I was careful to test if there are any cycles created but could not find issues with it so far. We have this code (money patched) in production use case since about 1 month and are very happy with it.

@maxime1992
Copy link
Contributor

@ntziolis I believe this will be solved by the rewrite here: #176

When you have some time please have a look and close this one if it's ok as the other one is kind of massive 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants