Handle getting value from disabled form controls in `form-events` util

michael-small commented 1 month ago

Something occurred to me just now

Problem

The current state of the form-events utils (allEventsObservable and allEventsSignal, names of them may change in a future version) doesn't handle getting the current value of form controls if a particular one is disabled: https://stackblitz.com/edit/stackblitz-starters-j5jucv?file=src%2Fform-events-utils.ts,src%2Fmain.ts

Example form group

  fb = inject(NonNullableFormBuilder);
  form = this.fb.group({
    firstName: this.fb.control('', Validators.required),
    lastName: this.fb.control(''),
  });

Here is a scenario that the current util's code and the stackblitz demonstrates

  ngOnInit() {
    this.form.setValue({ firstName: 'Michael', lastName: 'Small' });
    this.form.controls.firstName.disable(); <--- this is the issue
  }

  // Value of "value" given by the utils if `firstName` was disabled, 
  //     since the util currently doesn't use the `.getRawValue()` trick, 
  //     but rather the `ValueChangeEvent`
  // "value": {
  //  "lastName": "Small"
  // },

This is because the value of a reactive form changes if a control is disabled. However, people like myself included normally use form.getRawValue() to get the value of all controls even if they are disabled. And this was my original intent as well, but it got lost as these utils were made, and because the util handles a whole form group being disabled fine but single controls being disabled is the issue I forgot about.

Proposed Solution

However, with a proposed change, the value would include disabled individual controls like this: https://stackblitz.com/edit/stackblitz-starters-ujq78h?file=src%2Fform-events-utils.ts,src%2Fmain.ts

  ngOnInit() {
    this.form.setValue({ firstName: 'Michael', lastName: 'Small' });
    this.form.controls.firstName.disable(); <--- this would be handled by the `.getRawValue()` change I am proposing
  }
  // "value": {
  //   "firstName": "Michael",
  //   "lastName": "Small"
  // },

This proposed change would replace using the value change events from the new unified form events API and just use the old fashioned .getRawValue() mapping like in the example code listed.

Considerations

I would just make a PR for this now, as my original intent was to account for this. And in my experience, people commonly want to use the raw value trick to always get the value, form control being disabled or not. Here is an example of that discussion that I weighed into which spurred on me making this issue: https://old.reddit.com/r/Angular2/comments/1f9501j/best_way_to_force_nonnullable_form_control_values/llj6f9s/.

HOWEVER, this may arguably be a breaking change. I see this as a bug from my intent and some people's intent when they want a form's value, but others may see this as a feature.

However that however: the util as is handles a whole form group being disabled by still giving its value, so that is another weird aspect of what may be expected out of the util. I think that is a point towards making this change however.

Lastly, and probably a huge factor

The util still doesn't have docs: https://github.com/ngxtension/ngxtension-platform/pull/451
It will probably be renamed: https://github.com/ngxtension/ngxtension-platform/pull/391#issuecomment-2273165339

So it is still up in the air for some changes and I am guessing not that used. But I don't want to assume.

Thoughts? I think the proposed solution would be good, but the considerations may be relevant to users

eneajaho commented 1 month ago

@michael-small The api is experimental currently so we can change stuff -> and break too.

michael-small commented 1 month ago

@eneajaho sounds good, I'll make this change and then get the doc finished up.

By the way, what do you think about the naming of fromFormEvents and fromFormState https://github.com/ngxtension/ngxtension-platform/pull/391#issuecomment-2274812549 ?

eneajaho commented 1 month ago

fromFormEvents and fromFormState make sense but we need a way to distinguish what they produce.

from* gives us the idea that it generates an observable.

I don't like the from* pattern for signals tbh

michael-small commented 1 month ago

@eneajaho ok, I've been dragging my feet too long on cleaning up all the assorted changes and documentation of this util. I will get this change squared away in a PR soon and then move onto finishing the doc PR.

When I put in the PR for this issue I'll @ in whatever place you think would be best to pickup naming talk. It has been discussed in either the closed original util PR and this issue, so idk if you want to pick it back up in the other PR or we should keep it here. Or new issue? Probably overkill idk.

I don't mean to be such a pain about this, but I just feel a similar caution about formStatus giving the idea it is about the form's literal .status as you do about from* giving the idea of an observable.

ngxtension / ngxtension-platform