[RFC] Creating Components

[levithomason]

Most UI components require a common set of behavior such as styling and accessibility. There should be a single point of responsibility for implementing these behaviors.

Goals

1. 👍 Provide a single path for creating components.
2. 👍 Allow future extension for base features, such as state machines.
3. 👍 Expose a public API for consumers to create components.
4. 👍 Support cascading the CSS-in-JS renderer and theme in the React tree.
5. 👍 Support overriding component styles, variables, and theme.
6. 👍 Allow passing component state to the style function
7. 👍 Keep the component on top in the render tree for optimal testing and debugging.
8. 👍 Consumer's should be able to react to props/context changes using shouldComponentUpdate.

Restrictions

1. 👎 Avoid private and deprecated APIs.
2. 👎 Avoid polluting props with computed values that aren't part of the public API
3. 👎 Limit the number of wrapping elements in the render tree
4. 👎 Keep the component first in render tree if wrapping components are required

Proposals

We explored the following approaches. The below code snippets simplified for ease of review.

createComponent(Component, config)

A HOC that wraps the provided component and passes features through props.

import imageRules from './imageRules'
import imageVariables from './imageVariables'

const Image = (props) => {
  const { classes } = props

  return <img className={classes.root} />
}

export default createComponent(Image, {
  rules: imageRules,
  variables: imageVariables,
})

<Render />

A component that takes config as props and computes all the values necessary to create your component.

class Image extends Component {
  render() {
    return (
      <Render
        handledProps={[ ... ]}
        state={StateMachine}
        props={this.props}
        rules={...}
        variables={...}
        render={({ ElementType, rest, classes }) => (
          <ElementType {...rest} className={classes.root} />
        )}
      />
    )
  }
}

Directly render consumers

Directly import context consumers and use them in the render method to access the CSS-in-JS renderer and theme.

import Provider from '../Provider'
import imageRules from './imageRules'
import imageVariables from './imageVariables'

class Image extends Component {
  render() {
    return (
      <Provider.Consumer
        render={(theme) => {
          const classes = getClasses(this.props, imageRules, imageVariables, theme)

          return <img className={classes.root} />
        }}
      />
    )
  }
}

HOC Context

A function to be returned in the render method. Calls back with the theme. Implemented by using a theme consumer in a function.

import Provider from '../Provider'
import imageRules from './imageRules'
import imageVariables from './imageVariables'

const Image = (props) => {
  return hocContext((theme) => {
    const classes = getClasses(props, imageRules, imageVariables, theme)

    return <img className={classes.root} />
  })
}

Others

We tested and ruled out these approaches as they rely on the legacy context API and private fela APIs.

• BaseComponent - Extend a base class which provides context and a method for creating style classes.
• Functional context - "Hides" usage of the legacy context API. Adds statics to the component and provides a function for accessing context.

Conclusions

• Multiple wrapping elements in the React tree are unavoidable due to the need for cascading themes and CSS-in-JS renderers.
• ...

[dvdzkwsk]

Don't you also want the error message?

[levithomason]

The message is included at the top of the stack.

[mnajdova]

Don't we want to extend here the React.Component, and not the BaseComponent? Is this a typo?

[levithomason]

Fixed, thanks!

[levithomason]

There has been a lot of conversation around this. The majority of participants in the conversation are leaning away from the createComponent() HOC. The preference is toward a pattern where the render method of a component is responsible for wiring the base functionality of the component.

// better :)
const Image = (props) => {
  return (
    <Render
      rules={imageRules}
      props={props}
      render={({ classes }) => <img className={classes.root} />}
    />
  )
}

// worse :/
const Image = (props) => {
  const { classes } = this.props
  return <img className={classes.root} />
}

export default createComponent(Image, { rules })

Why? When the functionality is added from within the component vs from outside the component:

1. Better debugging - The render tree shows the component on top and the wrapping components inside, vs the component being wrapped in several HOCs.
2. Easier test access - See 1. Since the component under test is rendered on top, it is easier to access it and make assertions. There is no need to hoist properties as in the case of HOCs.
3. Avoids polluting props - An HOC requires passing computed values through the component's props. It is not a design goal nor ideal to pass extra information through props, it is only a consequence of computing the values above the component. Computing values in the render method means the props of the component are not polluted with arbitrary values. The props API is limited to values that are useful to the user.
4. Using state to style functions - The render method approach means local component state can be passed to the style function. With an HOC wrapping the component, there is no equally clean way of getting state from the component back up to the HOC for use in the style function.

For these reasons, I'll add the <Render /> pattern to this PR for consideration. We may also further explore the pattern of returning a function in the render method. This accomplishes the same thing as the <Render /> component, but does not add yet another wrapping component to the tree:

// best?
const Image = (props) => {
  return renderComponent({ rules, props }, ({ classes }) => (
    <img className={classes.root} />
  ))
}

[kuzhelov]

If we'll consider the first approach - one thing that would like to clarify for myself to get the full picture.

// better :)
const Image = (props) => {
  return (
    <Render
      rules={imageRules}
      props={props}
      render={({ classes }) => <img className={classes.root} />}
    />
  )
}

Would we need to mock the module of Render component in that case? Would it be about something like the following?

// ------ Image.spec ------------
import render from './render'
mock(render.default, RenderMock)

import Image from './image'

.. actual tests ..

