instructure/instructure-ui

Unable to use React Fragments or normal components to return required types of parent components in InstUI

aaronshaf opened this issue · 5 comments

Background Information

Package Version(s):
8.x

Browser:
Netscape Navigator

OS:
BeOS

Component:
Problem includes components like <Table> and <List>

Describe the Bug

I cannot use a React Fragment (<>) or a custom component (Foo) to return a list of <List.Item> to populate <List>. Doing so results in a propType error. Such errors break our tests in Canvas.

For now we have to use workarounds.

Steps To Reproduce (courtesy of Cameron Ray)

const MyCustomTable = () => {
  return (<Table>
    <Table.Body>
      {/* This is the error right here, cap'n */}
      {assignments.map(assignment => <MyCustomComponentRow assignment={assignment} />)}
    </Table.Body>
  </Table>)
}

const MyCustomComponentRow = ({ assignment }) => {
  return (
    <Table.Row>
      <Table.Cell>{assignment.name}</Table.Cell>
      <Table.Cell>{assignment.grade}</Table.Cell>
    </Table.Row>
  )
}

Workaround for now

MyCustomComponentRow.displayName = "Table.Row"

Recommendation

Use TypeScript instead of prop-types

hi!

Thanks for the report, we will look into this. We know about this and similar issues, the proper solution would be to use an interface not a specific component type. This would require lots of refactoring (and likely introduce breaking changes), and there are always workarounds so we've never gotten to the point that we fix it. But it's on our roadmap, slowly rising in priority :)

Use TypeScript instead of prop-types

We will likely not get rid of prop-types in the near future, because it offers runtime type checking which is especially useful for JS codebases.

FWIW we use babel-plugin-typescript-to-proptypes, which enables runtime type checking in dev.

Hi, just wanted to pop in and +1 this. This bug has lead to some workarounds and hacks that have created a few maintenance headaches.

One is that Inst-ui validates React components of the desired type being passed as children, but not functions. This has lead to some functions inside of inst-ui components that cannot use hooks. The 'Workaround For Now' above that we found is a very genuinely a hack, it was only found while looking through the source code deep inside of inst-ui.

FWIW we use babel-plugin-typescript-to-proptypes, which enables runtime type checking in dev.

oh cool, I did not knew about this, we will investigate :)
We know this is an issue that people would like to see solved, and as I see there is a need for it from multiple teams. Sadly its not a simple fix (there are lots of places where such pattern is used), so it will need a bit of planning how and when we can do it. But we will treat it as a high priority issue and deal with it early next year

this is partially resolved in #1551 for Table and others are coming in the near future