Add localState ngrxDirective option

[mucaho]

@MrWolfZ
As discussed in issue #165 , this PR adds:

• The ngrxSuppressGlobalActionEmit input boolean to ngrx's directives, which controls whether to dispatch form actions to the global ActionsSubject or not, false by default of course to not change current semantics. I thought this was better wording than createLocalFormState, but I can adapt this of course.
• The ngrxFormAction output event emitter that always emits any form action. It does not hurt if the action is always emitted, regardless of ngrxSuppressGlobalActionEmit, right?
• Unit and e2e tests
• A representative example showcasing the benefit of using local state. Showcases how to handle state update on inter-dependent form state and additional local state used in the component. Showcases how to properly detect changes in lieu of input properties, but still being as efficient as possible.
• Does not include any documentation. To be frank, I do not know where to put and what to write what's not already showcased in example. Can change this of course, depending on your input.

I hope everything checks out, otherwise let me know.

(e: Improved upon the example and some wording, pushed again)

[codecov]

Codecov Report

Merging #166 into develop will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           develop   #166   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files          125    127    +2     
  Lines         2349   2385   +36     
  Branches       425    429    +4     
======================================
+ Hits          2349   2385   +36

Impacted Files	Coverage Δ
src/group/local-state-directive.ts	100% <100%> (ø)
src/module.ts	100% <100%> (ø)	⬆️
src/control/local-state-directive.ts	100% <100%> (ø)
src/group/directive.ts	100% <100%> (ø)	⬆️
src/ngrx-forms.ts	100% <100%> (ø)	⬆️
src/control/directive.ts	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d974581...727dcc6. Read the comment docs.

[MrWolfZ]

Thanks a lot for this thorough PR. Overall it looks great. Here's a couple of comments:

The ngrxSuppressGlobalActionEmit input boolean to ngrx's directives, which controls whether to dispatch form actions to the global ActionsSubject or not, false by default of course to not change current semantics. I thought this was better wording than createLocalFormState, but I can adapt this of course.

I like this design because it keeps the whole local form state concept in the directive without bleeding into the rest of ngrx-forms. At the same time I dislike its verbosity, because you need to pass the parameter to every single form control (which also makes it easy to accidentally forget adding it on a control). If there was an isLocal property on the form state then the directive could decide based on that, but this would require some changes to other areas. Thinking about it, there may also be a slightly different API design that at least takes care of the verbosity issue: instead of passing an additional flag there could be a new directive that uses ngrxLocalFormControlState (or ngrxFormLocalControlState) as a selector. This design still has the issue of it being possible to accidentally mix local and global form states though. What do you think?

The ngrxFormAction output event emitter that always emits any form action. It does not hurt if the action is always emitted, regardless of ngrxSuppressGlobalActionEmit, right?

Yeah, its fine to always emit locally.

A representative example showcasing the benefit of using local state. Showcases how to handle state update on inter-dependent form state and additional local state used in the component. Showcases how to properly detect changes in lieu of input properties, but still being as efficient as possible.

I feel that the example is maybe a bit too advanced to make the feature easy to understand. There should be at least one basic example that showcases purely how to handle only local form state without any interaction with effects etc, i.e. showing that this feature allows using ngrx-forms without ngrx. There could then be a second more advanced example for how to connect a local form with actions from the outside.

Does not include any documentation. To be frank, I do not know where to put and what to write what's not already showcased in example. Can change this of course, depending on your input.

Yeah, a good example is always important. Even though the text documentation may not contain any extra details, it is still useful, since there are different people out there that learn through different things. Some may prefer looking at an example, others may prefer reading through documentation. For this feature we could probably just add a new markdown file local-forms.md to the user-guide section.

Overall, great work. Let's see if we can nail down the API design. While it's great that the PR is already so complete, in the future I would recommend to not start writing any documentation/examples before the design is finalized to save yourself some unnecessary refactoring/rewriting.

[MrWolfZ]

the reason for e2e.spec/select.spec.ts is that there were some tricky interaction issues with angular itself. This is unrelated to the local form state. It's still a good idea to have an end to end test though, but I'd put it in a separate local-state.spec.ts file and maybe use just an input instead of the more complex select