Thanks!

[kuzhelov]

just few words around the following goal:

👍 Allow us to add static members and methods to components

It looks like even with render function approach - or, generally speaking, with any approach taken - we would be able to cover this aspect by introducing additional function that should be used to provide static members to component.

Specifically, this is how the render function approach might look like (simplified)

import { renderWithStyles, withStaticMembers } from './core'

class MyComponent {
     render = () => renderWithStyles((theme, classNames) => (<div className={classNames}> ... <>);
}

export default withStaticMembers(MyComponent);

Please, note that, essentially, any other approach that is suggested so far (but, I might bet, literally, any) could use the same pattern. With that being said, it seems that requirement of 'having general functionality to introduce static methods' is something that shouldn't be a decisive factor, due to this moment could be addressed pretty easily as a separate piece of common logic.

Also, if would take into account that there are conformance tests that will check whether specific properties are present or not for the exported component, the approach when we will divide common rendering aspects (with render function) and common logic to introduce static properties (via dedicated function) seems to be a promising thing even from the extensibility and maintainability point of view, due to there would be very low risk of coupling these aspects in the implementation code.

[levithomason]

'having general functionality to introduce static methods' is something that shouldn't be a decisive factor

Agreed, removed.

[levithomason]

@mnajdova has merged an update here that implements the renderComponent() function. Tests were also updated. The render tree is great with this approach and our conformance tests were greatly simplified.

We will merge this soon and use this pattern as our approach. We are open to changes, but they need to prove material superior to this one.

[brianespinosa]

I've been lurking and generally following the conversation here until I have some extra time to throw at contributing. I really like this final solution. It cleans a lot of things up. Another benefit I think we get here is a pattern that is easier to follow for new contributors.

[dzearing]

Hmm. How does this split out state from the view? Are they still intertwined? Where would state be managed?

I preferred the createComponent approach primarily because it provides an opportunity to enforce state/view separation and could enforce good coding practices (e.g. views should be dumb stateless components that take in props only):

export const Toggle = createComponent({
  state: toggleState,
  styles: toggleStyles,
  view: toggleView,
  statics: { defaultProps: ... }
});

The way I see props flowing is as such:

user props => optional state transforms => styling => final view props used for view

The "view" could be enforced as a functional thing, implying that both 1. it is the simplest logic possible for the render code, and 2. we could literally call the function to avoid adding any view overhead to reduce the react wrapper code.

This means for partners, if you want to re-scaffold a component to change around the composed subcomponent orientation, (e.g.) a chevron icon moves to the far side or near side) it's easy. Just create a component that provides all the same values except a new view.

You could technically even expose an augmentation API surface to take an existing component and easily replace parts of it.

I like this because it makes it easy to "glue" together separate pieces, rather than to create monolithic components that manage state, props, and rendering all in one place.

Because UIComponent based on React.Component, when and how would PureComponent ever be used? There are clearly cases where PureComponent could cut down on rendering. Or, are we avoiding pure because it's too likely to create difficult to understand regressions? I know there is a history here, but just wondering when there are clear opportunities, how this might be supported.

[mnajdova]

Guys, I have made some changes in the implementation of the UIComponent, that you can check in my pull request with the custom resolver for the react-docgen: #42 The change is in the following: I moved the implementation of the renderComponent service to the render method of the UIComponent in order to avoid passing the overridden renderComponent method as a callback. This is done in order for us to be able to use this in the context of the renderComponent method, which we cannot use if we are using it as a callback to the renderComponent function. I pushed the changes there, because the docs side is working and we can actually try the examples for the components.

[levithomason]

I moved the implementation of the renderComponent service to the render method of the UIComponent in order to avoid passing the overridden renderComponent method as a callback. This is done in order for us to be able to use this in the context of the renderComponent method, which we cannot use if we are using it as a callback to the renderComponent function.

We should be able to handle calling any function with a different context using function methods like .bind(), .call() or .apply(). These methods allow setting the this value inside the function.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@96fd1cb). Click here to learn what that means.
The diff coverage is 82.84%.

@@            Coverage Diff            @@
##             master      #21   +/-   ##
=========================================
  Coverage          ?   67.33%           
=========================================
  Files             ?       66           
  Lines             ?     1047           
  Branches          ?      190           
=========================================
  Hits              ?      705           
  Misses            ?      339           
  Partials          ?        3

Impacted Files	Coverage Δ
src/components/Icon/fontAwesomeIconRules.ts	20% <ø> (ø)
src/components/Provider/Provider.tsx	52.38% <ø> (ø)
src/components/Text/textRules.ts	0% <0%> (ø)
.../components/Accordion/accordionContentVariables.ts	0% <0%> (ø)
src/components/Icon/iconRules.ts	93.75% <100%> (ø)
src/components/Divider/dividerRules.ts	80% <100%> (ø)
src/components/Label/labelRules.ts	100% <100%> (ø)
src/components/List/ListItem.tsx	81.81% <100%> (ø)
src/components/Menu/Menu.tsx	100% <100%> (ø)
src/components/Button/buttonRules.ts	100% <100%> (ø)
... and 34 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96fd1cb...ad7dea6. Read the comment docs.

[levithomason]

I've finally caught up with all the merged PRs. All components now extend the UIComponent class. This will be merged now, though, we aren't opposed to improvements moving forward.