maslianok / react-resize-detector

A Cross-Browser, Event-based, Element Resize Detection for React
http://maslianok.github.io/react-resize-detector/
MIT License
1.25k stars 91 forks source link

4.x has broken render props #63

Closed TobyEalden closed 5 years ago

TobyEalden commented 5 years ago

Version 4.0.1 has broken renderProps usage:

      return (
        <ReactResizeDetector
          handleHeight
          handleWidth
          render={({height, width}) => (
            <LineChart
              width={width}
              height={height - 30}
              data={data}
            >
           ...
maslianok commented 5 years ago

well, I just tested the next code snippet and it works as expected

<ResizeDetector
  handleHeight
  handleWidth
  render={({ height, width }) => <ChildWrapper height={height} width={width} />}
/>

and ChildWrapper is

class ChildWrapper extends Component {
  render() {
    return <div style={{ width: '100%' }}>{`Width: ${this.props.width}, Height: ${this.props.height}`}</div>;
  }
}

@TobyEalden do you see any errors in the console?

TobyEalden commented 5 years ago

No errors in the console. It's totally reproducible for me in that if rollback to 3.4.0 it works and with 4.0.1 it stops working. It does call the render function once after load (with width and height undefined), but never calls it again even after resize.

The other thing to note which may or may not be relevant is that I am using it to wrap a recharts component, and it looks like recharts itself has a dependency on 2.3.x. Although again, this doesn't affect 3.4.0.

I don't have any spare time right now but when I get the chance I can look into this further and perhaps create a simple reproduction, but in the meantime this is the render that is failing for me:

      return (
        <ReactResizeDetector
          handleHeight
          handleWidth
          render={({height, width}) => (
            <LineChart
              width={width}
              height={height - 30}
              data={data}
            >
              <XAxis
                dataKey="timestamp"
                domain={domain}
                scale="time"
                tickFormatter={tickFormatter}
                type="number"
              />
              <YAxis />
              <Legend />
              <Tooltip
                labelFormatter={tooltipFormatter}
                contentStyle={{backgroundColor: theme.palette.background.defaultEmphasize}}
              />
              {chartLines}
            </LineChart>
          )}
        />
      );
maslianok commented 5 years ago

@TobyEalden Can you try to replace LineChart with a simple component provided above (ChildWrapper).

In case it works we will know that the problem appears when we use ResizeDetector in conjunction with rechart.

TobyEalden commented 5 years ago

Good idea.

This is working as expected with 4.0.1:

      return (
        <ReactResizeDetector
          handleHeight
          handleWidth
          render={({height, width}) => (
            <div><p>width is {width}</p><p>height is {height}</p></div>
            // <LineChart
            //   width={width}
            //   height={height - 30}
            //   data={data}
            // >
            //   <XAxis
            //     dataKey="timestamp"
            //     domain={domain}
            //     scale="time"
            //     tickFormatter={tickFormatter}
            //     type="number"
            //   />
            //   <YAxis />
            //   <Legend />
            //   <Tooltip
            //     labelFormatter={tooltipFormatter}
            //     contentStyle={{backgroundColor: theme.palette.background.defaultEmphasize}}
            //   />
            //   {chartLines}
            // </LineChart>
          )}
        />
      );

So I wonder what is different about 4.0.1 that prevents using LineChart as a child?

maslianok commented 5 years ago

I don't know... Let's ask rechart's author to update to v4.0.

https://github.com/recharts/recharts/issues/1659

TobyEalden commented 5 years ago

I think the recharts thing is a red herring.

This also doesn't work in 4.0.1, but works as expected in 3.4.0

      return (
        <ReactResizeDetector
          handleHeight
          handleWidth
          render={({height, width}) => (
            <div style={{backgroundColor: "lime", width, height}} />
          )}
        />
      );
evanzombie commented 5 years ago

Any updates on this issue?

maslianok commented 5 years ago

@TobyEalden Should be fixed in v4.0.2.

Thanks for reporting this issue!

TobyEalden commented 5 years ago

This is still not working with the Recharts case, although it has fixed the basic example above.

I think the issue is with https://github.com/recharts/recharts/blob/master/src/chart/generateCategoricalChart.js#L1492.

Admittedly this is easy to work around, but I think it could be a fairly common edge-case, especially since the first time the render prop function is called the width and height props are undefined. This means that the observer isn't set up and so any subsequent resize doesn't call the handler.

Reproduction:

      const CustomComponent = ({width, height}) => {
        if (!width && !height) {
          return null;
        }
        return <div style={{backgroundColor: "lime", width, height}} />;
      };

      return (
        <ReactResizeDetector
          handleHeight
          handleWidth
          render={({width, height}) => (
            <CustomComponent width={width} height={height} />
          )}
        />
      );

The above works as expected in 3.4.0.

maslianok commented 5 years ago

Great observation!

You can also try skipOnMount prop as a temporary workaround.

I'll try to fix this issue in the upcoming days.

maslianok commented 5 years ago

@TobyEalden Well, I reviewed everything one more time and I'm not sure that this behavior should be fixed somehow.

It's true, that a child component renders with width&height equal to undefined during the initial render. That's because ResizeObserver attaches to an element after it renders in a DOM tree (inside componentDidMount).

Of course, we can postpone child component rendering until ResizeObserver is attached to the DOM tree. But I'm sure that there are some use-cases when you want to render child component ASAP and it's ok to render it with some default width&height. In your case, I suggest to use skipOnMount=true or to add some default width and height values, e.g.

const CustomComponent = ({ width = 200, height = 200 }) => {
        return <div style={{backgroundColor: "lime", width, height}} />;
      };
TobyEalden commented 5 years ago

Using skipOnMount doesn't help - if used in combination with default width/height it means the component will stay at that size until a manual resize occurs (e.g. 200x200 in the example above).

Adding a default width and height does work but that feels like a hack to me, and it does cause a 'flicker' on initial render, whereas it works out-of-the box with no defaults using v3.4.

Having said that, I'm happy to stick with 3.4 as it serves my needs - so feel free to close this and thanks for your help.

maslianok commented 5 years ago

We have a tradeoff here: speed VS default props. In your particular case speed doesn't matter because you can't render component without width/height. But, as I said earlier, this may be crucial for other users who want to render a component even without width/height. So, unfortunately, I can't set it as default behavior.


Anyway, I highly recommend you to update to v4.x because it has tons of other performance improvements.

Thanks for the discussion!

goldo commented 5 years ago

Same problem here: I need to set a width for drawing my charts. Since react-resize-detector doesn't go 100% by default with 4.x, I can't draw my chart in one time: I need to set a default size for my chart, it draws, then resize.. This use case was working with 3.X since it gave my a width in px that would match 100% of the available width.

Edit: my charts can't take "100%" in value, it must be set in pixels