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.

Sign up for GitHub

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
Open

Feat/emit initial value on init #296

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
567005f
added emitInitialValue
ophirKatz Apr 18, 2023
3289515
Added emitInitialValueOnInit feature, works on example
ophirKatz Apr 20, 2023
f631ce4
Removed unnecessary demo code, need to add tests
ophirKatz Apr 20, 2023
0539c3e
Updated README file to include documentation on emitInitialValueOnIni…
ophirKatz Apr 21, 2023
4b97f08
ran prettier linter
ophirKatz Apr 21, 2023
d451c51
testing broadcastDefaultValue
ophirKatz Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,16 @@ This function takes as parameter a configuration object and returns an object re

<!-- ❌✅ -->

| Key | Type | Optional or required | Root form | Sub form | What is it for? |
| ----------------------- | --------------------------------------------------------------------------- | -------------------- | --------- | -------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `formType` | `FormType` | Required | ✅ | ✅ | Defines the type of the form. Can either be `FormType.ROOT` or `FormType.SUB` |
| `disabled$` | `Observable<boolean>` | Required | ✅ | ❌ | When this observable emits `true`, the whole form (including the root form and all the sub forms) will be disabled |
| `input$` | `Observable<ControlInterface \| undefined>` | Required | ✅ | ❌ | A root form is a component in between the parent passing raw data and the form itself. This property is an observable that you must provide which will be used behind the scenes to update for you the form values |
| `output$` | `Subject<ControlInterface>` | Required | ✅ | ❌ | A root form is a component in between the parent passing raw data and the form itself. This property is an observable that you must provide which will be used behind the scenes to broadcast the form value to the parent when it changes |
| `manualSave$` | `Observable<void>` | Optional | ✅ | ❌ | By default a root form will automatically broadcast all the form updates (through the `output$`) as soon as there's a change. If you wish to "save" the form only when you click on a save button for example, you can create a subject on your side and pass it here. Whenever you call `next` on your subject, assuming the form is valid, it'll broadcast te form value to the parent (through the `output$`) |
| `outputFilterPredicate` | `(currentInputValue: FormInterface, outputValue: FormInterface) => boolean` | Optional | ✅ | ❌ | The default behaviour is to compare the current transformed value of `input$` with the current value of the form _(deep check)_, and if these are equal, the value won't be passed to `output$` in order to prevent the broadcast |
| `handleEmissionRate` | `(obs$: Observable<FormInterface>) => Observable<FormInterface>` | Optional | ✅ | ❌ | If you want to control how frequently the form emits on the `output$`, you can customise the emission rate with this. Example: `handleEmissionRate: formValue$ => formValue$.pipe(debounceTime(300))` |
| Key | Type | Optional or required | Root form | Sub form | What is it for? |
| ------------------------ | --------------------------------------------------------------------------- | -------------------- | --------- | -------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `formType` | `FormType` | Required | ✅ | ✅ | Defines the type of the form. Can either be `FormType.ROOT` or `FormType.SUB` |
| `disabled$` | `Observable<boolean>` | Required | ✅ | ❌ | When this observable emits `true`, the whole form (including the root form and all the sub forms) will be disabled |
| `input$` | `Observable<ControlInterface \| undefined>` | Required | ✅ | ❌ | A root form is a component in between the parent passing raw data and the form itself. This property is an observable that you must provide which will be used behind the scenes to update for you the form values |
| `output$` | `Subject<ControlInterface>` | Required | ✅ | ❌ | A root form is a component in between the parent passing raw data and the form itself. This property is an observable that you must provide which will be used behind the scenes to broadcast the form value to the parent when it changes |
| `manualSave$` | `Observable<void>` | Optional | ✅ | ❌ | By default a root form will automatically broadcast all the form updates (through the `output$`) as soon as there's a change. If you wish to "save" the form only when you click on a save button for example, you can create a subject on your side and pass it here. Whenever you call `next` on your subject, assuming the form is valid, it'll broadcast te form value to the parent (through the `output$`) |
| `outputFilterPredicate` | `(currentInputValue: FormInterface, outputValue: FormInterface) => boolean` | Optional | ✅ | ❌ | The default behaviour is to compare the current transformed value of `input$` with the current value of the form _(deep check)_, and if these are equal, the value won't be passed to `output$` in order to prevent the broadcast |
| `handleEmissionRate` | `(obs$: Observable<FormInterface>) => Observable<FormInterface>` | Optional | ✅ | ❌ | If you want to control how frequently the form emits on the `output$`, you can customise the emission rate with this. Example: `handleEmissionRate: formValue$ => formValue$.pipe(debounceTime(300))` |
| `emitInitialValueOnInit` | `boolean` | Optional | ✅ | ✅ | Controls whether the form (root or sub) will emit the default values provided with the `FormControls` if they are valid. The default behavior is not to emit the default values |

# Principles

Expand Down
11 changes: 7 additions & 4 deletions projects/ngx-sub-form/src/lib/create-form.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,15 +152,18 @@ export function createForm<ControlInterface, FormInterface extends {}>(

const broadcastValueToParent$: Observable<ControlInterface> = transformedValue$.pipe(
switchMap(transformedValue => {
const valueChanges = formGroup.valueChanges.pipe(
options.emitInitialValueOnInit ? startWith(transformedValue) : tap(),
);
if (!isRoot<ControlInterface, FormInterface>(options)) {
return formGroup.valueChanges.pipe(delay(0));
return valueChanges.pipe(delay(0));
} else {
const formValues$ = options.manualSave$
? options.manualSave$.pipe(
withLatestFrom(formGroup.valueChanges),
withLatestFrom(valueChanges),
map(([_, formValue]) => formValue),
)
: formGroup.valueChanges;
: valueChanges;

// it might be surprising to see formGroup validity being checked twice
// here, however this is intentional. The delay(0) allows any sub form
Expand All @@ -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

}),
options.handleEmissionRate ?? identity,
);
Expand Down
1 change: 1 addition & 0 deletions projects/ngx-sub-form/src/lib/ngx-sub-form.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export type NgxSubFormOptions<
formControls: Controls<FormInterface>;
formGroupOptions?: FormGroupOptions<FormInterface>;
emitNullOnDestroy?: boolean;
emitInitialValueOnInit?: boolean;
componentHooks?: ComponentHooks;
// emit on this observable to mark the control as touched
touched$?: Observable<void>;
Expand Down