Compound components #864

vnys · 2020-11-12T08:25:43Z

vnys
Nov 12, 2020
Maintainer

Some of the components in the EDS have a parent child relationship. It does not make sense, for example, to use a <Cell /> component outside the context of the <Table /> component. To make this relationship clear in code, and also to share implicit state using the Context API between a component and it’s sub components, we have used the pattern of compound components.

Kent C. Dodds explains the rationale behind this pattern in his blog post React Hooks: Compound Components.

…and this is an example of how we use this pattern in the EDS Core React library

The way we’ve implemented this, using <Table /> as an example is that we have the following file structure:

└── Table
│   ├── Body.jsx
│   ├── Cell.jsx
│   ├── Head.jsx
│   ├── index.js
│   ├── Row.jsx
│   ├── Table.tokens.js
|   └──Table.jsx
└── ...

…then index.js is where we tie all the sub components together with the parent component like so:

import { Table } from './Table'
import { Body } from './Body'
import { Cell } from './Cell'
import { Head } from './Head'
import { Row } from './Row'

Table.Body = Body
Table.Cell = Cell
Table.Head = Head
Table.Row = Row

export { Table }

…and then the consumer could use destructuring to pick out the sub components:

import { Table } from '@equinor/eds-core-react'

const { Head, Body, Row, Cell } = Table

<Table>
  <Head>...</Head>
  <Body>...</Body>
</Table>

…or use dot notation:

import { Table } from '@equinor/eds-core-react'

<Table>
  <Table.Head>...</Table.Head>
  <Table.Body>...</Table.Body>
</Table>

This pattern has proved to work quite well, but in the process of refactoring all the components to Typescript, we’ve run into some problems with intellisense and compound types – and it does seem that this pattern is not particularly popular in typescript projects – most likely because of similar issues. The main problem is that if we rewrite the above in typescript, like so:

import { Table as BaseComponent } from './Table'
import { Body } from './Body'
import { Cell } from './Cell'
import { Head } from './Head'
import { Row } from './Row'

type TableCompoundProps = typeof BaseComponent & {
  Body: typeof Body
  Cell: typeof Cell
  Head: typeof Head
  Row: typeof Row
}

const Table = BaseComponent as TableCompoundProps

Table.Body = Body
Table.Cell = Cell
Table.Head = Head
Table.Row = Row

export { Table }

…then the intellisense you get in VS Code for example, is no longer the types of the <Table /> component, but the compound types, which isn’t very helpful and just adds a lot of unnecessary noise.

Diondre Edwards wrote a blog post about how to implement the pattern in typescript, but by looking at the Github repo the sub components are not properties on the parent component in the same way they are in Kent D. Todds original code – which is perhaps where we need to do things differently. Also worth noting the last comment in the original example:

The alternative then could be to rename all the sub components to something like <TableHead />, <TableBody /> and so forth – but the drawback is that it would make it more verbose to use the library for the consumers. A different approach could be to not export the compound types by using the @internal or @ignore tag in a JSDoc comment – at least we should investigate if that mitigates the problem.

mimarz · 2020-11-12T13:03:55Z

mimarz
Nov 12, 2020

@vnys api-extractor ran into problems trying to decipher the "release" tags when using compound components also

0 replies

pomfrida · 2020-11-13T14:02:56Z

pomfrida
Nov 13, 2020

List of the compound components with their respective sub components in our library that don't provide correct prop types in the intellisense in the IDE (if the sub components are not mentioned here they are OK):

