[WIP] Use forwardRef on most components.

[seansfkelley]

• Bug fix
• New feature
• Chore
• Breaking change
• There is an open issue which this change addresses
• I have read the CONTRIBUTING document.
• My commits follow the Git Commit Guidelines
• My code follows the code style of this project.
• My change requires a change to the documentation.
• • I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

This PR is a work in progress and serves to demonstrate how this would be done.

Per React's official recommendation, this should be considered a breaking change and there is no recommended way to smooth the transition beyond that ("Conditionally applying React.forwardRef when it exists is also not recommended...")

Changes in This PR

• Bumped minimum React version to ^16.3.
• Bumped enzyme and enzyme-adapter-react-16 to get support for forwardRef.
• Converted many components to use forwardRef.
  • Some components that did not have innerRef before can now be ref'd.
  • Deprecated components were not converted.
  • See below for a more-detailed breakdown of unconverted components.
  • Removed all innerRef props.
• Removed some (but not all) crufty getRef props.

Open Questions

• Which components don't make sense to have refs?
• Should some of the class-style components be converted to functional components (such as Button)?
• What branch should this go to?

TODO

• Update docs.
• Fix existing tests.
• Add new tests.
• Convert select class components (i.e. those with existing innerRef).
  • Collapse
  • Modal
  • NavLink
  • TooltipPopoverWrapper

Converted Components of Note

Most components that were converted were either already functional components. Button, Form and Input were class components, but I chose to convert them into functional components.

• Button: The only reason this was a class component was to permit this-binding of onClick for performance reasons. I traded off simplicity and ergonomics of the ref prop for negligible performance cost.
• Form/Input: Both of these had what appeared to be crufty getRef props from an earlier implementation that did not appear to be used, along with wrappers around imperative methods (submit and focus respectively). Assuming that getRef is no longer desired, it is safe to convert to a forwarded ref component because the imperative methods will not be available directly on the ref'd DOM element.

Unconverted Components

Some components were not converted, namely:

• Deprecated components.
• Class components that could not be easily translated to functional components: ref forwarding gets pretty tedious for class components, and the components in question didn't have an innerRef or any other indication that they would benefit from forwarding. It would be easy to make this improvement in the future, but in the interest of keeping this PR focused on (1) functional components that would benefit the most from this change and (2) being mergeable in the near future, I opted to avoid them.

The pattern for ref-forwarded components looks like this:

class MyComponent extends React.Component {
  render() {
    const {innerRef ...attributes} = this.props;
    // do something with innerRef
  }
}

export default React.forwardRef((props, ref) =>
  <MyComponent {...props} innerRef={ref} />
);

Components that have not been converted for this reason are:

• Carousel
• CarouselItem
• Dropdown
• DropdownItem
• DropdownMenu
• DropdownToggle
• PopperContent
• PopperTargetHelper
• Portal
• TabContent
• TabPane
• UncontrolledAlert
• UncontrolledButtonDropdown
• UncontrolledCarousel
• UncontrolledCollapse
• UncontrolledDropdown
• UncontrolledPopover
• UncontrolledTooltip

Fixes #1082.

[TheSharpieOne]

Which components don't make sense to have refs?

Probably on the more complex components where users my want a ref to the component itself and not forwarded (or when the component's output is complex and there is no obvious child to forward the to). Maybe Modal, Dropdown, and Carousel,

Should some of the class-style components be converted to functional components (such as Button)?

Button, for example, has some functions which it binds to cache them. If it was converted to a function component the onClick would be create every render. I know Dan Abramov has some good reads on the topic and mostly comes down to perf testing, but since this is a library which can be used and many ways, I am not really sure how to provide the best perf to the end users.
You can create a wrapper which uses React.forwardRef to allow ref but then forwards it to innerRef or some prop on the class-based component. This allows for the class-based to remain class-based but it's usage to allow for ref.

What branch should this go to?

This seems like a feature that is wanted by many, go ahead and PR to master, I'll make another major release.

[seansfkelley]

Thanks for the quick response! I'll work through the TODOs and incorporate your suggestions, then put it up for proper review.

[TheSharpieOne]

@seansfkelley I'm eagerly watching, keep up the good work! Let me know when it's ready.

[seansfkelley]

Thanks for the encouragement. :) There's been a lot of legwork to do to add it everywhere, though it's mostly working now.

I'm struggling a bit with getting the tests to pass -- Enzyme will often inspect/return the forwardRef wrapper rather than the thing it rendered, so there are ~50 test failures at the moment that need to learn how to deal with that. I'm not sure at the moment what that means, so if you've got any knowledge to share on that, I'd love to have it!

[seansfkelley]

Unfortunately I don't have the bandwidth to work on this for the foreseeable future, so I'm going to close it out to reduce noise.