FormBuilderLoader shouldn't be a component

[ScopeyNZ]

Twice now with silverstripe-elemental we've had to deal with unmounting and remounting forms:

The first time we were allowing the user to "collapse" the form. Simple workaround - we can just display: none the form when collapsed rather than removing it from the DOM.

The second time we are allowing the user to drag and drop to re-order forms (whether they're visible or not). This is causing issues when using FormBuilderLoader as the component is re-mounted and then a fetch occurs.

It seems to me that FormBuilderLoader really doesn't need to be a component that is mounted. Why not provide a service that can load the schema and form state for a FormBuilder, provide a simple default (keeping FormBuilderLoader for that purpose, but make it extremely lightweight)? This would allow us to load schema and state for a FormBuilder and store it in a component (or store) that isn't going to be unmounted.

Please provide any thoughts or recommendations for this.

Additionally, redux will remove state for forms when they're unmounted, but there's a configuration setting for redux-form > 6.3.1 that turns that behaviour off. Doesn't work for us as a fetch always re-occurs when FormBuilderLoader is re-mounted.

[robbieaverill]

This is a pretty important part of the future of the React ecosystem, so a weigh in from @silverstripe/core-team would be good here

[flamerohr]

Yeah, totally agree with all points, FormBuilder could be broken down to 3-4 different HOCs with available hooks and callbacks which are opt-out automatic :)

I'm also of the (strong) opinion to remove redux-form in favour of Formik, but I digress.

A few parts come to mind off the top of my head:

• the loader/fetch
• the schema handling
• the form handling
• the validation handling

[ScopeyNZ]

Yeah, totally agree with all points, FormBuilder could be broken down to 3-4 different HOCs with available hooks and callbacks which are opt-out automatic :)

Agreed for the most part. Some things I think we don't even need as HOCs. Why bother putting form schema loading and state loading into an HOC? State is currently not stored in a component right? It's stored in redux, seems like it's sort of counter intuitive to have it in a component (plus I feel like "make an HOC" is an overused pattern in React).

I'll work on a POC soon then I'll have my head around everything in a little more detail.

[unclecheese]

I'm of two minds on this one. I think both approaches are useful.

You can kind of think about FormBuilderLoader like Apollo's graphql() function. It provides a component with the API and mechanics needed to fetch data from some agreed upon source, and makes no presumptions about how that data will be used (that's FormBuilder's job). That's all good, but what we found out with Apollo is that sometimes HOCs are a pain. They're singletons, and basically immutable for the entire lifecycle of the application. i.e. if the schema URL changes, you're not in a great position to re-render.

Apollo solves the unmounting problem with global state. It assumes there's a client somewhere in the context that is negotiating all of this fetching, and when it sees something it's seen before, it handles it gracefully and the loader doesn't have to know anything about it. FormBuilderLoader on the other hand is much more deterministic. Subsequent mounts look exactly like the ones that preceded them.

With that said, I wonder if we just need a FormBuilderClient floating around in the ether to manage the fetching.

State is currently not stored in a component right? It's stored in redux, seems like it's sort of counter intuitive to have it in a component

Duplicitous state isn't nearly as big a concern as unpredictable state. Since the data flows one way, having seemingly redundant state is rarely a problem. It makes thing a lot more testable and scalable, if say, someday we jump of the Redux train in favour of some other state management solution.

[flamerohr]

Agreed for the most part. Some things I think we don't even need as HOCs. Why bother putting form schema loading and state loading into an HOC? State is currently not stored in a component right? It's stored in redux, seems like it's sort of counter intuitive to have it in a component (plus I feel like "make an HOC" is an overused pattern in React).

Overused is maybe to the inexperienced, I don't see it that way at all. It's a great pattern to get into for composing the sort of props, functions and components you need (e.g. Injector in SilverStripe admin, and even redux itself is an HOC). But this is an entirely different conversation to this thread.

I also think having form state in redux is an anti-pattern, but it's nice for the customisation level (if you don't mind the brittle-ness that comes with that)

I'll follow up with Aaron's post later tonight, tough to read currently :)

[flamerohr]

That's all good, but what we found out with Apollo is that sometimes HOCs are a pain. They're singletons, and basically immutable for the entire lifecycle of the application. i.e. if the schema URL changes, you're not in a great position to re-render.

That sounds more like a pain with Apollo's HOC architectural design than HOCs itself.

Maybe I'm not understanding something here, but HOCs are just functions that act as component factories with potentially a configuration, they're not singletons - unless you're talking about the functions themselves being the singleton?

[ScopeyNZ]

Overused is maybe to the inexperienced

Granted. I'm still pretty new to React.

In the end I've managed to resolve my issue with minimal changes to FormBuilderLoader: #654 . I still think it makes sense to allow users of FormBuilder to manage their own state, but FormBuilderLoader does more than just loading state...

[flamerohr]

I still think it makes sense to allow users of FormBuilder to manage their own state, but FormBuilderLoader does more than just loading state...

Yeah, understandable, it's a fair bit of thinking and mangling to get state-handling and state-loading separated and may not even be completely separated theoretically :/

Granted. I'm still pretty new to React.

I hope I didn't cause any hard feelings, but I try to encourage "explore it before knocking it" :) And HOCs are one of those things you could explore in so many ways with various experiences. I do empathise the overwhelming nature of it when someone new comes aboard.

[unclecheese]

I don't mind the knock on HOCs per se. I think most implementations of an HOC can be substituted with render props, and I find the latter much more developer friendly.

The problem I have with the overuse of HOCs is that they sidestep what React does best, which is declarative, transparent representations of UI, where as render props embrace that and leverage it.

If you take, for instance, the hypothetical formBuilder HOC, my guess is that it looks something like this:

const SchemaAwareForm = formBuilderLoader(FormComponent);

<SchemaAwareForm schemaUrl={...} />

That schemaUrl prop is now implicit, and without seeing the implementation, I really have no idea what that function does. Whereas, this, to me, tells a better story:

<FormBuilderLoader>
{(schemaResult) => <div>I can be anything you want!</div>}
</FormBuilderLoader>

Personally, I think overuse of HOCs is one of the conditions that make people say React is hard to learn.

tl;dr My preference would be to keep FormBuilderLoader and use render props.

[flamerohr]

yeah, fair enough. I guess I view it differently with that regard. It provides an additional wrapper or behaviour which the original Component does not need to concern itself with directly. in that example schemaUrl is implicit in the sense that FormComponent doesn't declare it, but it shouldn't stop the formBuilderLoader() HOC making it explicit either.

I've also found re-render issues with the render props pattern, so it could lead to confusion if you're unfamiliar with how React chooses to re-render a Component.

[robbieaverill]

@ScopeyNZ does #654 close this?

[ScopeyNZ]

Not really. I still think that splitting up FormBuilderLoader would serve CMS module authors better - However I did fix the issue of unmounting and remounting FormBuilderLoader with #654.