Accordion
Banner (BannerIcon, BannerMessage, MannerActions)
Breadcrumbs
Card
Dialog (it doesn't have any props either, but we want consistency..)
Icon (is compound under the hood using Icon.add)
List
Snackbar (is compound under the hood using SnackbarAction)
Table (Cell, Body, Row, Caption)
TableOfContents
Tabs
Menu (MenuSection)

0 replies

mimarz · 2021-01-12T07:59:58Z

mimarz
Jan 12, 2021

One other problem discovered with using Typescript, Compound Components and letting our users extend our components can be confusing naming for exported Typescript Props when deconstructing from our library.

Example using `Table`:

We have Cell, Head, Body and Row as compound components of Table.
The exported props for the compound components are then named CellProps, HeadProps, BodyProps and RowProps.

Importing them along with other would then look something like:

import {
  Table,
  Typography,
  Icon,
  TableProps, // Table props
  CellProps, // Table.Cell props
  RowProps, // Table.Row props
  CaptionProps, // Table.Caption props
  BodyProps, // Table.Body props
  HeadProps, // Table.Head props
} from '@equinor/eds-core-react'

const { Caption, Body, Row, Cell, Head } = Table

Exporting component prop types on top level for compound/sub components does not feel like it will scale well, and cause confusion if your are getting intellisens for props types on components that are not clearly defined in the top level. For example, HeadProps can be confused with headings usage for other some other component, or standalone.

0 replies

mimarz · 2021-01-12T11:53:26Z

mimarz
Jan 12, 2021

Proposed solution 1

Keep compound components and update rest to match.
Examples of sub-component names: <Table.Cell>, <Topbar.Actions>, <Breadcrumbs.Breadcrumb>

Keep compound components
- Fix naming (remove prefix) on semi compound components, such as Card.CardActions -> Card.Actions (multiple components here)
Rename typescript prop types to include "parent" component
- Example; proptypes for Table.Cell would be name TableCellProps
We will still have problems with Microsofts api-extractor
Needs compound-component intersection type to work with Typescript. Example with TopBar
Looking at story in Storybook does not show deconstructing of sub-components which may cause confusing when story only shows function scope
- Fix stories to have full Sub-compnent path
Users needs to know somehow which components are compound and to deconstruct the "main" component for sub-components
Problems with react-docgen?
Commonjs tree-shaking using absolute paths on main components
Clearer definition of a components sub-components

Cost

Low, we only need to fix naming of exposed sub-components and update stories accordingly.

Usage

Alternative 1

import { Table } from '@equinor/eds-core-react'

<Table>
  <Table.Row>
    <Table.Cell>Text</Table.Cell>
  </Table.Row>
</Table>

Alternative 2

import { Table } from '@equinor/eds-core-react'

const { Row, Cell } = Table

<Table>
  <Row>
    <Cell>Text</Cell>
  </Row>
</Table>

4 replies

vnys Jan 13, 2021
Maintainer Author

Users still need to read the docs – that’s how they will know which components have sub components. I think this is a good pattern. It’s a shame it doesn’t work better with Typescript – there’s got to be somebody else who have solved this problem out there. I fear the component list will become convoluted.

Let’s say you have 10 components, each with 4 subcomponents. Now, looking at the list of those 10 components – it’s easy to get an overview and find the component you need. If we then lift all those sub components up one level – you have a list of 50 components, where some of them are main components and most of them sub components. Sounds like a mess.

When it comes to destructuering – that’s a pattern we have decided to use ourselves – but it’s not something the users have to use. There’s nothing stopping you from using <Table.Cell> instead of destructuring first just to be able to use <Cell> in your code. But with compound components at least you have a choice, which you don’t have with

<Table>
  <TableRow>
    <TableCell>Hi there 👋</TableCell>
  </TableRow>
</Table>

I think number one is the best option, if we can manage to solve it with Typescript.

vnys Jan 13, 2021
Maintainer Author

Also… what does Semantic UI React do that we don’t? How have they solved the problems we face?

mimarz Jan 14, 2021

Also… what does Semantic UI React do that we don’t? How have they solved the problems we face?

They are using the same compound-intersection type we have (example Topbar), but its split between their Dropdown.js and Dropdown.d.ts files

mimarz Jan 22, 2021

We went with proposal 1 due to cost and no clear preference from community (although we had two! @cbuv @ErlendLygre, thanks).

Fix: #1075

mimarz · 2021-01-12T11:55:33Z

mimarz
Jan 12, 2021

Proposed solution 2

Deprecate compound components in favour of more streamlined approach. This will make component names more verbose.
Examples of sub-component names: <TableCell>, <TopbarActions> , <Breadcrumb>

Remove compound components
- Change naming convention for sub-components, for example; Table.Cell becomes TableCell
Rename typescript prop types to include "parent" component
- Example; proptypes for Table.Cell would be name TableCellProps
No problem with Microsofts api-extractor
No compound-component intersection of types needed (less-code)
Looking at story in Storybook shows only functions scope, but name of component is the same as in library so its easier to see what needs to be imported from @equinor/eds-core-react
Better Intellisens as your are using the component directly and props are displayed correctly on for example mouse hover
All components are at same level in library, maybe avoids confusion?
Less problems with react-docgen?
Commonjs tree-shaking using absolute paths
Verbose definition of a components sub-components
Longer and maybe more poluted library list when looking at @equinor/eds-core-react

Cost

Medium, we need to remove compound components boilerplates and rename several components. Affected stories will also need to be changed.

Usage

Alternative 1

import { Table, TableCell, TableRow } from '@equinor/eds-core-react'

<Table>
  <TableRow>
    <TableCell>Text</TableCell>
  <TableRow>
</Table>

1 reply

wenche Jan 12, 2021

I think number 2 will be the best solution while moving forward 👍

ErlendLygre · 2021-01-15T14:00:48Z

ErlendLygre
Jan 15, 2021

As a user, I'll take compound components over the alternative any day. It just makes much more sense to me, and seems like a cleaner, more modern approach. The only problem i had with it, was that I couldn't find any info on handling subcomponents in Storybook or in the storefront.

If you do decide to keep it, please provide an example or notify us users. If not in the Storybook Stories themselves, then at least in the intro article or the developers-article in the Storefront.

1 reply

mimarz Jan 15, 2021

Thank your for your feedback!

We definitely have some cleaning up to do either way. We due also lack some documentation for this 😅

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compound components #864

{{title}}

{{editor}}'s edit

{{editor}}'s edit

Replies: 6 comments 6 replies

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

Select a reply

Compound components #864

vnys Nov 12, 2020 Maintainer

Replies: 6 comments · 6 replies

Example using Table:

Importing them along with other would then look something like:

Proposed solution 1

Cost

Usage

Alternative 1

Alternative 2

vnys Jan 13, 2021 Maintainer Author

vnys Jan 13, 2021 Maintainer Author

Proposed solution 2

Cost

Usage

Alternative 1

vnys
Nov 12, 2020
Maintainer

Replies: 6 comments 6 replies

Example using `Table`:

vnys Jan 13, 2021
Maintainer Author

vnys Jan 13, 2021
Maintainer Author