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

Feat/emit initial value on init #296

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ophirKatz
Copy link

Allowing the form (root or sub) to emit the default values on init, if they are valid.
Solving issues #284 #232

@@ -180,7 +183,7 @@ export function createForm<ControlInterface, FormInterface extends {}>(
return options.outputFilterPredicate(transformedValue, formValue);
}

return !isEqual(transformedValue, formValue);
return options.emitInitialValueOnInit ?? !isEqual(transformedValue, formValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would have an influence on the rest of the emissions, not only the initial one. I wonder if we'd be better off having an option to exclude the isEqual check, or maybe even customise it entirely 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was thinking here is that it's good that it will affect all emissions because if (and only if) the initial value of the form (let's name it V) is valid, then I would want to emit it on init, and then if it would change to V overtime, it's still a valid value. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think then it's not the correct name for the option as it's misleading on what is going to happen while using it.

When I see an option that finishes by onInit I assume it's going to change a behavior only on init, not for the rest of the lifecycle.

This sounds more like a variable that should be named skipEqualCheck or something similar (don't really like the one I'm mentioning here but hopefully you get the idea).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I get that, but for me I think it's a side effect of what I'm trying to achieve - emitting the default values. I've been thinking that it should be named that, maybe - emitDefaultValues, but that still doesn't resonate with the side effect of skipping the equals check. But what do you think about it regardless of naming?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what do you think about it regardless of naming?

I'm not sure this is a good idea, the reason being that every time one of the sub form will "connect" to the root form, it'll try to emit.

So if you've got a root form with subForm1, subForm2, subForm3, and each of them have 3 sub forms as well, you'd get 1 (root form init) + 3 (the direct sub forms) + 3*3 (sub sub forms) = 13 events out of the root form. Which doesn't sound ideal to me tbh. And it's probably the main reason we left that as it is today, because we didn't see an easy fix. I'm not saying it's impossible, but I suspect the approach you've taken here may not be enough to be taken as is. When a form changes, especially a root form, it can trigger side effects (network calls, large computations etc) and I don't think that emitting multiple times is reasonable.

Now, I know it wouldn't be by default and this is an option BUT, the option idea is to emit the initial value. Except that when using it, so much more would be happening and changing the default behavior a lot more than just emitting the initial value.

The issue being, there's no easy way to know when all the form/sub forms have been properly initialized. Maybe we could go around that using DI and declare a token on a root form and then each sub form would register itself along the way? But that's a lot more work and questions

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