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(form events): Create unified observable and signal data accessors for form events #391

Merged
merged 17 commits into from
Jul 15, 2024

Conversation

michael-small
Copy link
Contributor

@michael-small michael-small commented May 27, 2024

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.

@michael-small michael-small changed the title feat(form events): Create unified observable and signal data accessor for form events feat(form events): Create unified observable and signal data accessors for form events May 27, 2024
@msmallest
Copy link

msmallest commented May 29, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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

@michael-small michael-small marked this pull request as ready for review June 4, 2024 15:36
@nartc nartc self-assigned this Jun 10, 2024
@msmallest
Copy link

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
Copy link
Contributor Author

michael-small commented Jun 12, 2024

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
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

  • value can be optionally stronger typed as T and not Partial<T> thanks to Josh
  • Edge case where ngOnInit value sets may not be accounted for in value now accounted for using RXJS defer. With new test.

If there is anything that is needed (docs, changes, clarification) then I am happy to help.

@nartc nartc merged commit 04db92a into ngxtension:main Jul 15, 2024
1 check passed
@msmallest
Copy link

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

@eneajaho
Copy link
Collaborator

eneajaho commented Aug 7, 2024

@michael-small
Before adding the docs page, I'd like to take this for a review, and maybe update some stuff first.
First thing I'd like to update is the naming of these functions. When working with signals I'd like to not see the signal name in the function so we can have sth like:

status = formStatus(this.form);  // status is a signal

status$ = fromFormEvents(this.form); // status$ is an observable that gets created from form events

allEventsSignal doesn't make much sense imo, because we are converting all those events to a unified signal, so the function loses the events meaning somehow. What do you think about the fromStatus function name? It immediately gives you an idea on what it will produce.

allEventsObservable naming is fine, but I'd like to have a more streamlined name. What do you think about fromFormEvents ? It uses the from* pattern like some other rxjs creation operators, and feels normal when being used.

@michael-small
Copy link
Contributor Author

michael-small commented Aug 7, 2024

  1. I agree that the naming could be better and aught to change. However, I do have some thoughts about "status" due to forms having a .status. Going to think on this and some possible alternatives if you don't mind.
  2. Also I have one other question about if it's too late to export some of the typing for the utils as well. Since changes to the source code are due with these proposed changes anyways, this has been something on my mind.

I'll follow up with more later tonight when I have more time

@michael-small
Copy link
Contributor Author

@eneajaho

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?

What do you think about the fromStatus function name? It immediately gives you an idea on what it will produce.

The name of "status" crossed my mind before, but part of the issue is that there is a FormControlStatus. There is also an issue with an alternative that I considered about referring to it as a "value", as FormControlValue is also closely tied to form data. However, I do think that your idea for naming makes more sense. I would be open to formStatus for the observable utility, but it may also make sense to workshop a different word to key off of.

status$ = fromFormEvents(this.form); // status$ is an observable that gets created from form events

fromFormEvents makes the most sense with the observable version in my mind. I think that would be best.

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 FormEventData<T> as it is good to have. Especially in conjunction with the signalSlice's exported type Source like my Stackblitz example.

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.

5 participants