[mucaho]

Was not able to trigger a simple change event manually on an input element and have ngrx forms register it properly. Extracted the example into a separate spec file though.

[mucaho]

Thanks for the review, i will incorporate all the remarks once we settle on the API, as you said.

One thing I am not certain about: For now we have the ngrxFormAction output on both ngrxFormState and ngrxFormControlState. Should the output be called ngrxFormControlAction in the latter?

At the same time I dislike [the ngrxSuppressGlobalActionEmit input boolean's] verbosity, because you need to pass the parameter to every single form control (which also makes it easy to accidentally forget adding it on a control). If there was an isLocal property on the form state then the directive could decide based on that, but this would require some changes to other areas. Thinking about it, there may also be a slightly different API design that at least takes care of the verbosity issue: instead of passing an additional flag there could be a new directive that uses ngrxLocalFormControlState (or ngrxFormLocalControlState) as a selector. This design still has the issue of it being possible to accidentally mix local and global form states though. What do you think?

• The isLocal form state property would be the least verbose one, and once you have set it, you would not have to think about using the proper input / directive for all your form controls. While this sounds nice I'm not a big fan of mixing form state with configuration options.
• ngrxLocalFormControlState has the same issue of being accidentally mixed up or forgotten about like ngrxSuppressGlobalActionEmit. However, it's less verbose as you eliminate the need for this additional input.

In my opinion another set of directives (ngrxLocalFormControlState & ngrxLocalFormState) would be the best option.

If you agree, I would proceed with this approach. However, in this case the question remains how to go about the implementation. As far as I can tell, there are two options:

• make ngrxLocalFormControlState directive extend ngrxFormControlState directive, passing the former's output EventEmitter in the super constructor as the ActionsSubject. A bit hacky since EventEmitter being a subject is an implementation detail, which theoretically could change in future releases.
• rename current ngrxFormControlState directive to ngrxLocalFormControlState. Remove ActionsSubject from constructor and only emit to the local ngrxFormAction output.
  Make a new directive called ngrxFormControlState which has the same constructor parameters plus the ActionsSubject and overrides the dispatchAction method by also emitting to this ActionsSubject. (And hope for the best that angular inherits all decorators properly).

[MrWolfZ]

One thing I am not certain about: For now we have the ngrxFormAction output on both ngrxFormState and ngrxFormControlState. Should the output be called ngrxFormControlAction in the latter?

Hmm, I am honestly divided. In the grand scheme of things this is probably just a minor detail though, so any option is fine. We could also just call it ngrxFormsAction to indicate these are actions from this library. Choose whatever feels right to you.

In my opinion another set of directives (ngrxLocalFormControlState & ngrxLocalFormState) would be the best option.

Yeah, let's go with this design.

If you agree, I would proceed with this approach. However, in this case the question remains how to go about the implementation. As far as I can tell, there are two options:

I was thinking about this as well. I would go for the inheritance approach. You can simply make the ActionsSubject constructor parameter @Optional() and then provide null as a value to the super class constructor in the new directive. In addition you would keep the dispatchAction method and just override it in the subclass to emit on the local EventEmitter instead of the ActionsSubject. This should work for both directives.

[mucaho]

Updated comment:

Scratch everything I wrote before, I can make the new directive's selector be [ngrxFormControlState][ngrxFormsAction] and change the old one's to [ngrxFormControlState]:not([ngrxFormsAction]) and everything looks to be working properly.

Old comment for reference:

I encountered some unexpected roadblocks while trying to implement the directive ngrxLocalFormControlState as a subclass of ngrxFormControlState:

• My e2e test kept failing, much to my surprise. Reason is that the new directive ngrxLocalFormControlState can not find any @Self-scoped FormViewAdapters. After some investigation I found out that these are added by the same setter property ngrxFormControlState in the various adapters. While adding another selector and input setter would be possible for in-house viewAdapters that same can not be said for external adapters that exist already and rely on this naming convention. e.g. for NgrxRadioViewAdapter:

@Directive({
  selector: 'input[type=radio][ngrxFormControlState]',
  // ...
})
export class NgrxRadioViewAdapter implements FormViewAdapter, OnInit, AfterViewInit {
  @Input() set ngrxFormControlState(value: FormControlState<any>) { /*...*/ }
}

• The documentation section on user-defined properties uses the same, aforementioned naming convention. Same problem again.
• Same problem for CustomErrorStateMatcherDirective, although I don't know for what that's used.

Here the implementation for reference:

@Directive({
  // tslint:disable-next-line:directive-selector
  selector: '[ngrxLocalFormControlState]',
})
export class NgrxLocalFormControlDirective<TStateValue extends FormControlValueTypes, TViewValue = TStateValue>
  extends NgrxFormControlDirective<TStateValue, TViewValue> {

  @Input() set ngrxLocalFormControlState(state: FormControlState<TStateValue>) {
    this.ngrxFormControlState = state;
  }

  @Output() ngrxFormsAction = new EventEmitter<Actions<TStateValue>>();

  constructor(
    el: ElementRef,
    @Optional() @Inject(DOCUMENT) dom: Document | null,
    @Self() @Optional() @Inject(NGRX_FORM_VIEW_ADAPTER) viewAdapters: FormViewAdapter[],
    @Self() @Optional() @Inject(NG_VALUE_ACCESSOR) valueAccessors: ControlValueAccessor[],
  ) {
    super(el, dom, null as any as ActionsSubject, viewAdapters, valueAccessors);
  }

  protected dispatchAction(action: Actions<TStateValue>) {
    this.ngrxFormsAction.emit(action);
  };

}

---

If you know of a good way of introducing another directive, while still being backward-compatible let me know. A cool solution would be to apply the new directive on all elements that have the output binding too, something like selector: [ngrxFormControlState] (ngrxFormsAction), but that does not work of course.

Otherwise, maybe we should go ahead with the original approach. Looking forward to hear your thoughts.

[mucaho]

Changes from previous version:

• Added the local directives. as mentioned in previous comment they are added if you bind an event listener to (ngrxFormsAction), otherwise the default directives are used.
• Add an introductory local state example.
• Added documentation.

Codecov is failing because I did not test the branches where the @Optionally injected ActionsSubject is null. Should I throw an error in this case? It should never happen, as I override the dispatchMethod in that case when it's null.

[MrWolfZ]

Scratch everything I wrote before, I can make the new directive's selector be [ngrxFormControlState][ngrxFormsAction] and change the old one's to [ngrxFormControlState]:not([ngrxFormsAction]) and everything looks to be working properly.

Yeah, that's a pretty cool approach. For reference in the future, I believe you can also have multiple selectors for a component, e.g [ngrxFormControlState],[ngrxLocalFormControlState]. However, I believe your solution is much more elegant since it is much less invasive than using a different attribute as a selector. I'll take a look at the code.

[MrWolfZ]

Great work, this is looking really good. As you can see my comments are mostly minor things (for many of the formatting comments I should really tweak the linting rules).

Let's also bring test coverage to 100% and then this is good to merge.

[MrWolfZ]

you can remove the casts when the base class constructor accepts null

[mucaho]

See #166 (comment)

[MrWolfZ]

i'd prefer form[ngrxFormState]:not([ngrxFormsAction])

[mucaho]

Tried that initially, but the selector won't work this way.

[MrWolfZ]

type as ActionsSubject | null

[mucaho]

See #166 (comment)

[MrWolfZ]

can remove cast when base constructor accepts null

[mucaho]

See #166 (comment)

[mucaho]

Alright, applied all proposed changes, or left responses where it's not possible to do so.
I hope we are good to go now.

[MrWolfZ]

Yup, we are good. Thank you very much for your work.

[MrWolfZ]

And it's published as version 6.1.0.

[mucaho]

Thanks for all your input, I am glad we managed to find a nice API and provide it with examples & documentation!

I see you have done some linting afterwards, thanks!
As you have mentioned already before, I think you can add tslint to the npm test script, so the CI check fails if it's not properly formatted. This would remove the need for the manual chore work of fixing formatting, as you can run tslint --fix to resolve most of the problems.