Render prop implementation leads to potential issues with react hooks

mattcosta7 commented 3 years ago

Describe the bug When a renderProps needs to internally use a hook, the fact that renderProps are called as functions like renderItem() instead of become 'rendered' as components may lead to subtle bugs based on where those hooks are defined.

Internally, react scopes hooks to the component they're rendered in rather than to the callable they're called in.

This leads to

const Component = ({ renderItem }) => {
   return renderItem()
}

function App() {
<Component renderItem={() => {
      useHook()
      return <div />
}} />
}

render(<App />)

associating the useHook hook as being read on the app component, and not on the renderItem as might be anticipated by a user

const Component = ({ renderItem: ItemRenderer }) => {
   return <ItemRenderer />
}

function App() {
<Component renderItem={() => {
      useHook()
      return <div />
}} />
}

render(<App />)

However, registers the hook on the renderItem component instead, which properly scopes accesses to things like context where they belong.

This stems from hooks being registered through calls to React.createElement or jsx rather than when they are used in a called function (ie: `React.createElement(RenderItem, props) or jsx(RenderItem, props)vsrenderItem()`

This does still create a small issue where users can pass renderProps that are generated on every render, throwing out react's ability to diff them, but that seems acceptable (and not different than the current behavior). We could potentially warn users in dev by checking if references to it change between renders and warning that they might prefer a useCallback declaration there, or a useMemo one that returns a lazy/memo/forwardRef, or to move the delcaration outside of the component and call it elsewhere instead

This should give consumers more latitude in passing various react component types to renderItem, as well as a plain functional component definition, without potential hook related issues from hooks being registered to the wrong component

Note: As this is a public repo, please be careful not to include details or screenshots from unreleased GitHub products or features. In most cases, a good bug report should be able to describe the new component without including business logic or feature details, but if you must discuss context relating to an unreleased feature, please open an issue in the private Design Systems repo and link to it here.

colebemis commented 3 years ago

Thanks for opening this issue, @mattcosta7! I've added it to our project board 👍

siddharthkp commented 2 years ago

Hi @mattcosta7 We are generally moving away from render prop APIs like the one you mentioned. Do you remember the component/example you were working with that sparked this issue?

siddharthkp commented 2 years ago

primer / react

Render prop implementation leads to potential issues with react hooks #1428