ankane / react-chartkick

Create beautiful JavaScript charts with one line of React
https://chartkick.com/react
MIT License
1.2k stars 58 forks source link

Create forward ref to make Chartkick API accessible #42

Closed btorresgil closed 5 years ago

btorresgil commented 5 years ago

This PR does 2 things:

ankane commented 5 years ago

Hey @btorresgil, thanks for the PR đź‘Ť Can you explain 1 a bit more and give a code sample for what it'll allow developers to do? I'm not super familiar with forward refs, but have read this.

btorresgil commented 5 years ago

Hey @arkane, thanks for the quick reply. Sure, happy to help. Here's an example where a ref is useful:

import React from 'react'
import { LineChart } from 'react-chartkick'

class App extends React.Component {
  constructor(props) {
    super(props)
    this.chartRef = React.createRef()
  }

  getChartYRange() {
    const yScale = this.chartRef.current.chart.getChartObject().scales['y-axis-0']
    // Do something with yScale
  }

  render() {
    return (
      <LineChart data={['some-data-here']} ref={this.chartRef} />
    )
  }
}

In this example, the developer wants to access the yScale of the inner Chart.js, perhaps to synchronize another chart's scale, or to output min/max values in another component.

React-chartkick uses a higher order function (createComponent) to dynamically create the ChartComponent, which means a ref on the LineChart component refers to the outer createComponent, not the ChartComponent with the chart in it. The forward ref simply bypasses the createComponent and makes a ref refer to the ChartComponent instead.

More information here: https://reactjs.org/docs/higher-order-components.html#refs-arent-passed-through

Another option instead of forward ref is to get rid of the createComponent function.

btorresgil commented 5 years ago

In fact, since forward ref was added in React 16.3, getting rid of the HOC might be the better solution for compatibility. Could maybe use a render prop or innerRef instead?

btorresgil commented 5 years ago

ok, after considering this, I changed the forward ref to an innerRef for better compatibility. Didn't realize forward ref required React 16.3.

So now, the line above with the LineChart component would look like this:

<LineChart data={['some-data-here']} innerRef={this.chartRef} />

Honestly, this is still a hack around the higher order function, but it's a quick fix without changing much code, and it should work in any older versions of React (though I haven't tested this myself).

ankane commented 5 years ago

Thanks @btorresgil, think innerRef is a good approach. Tested in React 15 too and it works well.

ankane commented 5 years ago

And as a reminder to myself if forwardRef comes up later:

Note for component library maintainers

When you start using forwardRef in a component library, you should treat it as a breaking change and release a new major version of your library. This is because your library likely has an observably different behavior (such as what refs get assigned to, and what types are exported), and this can break apps and other libraries that depend on the old behavior.

Conditionally applying React.forwardRef when it exists is also not recommended for the same reasons: it changes how your library behaves and can break your users’ apps when they upgrade React itself.

https://reactjs.org/docs/forwarding-refs.html#note-for-component-library-maintainers