clbond / ng2-redux-form

Connect Angular 2 forms to Redux stores
17 stars 7 forks source link

Code Review Notes #1

Closed SethDavenport closed 7 years ago

SethDavenport commented 8 years ago

Overall I like the approach. I also like the API: connect a form and you're done, this lib is just glue between the Angular forms API and the store.

Some notes:

Docs

Needs a README :) Just a basic usage example will do for now. We can beef it up as we go.

API

We should make sure we output a type definition file as part of the build process, particularly for defaultFormReducer and provideReduceForms.

How about renaming provideReduceForms to provideFormReducer?

The Connection directive exposes the connect attribute. Might be worth harmonizing the naming to the same thing?

Action prefix:

I notice it's using @@redux-form as the action prefix. We should probably change this to our own, e.g. @@ng2-redux-form or similar.

Immutable getIn stuff

I saw your comment about this and I'm not super convinced that it won't be a perf issue. I've seem some projects with lots of very large forms... it's only a matter of time until someone decides to build 'Excel in the browser' with this :)

That said, probably something we can look at in 0.0.2 :)

We could expose the getIn implementation from ng2-redux and beef it up a little. Or we could use lodash or something.

Webpack

Kudos for webpacking it, but declaring externals. It might be worth bringing this to ng2-redux itself at some point, although that's a very low priority at this point.

Package.json audit

clbond commented 8 years ago

Wow! Thank you Seth. Excellent feedback. I will implement these changes tomorrow. Much appreciated

clbond commented 8 years ago

OK. I believe I have addressed these issues. Would you mind taking a second look when you get some free time?

SethDavenport commented 7 years ago

Looking good. A couple of things with the peer deps but I'll just make a PR.