-
Notifications
You must be signed in to change notification settings - Fork 87
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(form events): Create unified observable and signal data accessors for form events #391
feat(form events): Create unified observable and signal data accessors for form events #391
Conversation
TL;DR - I considered broadening the signature of the functions to be not just a form but also a signal or observable of the form, but it is a real pain with some gotchas. I think it is worth noting but are more of a footnote to document how to work around those and not something to change the proposed implementation One pain discussed in #365 is when forms are passed as inputs to different components. To my knowledge, doing so while retaining types and also being able to key off of https://stackblitz.com/edit/ptxmym-pggt7i?file=src%2Fapp%2Fchild%2Fchild.component.ts edit: after messing around in a fork of this Stackblitz, I realized that conveying this context and account for it in a shared utility is kind of a mess. I will probably just roll my own thing or suggest something for that scenario. See the pitfalls of trying to get too slick with this https://stackblitz.com/edit/ptxmym-jwxuyo?file=src%2Fapp%2Fchild%2Fchild.component.ts. The smartest thing to do is probably narrow the accepted type back to just taking in a form and not an observable or signal of a form, ore maybe like the above stackblitz, and then just note the mapping that may be needed ad hoc to transform anything to the |
I am fairly satisfied with this as is after tweaking a few things. If nobody has any sort of questions or suggestions then I will mark this as an official PR in the next day or two. Though of course those are welcomed then too. |
One thing I have considered but I am leaving out unless people want: equivalent event property names. For example, Is that a thing people would want? My inclination is to keep things simple but be open to more. And for the moment being, the exposed state of the signal and observable have the essentials and other data could be derived from there. As I think out loud, I could see people wanting |
Link to example of what I mean. Names of functions WIP: https://stackblitz.com/edit/4gahmc?file=src%2Fexample%2Fform-event-utils.ts Output Form Unified Properties + Form Derived Properties all in one {
"value": {
"firstName": "",
"lastName": ""
},
"status": "INVALID",
"touched": false,
"pristine": true,
"valid": false,
"invalid": true,
"pending": false,
"dirty": false,
"untouched": true
} another state with a value {
"value": {
"firstName": "Test",
"lastName": ""
},
"status": "VALID",
"touched": true,
"pristine": false,
"valid": true,
"invalid": false,
"pending": false,
"dirty": true,
"untouched": false
} |
After making the example and messing around with it I decided to add these |
If this is to be approved, should I make the documentation page now or after? I don't want to presume but also I am willing to write it up now if the feature is to be considered. |
edit: https://v17.angular.io/guide/typed-forms#partial-values Note: I realized that despite the return signature, quirks of the forms API and values, it tends to when used return a partial. Despite I am going to try some tweaks on a few things to see if I can either avoid it becoming partials, and at worst just explicitly mark that it is effectively giving partials for edit: as an aside but also a new update, I am also going to tweak it to use |
@michael-small I'll respond to your question in #365 here to keep it all in once place. As I understand you are looking to provide a way to make the form values not a partial type. Of course I'm lacking some context here, and not sure if this is the best approach, but I was able to get this to work. First you could add an overload: export function allEventsObservable<T>(
form: AbstractControl<T>,
): Observable<FormEventData<T>>;
export function allEventsObservable<T>(
form: AbstractControl,
): Observable<FormEventData<T>>;
export function allEventsObservable<T>(
form: AbstractControl,
): Observable<FormEventData<T>> { and then this would allow manually passing in the type if you wanted to force the value type to not be a partial, e.g. this would give you a partial type: form$ = allEventsObservable(
this.form,
); and this would allow you to supply the non-partial type: form$ = allEventsObservable<ReturnType<typeof this.form.getRawValue>>(
this.form,
); Not sure if that's exactly what you want or if it's the best approach, but maybe you could play around with that |
This is good stuff, I'll experiment with this. Overloads and util types are something I don't think about that often but this opens a lot up. Thank you for taking the time to give feedback/advice. |
…l-small/ngxtension-platform into feature/form-events-utils
Pinging @nartc because I have made significant changes since this was originally marked as ready. Apologies for making so many changes and edits ever since marking this PR as ready... and then spreading that out via a bunch of comments and edits. I hope I didn't make too much noise, but I don't want to drown this out: I am now confident that it is ready. Two biggest changes
If there is anything that is needed (docs, changes, clarification) then I am happy to help. |
Woo, thank you for including this feature! I'll write up a doc page and put in a PR for that soon. |
@michael-small status = formStatus(this.form); // status is a signal
status$ = fromFormEvents(this.form); // status$ is an observable that gets created from form events
|
I'll follow up with more later tonight when I have more time |
This is mostly what I already said, but slightly more thought out. And I had an idea for a unified kind of name for both the signal and observable version. Here is a link to it renamed and used in a Stackblitz to get a feel for it in action: https://stackblitz.com/edit/stackblitz-starters-qoq8b9?file=src%2Fmain.ts Form Util Naming - Your Idea for Observable naming, my idea for the Signal naming?
The name of "status" crossed my mind before, but part of the issue is that there is a
Taken together... what if the signal variant was fromFormState(this.form) I know that "from" is more RXJS-y, but it is also making state "from" a form. I feel like this naming would make them more apparently tied together, and Event is to RXJS as State is to Signals. PS - while we will be tweaking the utility's code...Whenever we agree on naming, can I also make sure to export the type |
Context
Broached in Discussions #334 and Issue #376
Stackblitz: https://stackblitz.com/edit/stackblitz-starters-nbicus?file=src%2Fform-events-utils.ts
Usage
End feature - two functions (or more if there is interest, see end notes)
with usage like
and end values after calling the signal or subscribing like this, where there is always an initial value for either.
Process on PR: Marked for Review
Outstanding enhancements that I could slide in
function isStatusEvent<T>(event: ControlEvent | T): event is StatusChangeEvent
or this for getting the values of individual pieces of the form's data as either observables or signals:function statusEvents$<T>(form: AbstractControl<T>): Observable<StatusChangeEvent>
/function $statusEvents<T>(form: AbstractControl<T>): Signal<StatusChangeEvent | undefined>
.