ngxtension / ngxtension-platform

Utilities for Angular
https://ngxtension.netlify.app/
MIT License
544 stars 80 forks source link

feat(form events): Create unified observable and signal data accessors for form events #391

Closed michael-small closed 2 weeks ago

michael-small commented 2 months ago

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)

function allEventsObservable<T>(form: AbstractControl<T>): Observable<{
    value: T;
    status: FormControlStatus;
    touched: boolean;
    pristine: boolean;
    valid: boolean;
    invalid: boolean;
    pending: boolean;
    dirty: boolean;
    untouched: boolean;
}>

function allEventsSignal<T>(form: AbstractControl<T>): Signal<{
    value: T;
    status: FormControlStatus;
    touched: boolean;
    pristine: boolean;
    valid: boolean;
    invalid: boolean;
    pending: boolean;
    dirty: boolean;
    untouched: boolean;
}

with usage like

import { allEventsObservable, allEventsSignal } from 'ngxtension/form-events';

@Component({
    // ....
    template: `
        <form [formGroup]="form">
            <label for="firstName">Name</label>
        <input formControlName="firstName" name="firstName" />
        </form>
    `,
})
export default class FormEventsComponent {
        // ....
    form$ = allEventsObservable(this.form);
    $form = allEventsSignal(this.form);
}

and end values after calling the signal or subscribing like this, where there is always an initial value for either.

{
  "value": {
    "firstName": "Dave",
    "lastName": ""
  },
  "status": "VALID",
  "touched": true,
  "pristine": false,
  "valid": true,
  "invalid": false,
  "pending": false,
  "dirty": true,
  "untouched": false
}

Process on PR: Marked for Review

Outstanding enhancements that I could slide in

  1. There could be more utilities like this, but I don't want to overwhelm.
    • For example, non exported functions which are helper functions for the end two functions could be beneficial to expose. For the 4 types of form data (value, status, pristine, touched), there is two types of helpers that could be exported as well: type narrowing: 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>.
    • For the moment being, all of these helpers even if they are not used are included in the util file, though ones not directly used are commented out. They do not all have initial values, since that was done for the two functions that are the focus of this PR. But if these helpers are to be considered then they can be refactored to have initial values.
  2. There are two other form events that this unified control events API exposes: submitted and reset.
    • However, those are moreso what I call for a lack of technical terms "event events"; they do no have synchronous accessors like the other four types of events. And that makes sense, as they are events that happen at a given point of time but they do not necessarily make sense to keep track of synchronously.
    • It could probably been done and I have some ideas for that, but I want to see interest in the other functionality of this proposed util first.
msmallest commented 2 months ago

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 valueChanges/statusChanges as well as these events requires some hacky solutions, which I outline in the issue. To remedy this, I can extend the functionality of this util to take in different parameters, so that the form is passed down as a signal input, and allEventsObservable can take form: AbstractControl<T> | Signal<AbstractControl<T>>, and allEventsSignal could take form: Observable<AbstractControl<T>> | AbstractControl<T>. Here is a proof of concept doing it with a similar util that I made with valueChanges

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 AbstractControl<T> shape. Idk. See the commented out examples, and the neighboring formValuesFromFormInputWorkaround$ or $formValuesFromObservableInputWorkaround which do work but are transformed a bit first.

michael-small commented 1 month ago

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.

michael-small commented 1 month ago

One thing I have considered but I am leaving out unless people want: equivalent event property names. For example, untouched in addition to already having touched, dirty in addition to pristine, etc.

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 valid rather than just status: 'VALID' | 'INVALID' | 'PENDING' | 'DISABLED' (aka FormControlStatus) .

msmallest commented 1 month ago

One thing I have considered but I am leaving out unless people want: equivalent event property names. For example, untouched in addition to already having touched, dirty in addition to pristine, etc.

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
}
michael-small commented 1 month ago

One thing I have considered but I am leaving out unless people want: equivalent event property names. For example, untouched in addition to already having touched, dirty in addition to pristine, etc.

After making the example and messing around with it I decided to add these

msmallest commented 1 month ago

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.

michael-small commented 1 month ago

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 .getRawValue, despite { nonNullable: true }, etc. I think I have been a bit pulled away from noticing this directly due to how some of my approaches of using it in effect kind of coerces partials.

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 value properties and some links to how to work with that.

edit: as an aside but also a new update, I am also going to tweak it to use defer like this https://fullstackbuff.dev/stories/defer-form-value-changes

joshuamorony commented 1 month ago

@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

michael-small commented 1 month ago

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.

michael-small commented 1 month ago

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.

msmallest commented 2 weeks ago

Woo, thank you for including this feature! I'll write up a doc page and put in a PR for that soon.