Alternative to 'renderX' set of props for slots

[kuzhelov]

Let this issue serve as a discussion hub for alternative approach to renderXXX one that is currently used for Stardust components.

TL;DR Here is the list of main reasons for that:

• downsides of renderXXX
  • necessity to bloat API surface with renderXXX methods for each slot of each component
  • general incorrectness of shorthand evaluation for async scenarios
  • it is easy to for the client to accidentally use this API improperly
  • points of confusion related to API
• suggested alternative approach
  • is able to cover all the cases covered by the original one
  • introduces safe API that could be easily explained to user
  • is able to correctly handle async shorthand rendering cases

Please, note that scenario-based comparison examples for both approaches are also provided.

---

Contents

• Motivation
• Proposed Solution Overview
  • Idea
  • Example
  • Benefits
• Scenario-based comparison
  • Explain Concept
  • Async Rendering
  • Custom Tree Rendering
  • Selectively Apply Custom Rendering

Motivation

There are several problems with current `renderItem` prop - let me mention them.

1. "Do It Yourself" pattern provided by `renderXXX` to compose component

Lets consider the API we now have for renderItem:

/**
* Its client responsibility now to not forget about:
* - consuming props as a second argument
* - spread these props to the component
* 
* Failure to do these steps will result in the problems 
* that could be hard to immediately detect
* - broken/incorrect styles
* - broken/incorrect component behavior/accessibility behavior
**/
<Menu items={items} renderItem={(Component, props) => {  // <== don't forget 'props' !
    ... 
     return <Component {...props} />    // <===== oh, don't forget to expand 'props'!
}/>

As we might see, there are quite a lot of things that client should worry about now - thus, this approach is quite error-prone.

2. Shorthand is evaluated at different stage for async case

For synchronous case we've had that shorthand item was rendered after its descriptor object (i.e. element of items array) has been fully initialized.

Now lets move to async case. For the sake of example lets suppose that we have only key data being defined for each item, and that content data should be fetched for it.

const items = [
  { key: 'a' },      // <----- no 'content' for these initially
  { key: 'b' }
  ...
]

With renderItem approach we have shorthand (Component, props) evaluation happened before data is fetched - while, to comply with synchronous semantics of shorthand, we need this evaluation to happen after necessary data is fetched. Note that this might be absolutely necessary if shorthand evaluation relies on some data that is fetched asynchronously.

<Menu 
  items={items} 
  renderItem={(Component, props) => {   // <== note, shorthand is already evaluated!
   <Async 
     getData={() => ...}
     render={content =>            
       <Component {...{...props, ...{content: data} }} />}     //  <=== but should be here
   />
}/>

3. Bloat of additional props needed for each component

With renderXXX approach we need to introduce new render function for each slot of each component. With the alternative approach there won't be any need to support render functionality on the component's side - once written it will be automatically reused by all components.

4. Points of general confusion for client

General question that immediately arises for renderXXX approach is how shorthand's object data is passed to renderItem? It turns out that all the item's data is merged into component's props provided in the callback - arguably, not something very intuitive for the client.

<Menu items={[ 
  { key: 'a', content: ..., },
  { key: 'b', content: ..., }
  ...
]} 
renderItem={(Component, props) => ... how item's data is provided to callback? .. }

Proposed Solution

General Idea

TL;DR General idea is to support callback function for shorthand, where render function as an argument.

Full Story

Stardust has a notion of shorthand to provide a declarative way for specifying components that should be rendered - in a form of object or React Element. Specifically, for the following case we have a Menu which items array is declared by object shorthands used:

<Menu items={[
  { key: 'a', content: '....' }
  { key: 'b', content: '...' }
  ..
]} />

However, at the very end each object is evaluated to React Element when Menu is rendered. Thus, the following one will provide identical results: here each object of items array is transformed to MenuItem element (with necessary props being evaluated and properly applied).

<Menu items={[
   <MenuItem key='a' content='...' accessibility='...' />
   <MenuItem key='b' content='...' accessibility='...' />
  ..
]} />

So, essentially, we could imagine some transformShorthandObject function that produces React Element from the object provided - and this is, essentially, the only piece hidden from the client when shorthand is provided in a form of object. This hidden piece, render (transformShorthandObject) function, is the corner stone of the proposed approach.

Example

Essentially, these two will provide identical rendered Menu:

<Menu items={[
   { key: 'a', content: '...' },
   { key: 'b', content: '...' },
   ...
  ]} />,

<Menu items={[
   render => render({ key: 'a', content: '...' }),
   render => render({ key: 'b', content: '...' }),
   ...
  ]} />,

Benefits

• only one prop is (re)used
• doesn't introduce need to provide renderXXX prop for all slots of all components
• solves the problems of renderXXX approach mentioned above
• meet all the merit points of original approach, namely

• Define shorthand data (strings, numbers, props objects)
• Control the render tree of the shorthand components
• Accept and use state, styling, and accessibility computed by the Menu
• Asynchronously render child shorthand components

• doesn't introduce any semantic changes to the process of shorthand evaluation

---

Scenario Examples

Explain concept to consumer

Suppose that we have the following code as the originally written. Following example provide identical effect as this original one.

<Menu
  items={[
    { key: 'a', content: 'custom chat message' },
    { key: 'b', content: 'custom chat message' },
    { key: 'c', content: 'custom chat message' },
  ]}
  ...
/>

Before

<Menu
  items={[
    { key: 'a', content: 'custom chat message' },
    { key: 'b', content: 'custom chat message' },
    { key: 'c', content: 'custom chat message' },
  ]}
  ...
  renderItem={(Comp, props) => <Comp {...props} />}
/>

Proposed

<Menu
  items={[
    // 'done' callback semantics
    render => render({ key: 'a', content: 'custom chat message' }), 

    { key: 'b', content: 'custom chat message' },
    { key: 'c', content: 'custom chat message' },
  ]}
  ...
/>

Thoughts

• not clear how objects from items are used in renderItem
• client is required to compose rendered tree by herself, there is a possibility to accidentally introduce bugs that would be hard to discover. Simplest example - forget to spread props to element.
• clear 'callback' semantics for alternative proposal - the same approach that is commonly used bto allow customizations in general processing logic. Just few examples of domains where applied
  • async unit tests (Jasmine, Jest)
  • middleware pipeline (Express)
  • build pipeline (Gulp)
  • ...
• only one prop (item) is utilized for the alternative proposal - this will prevent potential inconsistent use of two items and renderItem props that are dependent on each other now.

Async Rendering

Suppose that client's intent is to render each item's shorthand asynchronously, once all the necessary shorthand data is fetched.

Also, for the sake of argument, suppose that client's code have some component that provides abstraction for data fetching logic - e.g. Async, that provides the following basic props

• getData - defiines logic for async data fetching
• render - defines logic to render tree once data is fetched

const urls = [
  'http://url-a',
  'http://url-b'
]

Before

const mapKeyToUrl = {   // <-----------------  should be defined and maintained
   'a': urls[0],
   'b': urls[1]
} 

// .......

<Menu
  items={[
    { key: 'a' },
    { key: 'b' },
    { key: 'c' },
  ]}
  renderItem={(Comp, props) => (
    <Async
      getData={() => fetch(mapKeyToUrl[props.key])}  // <---------- maps item to URL
      render={asyncData => <Comp {...{...props, ...{content: asyncData} }} />}
    />
  )}
/>

Proposed

<Menu
  items={urls.map(url => renderItem => (
    <Async
      getData={() => fetch(url)}
      render={asyncData => {
        const withContent = { ...item, ...{ content: asyncData } }
        return renderItem(withContent)
      }}/>
   ))}
/>

Thoughts

• with proposed solution there are no changes made to the process of evaluating shorthand object to element (by render method), semantics remain to be the same: once shorthand descriptor object is fully ready, pass it to the render method to evaluate the shorthand. With renderXXX approach we see the shorthand object being precomputed first (implicitly, based on the item), decomposed to Comp and props - and when async operation finishes, its client responsibility to properly compose the object back. Thus, with the only need of async rendering being introduced, client is provided with additional irrelevant responsibility to properly compose the element.
• problem related to aforementioned one - with current approach shorthand is evaluated for the object that doesn't contain full set of data necessary (as some data should be fetched). This requires us to maintain the following (quite strong!!) invariant for shorthand evaluation function to ensure that our logic is consistent and correct:

Evaluate(shortandProps + additionalProps }) = Evaluate(shorthand) + Evaluate(additionalProps)

• with the alternative approach we don't introduce any restrictions of linearity to shorthand evaluation functions. In fact, we are not introducing any additional restrictions with alternative approach, which opens much broader space for future maneuvers.
• *not strictly related to async case, but still - note that now, with current approach, client is not able to specify url directly as prop of the item in items array, as what intuitive thought would be - because in that case this will be merged to props that are passed in renderItem callback. Thus there is a need to introduce this keyToUrl mapper.

Custom Tree Rendering

Before

  <Menu
    items={[
      { key: 'a', content: 'custom chat message' },
      { key: 'b', content: 'custom chat message' },
      { key: 'c', content: 'custom chat message' },
    ]}
    .....
    renderItem={(Comp, props) => (
      <Comp {...props} >
        My cool subtreee!
      </Comp>
    )}
  />

Proposed

OPTION 1: Identical approach, use the same Comp, props tools so that the same set of cases could be addressed.

  <Menu
    items={[
      { key: 'a', content: 'custom chat message' },
      { key: 'b', content: 'custom chat message' },
      { key: 'c', content: 'custom chat message' },
    ]
    .map(item => render =>
        render(item, (Comp, props) => 
           <Comp {...props} >
             My cool subtreee!
           </Comp>
        )
    )}
  />

OPTION 2: Safer approach

  <Menu
    items={[
      { key: 'a', content: 'custom chat message' },
      { key: 'b', content: 'custom chat message' },
      { key: 'c', content: 'custom chat message' },
    ]
    .map(item => render =>
        render({ ...item, children: <>My cool subtreee!</> })
    )}
  />

Thoughts

• quite often client doesn't need to change the <Component {...props} /> part - and this what is guaranteed to be properly handled by alternative approach
• in cases where it is necessary, though, there is a possibility to utilize second argument of the render function - and, thus, it is possible to fully cover the same scenarios that are possible now.

Selectively apply special rendering

In this example lets suppose that our need is to fetch data for only one/several elements of Menu - and lets suppose that we have to use the Async component for that (for example, in case of Apollo-based app).

Before

    <Menu
      items={[
        { key: 'a' },    // <-------- item with unknown content, fetched async
        { key: 'b', content: 'custom chat message' },
        { key: 'c', content: 'custom chat message' },
      ]}
      renderItem={(Comp, props) => {
        if (props.key === 'a') {
            <Async
                getData={() => fetch(urlsMap[props.key])}
                render={asyncData => (
                    <Comp {...{...props, ...{content: asyncData} }} />
                )}
            />}

        return <Comp props /> 
    }}
 />

Proposed

    <Menu
      items={[
        // item with unknown content, fetched async
        render => <Async 
            getData={() => fetch('http://get-a')} 
            render={asyncData => render({ key: 'a', content: asyncData })} />,

        { key: 'b', content: 'custom chat message' },
        { key: 'c', content: 'custom chat message' },
      ]}
    />

Thoughts

• no 'opt-in' for renderItem prop case (the one we have now) - this will result in switch semantics being needed from client - either by if-switch expressions, or by using dedicated map object.

[mnajdova]

@kuzhelov thanks for the detailed RFC, pointing the issues of the current implementation and providing alternative one.
I have several points for supporting the proposed API

1. Avoiding the necessity to bloat the API

First, I would really appreciate if we can avoid the necessity to bloat the API with adding new property for each slot inside each component. Apart from this, the user will only have to remember just one usage of the shotrhand, and not having to make a decision about which one he/she should use, which I am sure will result in much cleaner and bug-free code.

2 Avoiding the 'Do it yourself' pattern

As a second point, I would mention your first point for avoiding 'Do it yourself' pattern, which again in the current implementation can lead to bugs that, for the user, may be hard to be located.

Conclusion

With this being said, and the examples provided, we can be sure that the proposed API looks much promising then the initial implementation. However we must consider that, the initial implementation is so far used by the client, and it is working as expected. If we go with this implementation, which I really think we should, we should provide very clean instructions about how the initial implementation should be migrate to the new one, and of course we should be sure that we won't reduce some of the functionality of the initial implementation (I am not saying that we are, but I haven't seen the implementation of the second one, and this is something that we just can't do).
So, in conclusion, I support the alternative proposed API, but if we decide to go with it, we should start with the implementation as soon as possible, because it would be nightmare for the client to go over through every usage of the async rendering once more, and replace with the alternative API.

[kohlikohl]

Thank you @kuzhelov for putting this detailed proposal together.
I like what is being proposed here and it seems to be the most intuitive solution, not bloating the API and providing flexibility.

If I understand it correctly, it it's most basic form render() takes whatever the shorthand would have received plus additional props that would be applied to , hiding <Component {...props}>?
As long as additional props can still be passed down to <Component> that should be fine.

On a side note regarding async rendering: Have we considered the react suspense API for Async components. It is still experimental, but since it is throwing Promises, it seems possible to surface data from a child component up to a parent, causing the parent to re-render with the new set of data. (cc: @levithomason)

[levithomason]

So others know, @kuzhelov is working on implementing this. Could you link the PR here if/once it is up?

[kuzhelov]

@kohlikohl, sorry for the late response on that.

Let me, please, address two main concerns/questions you've raised:

If I understand it correctly, it it's most basic form render() takes whatever the shorthand would have received plus additional props that would be applied to , hiding <Component {...props}>?
As long as additional props can still be passed down to that should be fine.

yes, exactly - as you've mentioned, in the basic case render({ .. shrothand .. }) will be evaluated to proper element plus + client props (provided in 'shorthand' object) + evaluated props (in most general case). So, in case if client would need to pass additional props to component, it could be done in the following ways:

<Button icon={render => { 
  const additionalProps = getAdditionalProps(...)
  return render({ name: 'user', ...additionalProps })
} />

or if rendered tree should be in full control (like it was always the case for render method approach), the following could be used:

<Button icon={render => { 
  const additionalProps = getAdditionalProps(...)
  return render({ name: 'user' }, (Component, props) => <Component {...props} {...additionalProps} />)
} />

---

On a side note regarding async rendering: Have we considered the react suspense API for Async components. It is still experimental, but since it is throwing Promises, it seems possible to surface data from a child component up to a parent, causing the parent to re-render with the new set of data. (cc: @levithomason)

Absolutely - actually, generally speaking, any approach that would be about hoisting state to state management parent component (that will propagate updated data to its children) is a preferred approach in comparison to <Async> components - and Suspense leads to one of these preferred approaches which Stardust will support with no changes necessary.

On a side note - at the present moment it is clearly seen (according to tweets and blog posts of React community) that this Suspense feature will be introduced almost for certain (unless if only some critical flaw would be found) :)

[kuzhelov]

closed by #519