html-like api vs react-like api for our components

[ismay]

Current situation

This is a proposal based on a chat I had with @HendrikThePendric. We were talking about the <Menu> component, and how it's a bit awkward that it has some state that it would need to communicate with the <SubMenu> he was conceptualizing.

A similar situation exists for the <SingleSelect> and <MultiSelect> with its options, the <Transfer> component and its options, and maybe other ones as well. Now I think we've talked about the above before, but I don't remember creating an issue for it, so I thought I'd create one to centralize the discussion a bit.

Benefits

The benefit of the current api is that it's reminiscent of how you'd compose regular html elements. I understand we also try to avoid incompatibility with web components, so we don't block that as a future upgrade path (and I might have missed other benefits).

Proposal

On the other hand, it does create a fair bit of extra complexity, with all the mapping and cloning of children, and the data flow in the components becomes a bit awkward. Where instead of everything flowing from the top component down, data flow is a bit more opaque. It feels less like a react-like flow and more like we're mixing approaches.

So the proposal is to move to an api where instead of specific children, the data is passed to the root component directly via a prop, as a simple object, instead of as react children. So a Select could accept an options prop, same with the Transfer, etc.

It'd be a big change, so I know it's quite a thing to propose. And there might valid usecases that this'd block. Nevertheless, it hass come up a couple times, so I thought I'd just document it somewhere, so we can form an opinion and remember why we like it or not.

[HendrikThePendric]

With regards to the Benefits section, I'd like to add the following perspective:

The idea behind the current API where we use children wherever possible is that this would produce a very composable set of components. However, in the cases mentioned above (Select / Menu / Transfer), the actual level of composability is more limited than it actually seems, mainly because the parent component makes (implicit) assumptions about the prop-API its children have.

One could argue that the above is in fact not an argument against the children-based API, but in favour of good documentation of custom children API requirements for components that clone children and append props...

[Mohammer5]

I've thought about that as well the last couple of weeks. Whenever I encounter react components not made by us, they usually use a options or some variant of config prop.

And lately it feels like we've fallen into a jsx trap, which is that we're looking at the code that will be used eventually to generate the dom and see it as html-like, while in reality it's functions and function calls. So from a functional programming perspective (input -> output), passing the data and the view separately should be preferred over mixing the data and view concepts. We can still achieve the same level of flexibility by accepting props that'd replace internally used components (of course only for components that we'd want to be changeable), which would work with web components as well.

Of course I'm not advocating for replacing the children prop in general, just for components that have to process their input before being able to display their children.

With the Menu component we'd "lose" the capability of replace one of the items with a custom component in a "plug'n'play" style, but on the other hand it's firstly not part of our design spec then (which means the data provided to the menu isn't handled) and secondly the item component could be replaced via a prop which allows consumers to create a item component that can handle their specific data and use case. (if they want to keep the existing styles, they can just create a "superset" of the existing item component).

Using an options object would reduce the complexity of the Transfer component quite a bit!

[amcgee]

I think an options prop makes a ton of sense for things like Select and Transfer especially!

I worry a little bit about things that can have deeper trees, though, like a menu with possibly multiple sub-menu levels. Could we cleanly support a deep tree of "data" in that case without adding a ton of overhead DSL for the structure of data passed into the component?

[ismay]

I think an options prop makes a ton of sense for things like Select and Transfer especially!

I worry a little bit about things that can have deeper trees, though, like a menu with possibly multiple sub-menu levels. Could we cleanly support a deep tree of "data" in that case without adding a ton of overhead DSL for the structure of data passed into the component?

The main problem that I've encountered is when components that are exported separately have some kind of shared state between them. Like the SingleSelect and a SingleSelectOption. In the Select we solved it by cloning the options and adding some props. Context would be another way I assume.

If you don't have that shared state, then it should be fine. With the Menu for example, it wasn't an issue until we wanted it to open on click, which meant it needed to keep track of what was clicked (maybe it can be circumvented by a different design btw., don't know).

So maybe the core of this issue is that we have certain components that have some sort of shared state/logic (so are coupled in a way), but are exported separately (and you'd expect them to be mostly decoupled). Connecting them is a bit awkward if they're exported separately. Or maybe we just need to find a better pattern for it.

[Mohammer5]

I worry a little bit about things that can have deeper trees, though, like a menu with possibly multiple sub-menu levels. Could we cleanly support a deep tree of "data" in that case without adding a ton of overhead DSL for the structure of data passed into the component?

@amcgee I thought the same, but now I think that - if an app has a complex menu - it probably will have some for of config-like object anyways, which would just need to be transformed (relates to MVVM?).
All props that the components now have would just become properties on the menu config object, except that children would have to be called something different (like menu or subMenu).
The actual transformation wouldn't become less or more complicated, but it wouldn't have to deal with components anymore, which I think is good.

It'd work with custom components as well, as you can just add any property to the config which would be ignored by the default components but can be picked up / used by custom components.

So I'd like to add "complex child-tree" to the issue @ismay identified (shared state between separately exported components),

[amcgee]

@Mohammer5 @ismay @HendrikThePendric I think it might be useful to sketch out the proposed prop-based API if we're going to move in that direction? What would this look like for the Transfer, Select, and Menu components?

[Mohammer5]

Transfer component

Current api

The options are currently provided as TransferOptions with the following props:

label: propTypes.string.isRequired,
value: propTypes.string.isRequired,
additionalData: propTypes.object,
className: propTypes.string,
dataTest: propTypes.string,
disabled: propTypes.bool,
highlighted: propTypes.bool,
onClick: propTypes.func,
onDoubleClick: propTypes.func,

The additionalData prop contains custom data that will be spread onto the options before passed to the filterOptions callback so the app can filter accordingly if customizing the filter mechanism.

New api

The transfer component would drop the children prop and instead have the following props added:

Transfer.propTypes = {
  // all previous proptypes except "children"
  options: propTypes.arrayOf(
    propTypes.shape({
      value: propTypes.string.isRequired,
      label: propTypes.string.isRequired,
      disabled: propTypes.bool,

      // can be used to use a different component for just one option
      component: propTypes.any, // any component
    })
  ),
  optionComponent: propTypes.any,
}

Transfer.defaultProps = {
  // all existing default props
  optionComponent: TransferOption,
}

• If a className or custom dataTest value should be passed to the TransferOption, the app would just have to create a wrapper component that does that.
• The onClick, onDoubleClick & highlighted props will be passed to the component by the Transfer component itself.
• additionalData can now just be added to the object itself and will simply be ignored by the Transfer component. That prop could be dropped on the TransferComponent

---

This would allow the app to either replace all or individual options with a custom component, so the flexibility stays the same.

[ismay]

@Mohammer5's example would also apply to the Select. So instead of:

<MultiSelect>
  {options.map(({ value, label }) => <MultiSelectOption value={value} label={label} />)}
</MultiSelect>

We'd have:

<MultiSelect options={options} />

[amcgee]

Am I correct in assuming that this also allows us to more easily support selected={value} instead of selected={{ value, label }}?

This seems like a great simplification to me

[Mohammer5]

The select components already do that. The Transfer still has to implement that

[HendrikThePendric]

@amcgee We've already made that transition for the SingleSelect and MultiSelect, I don't think it would have been a simpler transition if we used an options prop. So, I think we should not take this into account when considering a move away from our current HTML-like / children-based API

[Mohammer5]

Relates to dhis2/ui#60

The Transfer component already uses the new options prop api.