To redux-form or not to redux-form

mturley commented 6 years ago

I closed #47 because those comments were removed, but I'm going to leave this general issue open for any discussion about whether or not we should ditch redux-form. Had we started from scratch without it, I believe we would have been done in half the time... but we have sunk enough time into it and gotten it working after much frustration, so I see no immediate need to tear it out before the POC.

But if redux-form looks at me funny one more time I might delete it and rewrite the form validation with redux. It's form validation... it ain't that hard... and it wouldn't be more verbose so much as more explicit. It seems like redux-form decreases, rather than increases, the readability of our code (at least when it's breaking), because it tries to do too much magic. It is very unclear which components are passing what props where and how. Values magically get injected into input fields without a prop, so they can't be selectively overrided. And it's quite poorly documented (well, the docs are pretty, but the one i needed was a broken link).

The 2 big things we get out of redux-form are:

Implicit storage of form data in redux (arguable if that is even desired)
Pre-written validators for form data (not hard to roll our own, and there are alternatives like https://www.npmjs.com/package/react-validation). But if all we are doing is checking for required fields, it's trivial.

Low priority.. But I've had my 3 strikes with redux-form. I think I'd like to get rid of it in the long run. @priley86 I know you agree... @AparnaKarve I know you disagree. What do you like about redux-form? I bet we can easily emulate the good parts of it with our own code.

priley86 commented 6 years ago

yea i am wondering if any of those validators could live in patternfly-react... but will have to dive in some more soon. It was quite frustrating in some areas so I think it is worth further investigation.

AparnaKarve commented 6 years ago

When I went through some of the redux-form code examples, it looked really nice and simple. I mostly gave it high points because of how it handles validation and the metadata that it provides about the state of the field: error, warning, pristine, touched, visited etc to build a good UX, was exactly what I was looking for. Also the fact that the foreman project was also using it sealed the deal.

I would have definitely evaluated react-validation and compared the two, had I known about it before.

priley86 commented 6 years ago

re:

Implicit storage of form data in redux (arguable if that is even desired)

I continue to believe it is. Was considering that some this week... continuing to put more state in the individual components does not scale as well i.m.h.o., simply because, it can't be moved or shared as easily as redux state (which can be shared via the store keys and then selected individually anywhere). If I am putting that state in a component it can only be used in one place (and kind of makes testing that state a bit harder too i.mo.)... however I see where you are coming from if the form data is not shared beyond that form (but better to be safe than sorry?). I guess I am looking it more from the angle of things change and I want to reduce code digging as much as possible when refactoring or changing UIs. I believe we also have plans to start generating the entire redux "connected" component / domain file structure w/ yeoman. That should alleviate a lot of additional setup...

@mturley i also liked your point about finding ways to conditionally filter sensitive form data that would go to redux (that we would not want to share in a stacktrace/redux store log when recreating a bug). Maybe we add some kind of isSensitive flag to our redux stored domain models. There is still just so much value lost in easily recreating bugs and the entire timeline/history if we go w/o Redux...

Context apis / component state / composed state still have their usages though - I still think that is best served in pf-react when we are looking at simple UI state (expanded, hovered, etc.) that is not persisted or would never be shared across an application.

priley86 commented 6 years ago

I guess my biggest complaints w/ redux-form discovered so far were around the infinite loop scenario and what we found around the form resets (seems it would have been better to write our own handlers)? but there is maintenance to writing our own redux-form connectors too... and haven't made it far enough w/ the validators to know whether they are good/bad yet...

i think your initial summary above is pretty good...

thoughts?

mturley commented 6 years ago

I agree with everything you said, @priley86 . Also to clarify, I only meant that we might not want the redux persistence to be implicit. I definitely think we should persist those fields to redux, but doing so explicitly without a confusing library would have been fine. My thoughts are that we should probably ditch it in the long term, but in the interest of time and decluttering this repo, since we have invested a good chunk of time in redux-form, and because Ohad says this is being tracked in a wider context, I'm closing this issue for now.

If we want to revisit the use of redux-form in this repo post-POC, I'm all for that, we can reopen this issue at that time.

ManageIQ / manageiq-v2v

To redux-form or not to redux-form #50