fulcrologic / fulcro-rad

Fulcro Rapid Application Development
MIT License
201 stars 46 forks source link

form/handle-user-ui-props does not normalize components #84

Closed danskarda closed 1 year ago

danskarda commented 2 years ago

There are two ways how form initializes user ui props:

The first case properly initializes components and ui-props in subfoms. The second case does not handle subforms and uses plain merge instead of merge/merge-component.

How and why I noticed it:

My client wants to edit a form and its rules (1:n subform). To make writing rules easier client wants to see a time-series graph of past prices and application of current rules. After playing with reports, controls and forms, I concluded that the best way to implement this is to make read-only PriceHistory subform which has two date controls. But after implementation I found that controls are properly initialized only when I create new form. When I load data to a form then controls are empty. Data were not normalized and subform ::form/initizlize-ui-props were ignored.

awkay commented 2 years ago

I've fixed a different problem with this: joins in query-inclusions were not normalizing. That conflicted with your PR, and your PR had a more serious problem where using merge-component won't work right due to mark/sweep. See the fix I did for an idea of what needs to be done for the nested ui props.

On reflection, I think looking at query-inclusions at each level using component-options for nested forms makes more sense that what I'm currently doing (looking at ui props to infer which keys should be done, and how). I'll get to this at some point, but if you want to re-address it I'm game.

danskarda commented 1 year ago

Thanks you for thinking about the issue. I am sorry for not participating in discussion and solution. I was busy with other components of our systems (not related to Fulcro or Fulcro RAD), so it was hard to keep everything in head and manage some real life at the same time :)

Just to add my 2 cents: Looking back I am not yet proficient in Fulcro (RAD) internals to be sure my patch would be rock solid and work in all conditions.

I gave up on generally applicable solution and went for working app. My final solution (or rather workaround) does not need the patch. It takes advantage of the fact that controls are database entities. So if I have forms and its subforms, they share query-inclusion and values are initialed by the master subform when the form is set up.

High level question / reflection on the problem:

At this moment, when I compose two forms (form and its subform), the main form and its UISM takes over the behavior of a subform. This is very different from what we are used to in old OO / UI toolkits. In the old OO way object hierarchy intertwine state, behavior, dom hierarchy and position in space. Today I appreciate how Fulcro separates data, DOM and behavior / UISM. But it is hard to think new way :)

The really interesting design problem is a composition of behaviors and components. Now form UISM is hard-coded and takes over its subform. But can we make it more general? For example ship with Fulcro RAD master-detail behavior or nest forms / reports as you wish?

What are your thoughts on this?

awkay commented 1 year ago

Have you looked at the existing "container" stuff in RAD? Composition is always of interest to me; however, there was a really important reason to have the master form in control of all subforms: validation. Validation can be path dependent. So, what is valid for an entity on it's own may be different than for when it is embedded elsewhere. For example, an address might have loose constraints in one context, but as part of an order form for a US invoice might need to be a US address. That kind of thing. Also, the choice of fields can affect how things behave in a context. It seemed to me much better to have a single controller to think about with a "form set" than to have a bunch of moving parts interacting together in surprising ways.

Forms and reports are just components. They default to integrating with routing for convenience, but their lifecycle (start/end) can be managed by other means, which is how you go about combining them into any kind of container. They are individually otherwise self-contained, use normalized data, and should behave perfectly in any manner of composition as long as you start their respective state machines correctly (and potentially clear them from app state when routing away from them).