angular-redux / form

Keep your Angular2+ form state in Redux
MIT License
41 stars 15 forks source link

Does not work with reactive forms. Only works with template forms #22

Closed GenZodd closed 7 years ago

GenZodd commented 7 years ago

This is a...

What toolchain are you using for transpilation/bundling?

Environment

NodeJS Version: 6 Typescript Version: 2.2 Angular Version: 4 @angular-redux/store version: 6.2.0

Expected Behaviour:

I thought this would work with both reactive and template driven forms. When I set it up via a template driven forms it works. But when I try and add a reactive form to the page I get an error about "no provider for ngForms".

Is this going to/should support reactive forms?

<form [formGroup]="loginForm" connect="loginForm">
            <div class="form-group">
              <label class="center-block">Email</label>
              <input class="form-control" formControlName="email" />
            </div>
            <div class="form-group">
            <label for="password">Password</label>
            <input show-password class="form-control" id="password" type="password" formControlName="password">
          </div>
          </form>

private createForm() {
        this.loginForm = this.fb.group({ 
            email: ['', Validators.required],
            password: ['', Validators.required]
        });

Instead of the template approach found in the sample app (feedback form).

Actual Behaviour:

Returns ERROR Error: Uncaught (in promise): Error: No provider for NgForm!

frederikaalund commented 7 years ago

The problem occurs because NgForm is not declared on <form> components that use either the ngNoForm or formGroup attributes. The latter is the case here. See the CSS selectors for the ngForm directive for reference.

It can be trivially fixed by splitting the connect attribute into two:

// For template forms (with implicit NgForm)
@Directive({ selector: 'form[connect]:not([formGroup])' })
class ConnectTemplateDirective {
    constructor(protected form: NgForm, ...) { ... }
    ...
}

// For reactive forms (without implicit NgForm)
@Directive({ selector: 'form[connect][formGroup]' })
class ConnectReactiveDirective {
    @Input('formGroup') form;
    ...
}

I recommend using a base class to implement the common logic.

JohannesHoppe commented 7 years ago

Is a fix planned? Thanks for the hard work on angular-redux! :+1:

neilrussell6 commented 7 years ago

Can you provide an example of this selector (form[connect][formGroup]) being used in the html?

JohannesHoppe commented 7 years ago

In template driven world that's easy: <form [connect]="['personalInfo', 'myForm']">. This issue is about reactive forms!

neilrussell6 commented 7 years ago

Have you managed to implement the "For reactive forms " solution posted above by @frederikaalund ? If so can you post your usage?

JohannesHoppe commented 7 years ago

Nope, I haven't tried. We switched to template-driven for the moment.

frederikaalund commented 7 years ago

I'd like to add that my fix is not a work-around. Is is one possible way to fix angular-redux/form itself.

neilrussell6 commented 7 years ago

Ok cool, thanks for the clarification. Will wait for the fix.

JohannesHoppe commented 7 years ago

@frederikaalund Why don't you send a PR!? 👍

frederikaalund commented 7 years ago

@JohannesHoppe I would normally but I don't actually use @angular-redux/forms. I don't have a setup where I can quickly test my fix. Was a Plunker of the issue ever posted? That would make it a lot easier.

I actually use my own simple store directive that is bi-directional (changes from the store also goes into the form).

I ran into exactly the same issue as posted here while implementing my own directive. I just noticed that my fix also could be applied to @angular-redux/forms.

GenZodd commented 7 years ago

I took a shot at this but in trying to run the tests I now get this error.

ERROR in ./source/tests.entry.ts Module build failed: TypeError: Cannot set property 'babelOptions' of undefined at Object.ngcLoader (D:\GitHub\angular-redux\form\node_modules\@ngtools\webpack\src\loader.js:395:34) 12 06 2017 17:55:08.461:ERROR [karma]: { Error: no such file or directory at MemoryFileSystem.readFileSync (D:\GitHub\angular-redux\form\node_modules\memory-fs\lib\MemoryFileSystem.js:107:10) at MemoryFileSystem.(anonymous function) [as readFile] (D:\GitHub\angular-redux\form\node_modules\memory-fs\lib\MemoryFileSystem.js:300:34) at doRead (D:\GitHub\angular-redux\form\node_modules\karma-webpack\lib\karma-webpack.js:201:29) at Plugin.readFile (D:\GitHub\angular-redux\form\node_modules\karma-webpack\lib\karma-webpack.js:205:5) at _combinedTickCallback (internal/process/next_tick.js:73:7) at process._tickCallback (internal/process/next_tick.js:104:9) code: 'ENOENT', errno: 34, message: 'no such file or directory', path: '/_karma_webpack_/source/tests.entry.ts' }

Not sure how to resolve.

Update....I get this same error with the based forked repo as well.

GenZodd commented 7 years ago

Ok have this working locally (minus the test still for some reason). Have it broken apart and template approach is all working. However, I can't get reactive to work still as I get this error. ERROR Error: Uncaught (in promise): TypeError: Cannot read property 'constructor' of undefined TypeError: Cannot read property 'constructor' of undefined

Pretty sure this has to do with it not finding the form to get it set up. My HTML looks like this. <form connect="loginForm" [formGroup]="heroForm" novalidate>

I have tried this: <form connect="loginForm" formGroup="heroForm" novalidate>

That gets me past the constructor error but then, of course, angular errors because it can't bind its validators. Can anyone point out what I am missing?

Update: Actually, I think have this solved and I am close. The resetState method needs a bit of an update for differences between ngForm and FormGroup controls.

GenZodd commented 7 years ago

PR has been submitted. Hopefully the group can review soon.

GenZodd commented 7 years ago

This library seems to have been abandoned so I am just removing the forms module from my code.

frederikaalund commented 7 years ago

This library seems to have been abandoned

Sad to hear that is the official word. I sort of came to the same conclusion when I first found this project. A redux-form module is really needed.

Anyhow, thanks for all the effort that you put into fixing this issue!

SethDavenport commented 7 years ago

I've been planning to revamp this as part of our 7.0 roadmap - so maybe this is an opportunity in disguise. I'd like to collect some feedback on what people would be looking for in a reworked forms module.

In the meantime, I can get that pr merged as part of the 6.4 series as long as the breaking API change comment is resolved.

Then let's collect some community reqs (and maybe some help? :) ) and do it better for 7.

frederikaalund commented 7 years ago

Great news!

Should we just post our requirements here or do you plan to create a "feature requests" issue?

SethDavenport commented 7 years ago

Give @angular-redux/form@6.5.0 a whirl and let me know.

SethDavenport commented 7 years ago

As for the feature list, feel free to start an issue for now. We'll formalize a release out of it. One item is toolchain - it needs simplifying and the unit tests need fixing.