Skip to content

Graph not changing on input change #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DennisOosterling opened this issue Mar 7, 2018 · 33 comments
Closed

Graph not changing on input change #59

DennisOosterling opened this issue Mar 7, 2018 · 33 comments
Assignees

Comments

@DennisOosterling
Copy link

DennisOosterling commented Mar 7, 2018

According to the docs: This component currently creates a new plot every time the input changes.
It seems like the graph would do a redraw once the data changed. When passing updated props to the <Plot/> data attribute the plot stays the same.

Even when checking in componentWillReceiveProps() the nextProps.data is different than the this.props.data.

Update:
The same seems true when incrementing the revision attribute.

@nicolaskruchten
Copy link
Contributor

I'm just in the middle of updating the documentation and pulling together a release of this component which clears up this behaviour :)

@DennisOosterling
Copy link
Author

Awesome @nicolaskruchten any ETA & solution for the in the meantime?

@nicolaskruchten
Copy link
Contributor

Literally 10 minutes and I'll have the new version on NPM :)

@nicolaskruchten
Copy link
Contributor

OK, please try using version 1.7.0 and let me know if you're still having issues. If so, please provide some sample code that shows what is happening and explain what you would expect to have happen :)

@nicolaskruchten
Copy link
Contributor

(sorry, hit the wrong button)

@nicolaskruchten
Copy link
Contributor

Also, please check out the new notes in the props section: https://github.com/plotly/react-plotly.js#props

@DennisOosterling
Copy link
Author

Sadly it still doesn't work, will provide some code samples tomorrow.

@nicolaskruchten
Copy link
Contributor

OK. Are you adding data points to a data array or something like that? If you want to mutate the input you'll have to bump the revision number.

@DennisOosterling
Copy link
Author

Yes and yes haha. I am indeed adding datapoints to an array, but creating a new array in the process. Also I bump the revision number. the onUpdate callback is called btw.

@nicolaskruchten
Copy link
Contributor

So the onUpdate callback is being called but your chart isn't visually changing?

@DennisOosterling
Copy link
Author

exactly

@nicolaskruchten
Copy link
Contributor

Very strange. I'd love to see some sample code and I can help you debug :)

@DennisOosterling
Copy link
Author

class Monitoring extends React.Component {
  state = {
    realtimeData: [
      {
        type: "scatter",
        mode: "lines+points",
        x: [1, 2, 3],
        y: [2, 6, 3],
        marker: { color: "#fdcc00" },
        line: { shape: "spline" }
      }
    ]
  };

  componentDidMount() {
    // Simulate realtime data

    setInterval(() => {
      let newRealtimeData = [...this.state.realtimeData];
      newRealtimeData[0].x.push(newRealtimeData[0].x.length + 1);
      newRealtimeData[0].y.push(Math.floor(Math.random() * 6) + 1);

      this.setState({
        realtimeData: newRealtimeData
      });
    }, 2000);
  }

render() {
    return (
      <div>
          <MonitorChart
            realtimeData={this.state.realtimeData}
          />
      </div>
  }
}
const MonitorChart = props => {
  const { realtimeData } = props;

  return (
    <div>
      <Panel
        header={<PanelHeader number={1} stageName={"Chart"} />}
        initialOpen={true}
      >
        <Chart data={realtimeData} />
      </Panel>
    </div>
  );
};
class Chart extends React.Component {
  state = {
    revisionNo: 1
  };

  componentWillReceiveProps(nextProps) {
    this.setState((prevState, props) => {
      return { revisionNo: prevState.revisionNo + 1 };
    });
  }

  updateHandler = () => {
    console.log("Updated");
  };

  render() {
    return (
      <div style={{ marginTop: "10px" }}>
        <Plot
          divId={"chartmonitoring"}
          revision={this.state.revisionNo}
          style={{
            width: "90%",
            height: "100%"
          }}
          useResizeHandler={true}
          data={this.props.data}
          onUpdate={this.updateHandler}
        />
      </div>
    );
  }
}

I'm using webpack with the ify-loader. No error messages and onUpdate callback is called.

@nicolaskruchten
Copy link
Contributor

Thanks for this, I'll try to debug it this morning :)

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Mar 8, 2018

OK, here's a stripped-down CodePen version of your code: https://codepen.io/nicolaskruchten/pen/XEWmGY?editors=1010

The issue is that although you're creating a copy of the data array with let newRealtimeData = [...this.state.realtimeData];, this is a shallow copy: newRealtimeData[0].x === this.state.realtimeData[0].x is still true (check the console when running that CodePen).

You have two options moving forward:

  1. you can either have your realtime system copy-and-extend your x and y arrays, and then plotly.js will see that your data arrays have changed and will rerender
  2. you can 'cheat' and pass in the new layout.datarevision parameter to force updates even if the identity of your data arrays hasn't changed. In my CodePen just uncomment line 18 and it works

@jaredrcleghorn
Copy link

jaredrcleghorn commented Nov 8, 2018

@nicolaskruchten I'm experiencing a similar issue. I have a chart that is not being refreshed even when both the revision prop and layout.datarevision are bumped. After looking at the componentWillUpdate method of PlotlyComponent located in factory.js, it is obvious why this is the case: even if the revision prop is changed, the method still returns without doing anything if the identities of both the data and layout props have not changed. I am not sure whether or not this was the functionality desired by whoever implemented the method, but it does not seem to be the functionality that is documented in the README The documentation states that

This component will refresh the plot via Plotly.react if any of the following are true:

  • The revision prop is defined and has changed, OR;
  • One of data, layout or config has changed identity as checked via a shallow ===, OR;
  • The number of elements in frames has changed

but based on the code, it seems that even if the revision prop is defined and has changed, at least one of the second and third conditions still has to be true in order for the plot to be refreshed, meaning that only bumping the revision prop is not sufficient. I am thinking that this is probably not the intended behavior since it pretty much defeats (what seems to me to be) the whole purpose of the revision prop, which is to make it possible to force the plot to be refreshed even if the identities of the data and layout props haven't changed so that you can do things like update the data in-place. Is this the current functionality, and if so, is it what was intended?

@nicolaskruchten
Copy link
Contributor

@antoinerg can you take a look at this please?

@ghost
Copy link

ghost commented Dec 25, 2018

Hey, everyone! Take a look in this solution with explanations in how to create a graph using React and Plot.ly with the life cycle of the React.

https://medium.com/@jmmccota/plotly-react-and-dynamic-data-d40c7292dbfb

@GrandVizierOlaf
Copy link

I've run into the same issue and inside of the componentWillUpdate code the values of nextProps and this.props is always the same, making the equality check always true and it does not rerender.

I tried changing the method to shouldComponentUpdate, and the references of "this" to "_this3" like in the other functions, but could not fix it.

Ultimately I wrapped the component in a div and used the chart's datarevision as that div's key. It causes the entire div and plot to be recreated from scratch (rather than update), but it works.

@nicolaskruchten
Copy link
Contributor

@dmt0 can you grab this one too plz?

@dmt0
Copy link
Contributor

dmt0 commented Jan 12, 2019

OK, so our readme says:

This component will refresh the plot via Plotly.react if any of the following are true:

  • The revision prop is defined and has changed, OR;
  • One of data, layout or config has changed identity as checked via a shallow ===

Our code is:

if (
  nextProps.revision !== void 0 &&
  nextProps.revision === this.props.revision
) {
  // if revision is set and unchanged, do nothing
  return;
}
if (
  nextProps.layout === this.props.layout &&
  nextProps.data === this.props.data &&
  nextProps.config === this.props.config &&
  numNextFrames === numPrevFrames
) {
  // prevent infinite loops when component is re-rendered after onUpdate
  // frames *always* changes identity so fall back to check length only :(
  return;
}

So clearly as per the code we have to change revision AND identity of data or layout.

@jaredrcleghorn is that what you're referring to?

@jaredrcleghorn
Copy link

@dmt0 Yes that is what I was referring to. If I know what the desired functionality was, I would be happy to fix either the code or the README and submit a pull request myself, but the whole problem is that I don't know which one is wrong.

@nicolaskruchten
Copy link
Contributor

@dmt0 this seems like I made a coding or refactoring error... I'm pretty sure the readme describes the desired behaviour.

@dmt0
Copy link
Contributor

dmt0 commented Jan 14, 2019

@nicolaskruchten Yeah, I've been looking at blame, and it seems that you were fixing the infinite loop issue, let's chat :)

@nicolaskruchten
Copy link
Contributor

Yeah. You know, the revision prop is a holdover from a time when plotly.js didn't have datarevision and uirevision etc. Maybe we can just get rid of it and bump the major version number.

@jaredrcleghorn what do you think? Is revision useful for you?

@dmt0
Copy link
Contributor

dmt0 commented Jan 14, 2019

If we bump the major version, I would also like to replace componentWillUpdate with componentDidUpdate to make it compatible with React 17.

@dmt0
Copy link
Contributor

dmt0 commented Jan 14, 2019

BTW, tested @jaredrcleghorn's PR against our RCE, things work.

@GrandVizierOlaf
Copy link

I don't claim to be an expert, but wouldn't shouldComponentUpdate be preferable to componentDidUpdate? That way you can use this logic to prevent the update.

@dmt0
Copy link
Contributor

dmt0 commented Jan 14, 2019

@GrandVizierOlaf
https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate

Typically, this method can be replaced by componentDidUpdate(). If you were reading from the DOM in this method (e.g. to save a scroll position), you can move that logic to getSnapshotBeforeUpdate().

Tried it out, in our setup, things work.

@jaredrcleghorn
Copy link

@nicolaskruchten It would have been useful to me in the situation I was in when I originally posted on this issue. I got around the problem by basically changing the identity of one of the other props for the sole purpose of forcing a refresh, so it would have been much cleaner if I had been able to use revision or some other method that allowed you to force a refresh in a non-hacky way, and there may be another way to do it that I am simply not aware of (you mentioned datarevision and uirevision, which I am not really familiar with).

@nicolaskruchten
Copy link
Contributor

I would also like to replace componentWillUpdate with componentDidUpdate

we can do this anyway, without bumping the major version number IMO. It might be nicer to use shouldComponentUpdate but this is the smaller change.

dmt0 pushed a commit that referenced this issue Feb 21, 2019
Previously, chaning the revision prop alone was not sufficient to cause
the plot to be refreshed, contrary to the behavior indicated in the
REAMDE. Fix this so that the revision prop can be used to force the
plot to be refreshed.

Resolves: #59
dmt0 pushed a commit that referenced this issue Feb 21, 2019
Previously, chaning the revision prop alone was not sufficient to cause
the plot to be refreshed, contrary to the behavior indicated in the
REAMDE. Fix this so that the revision prop can be used to force the
plot to be refreshed.

Resolves: #59
@dmt0 dmt0 self-assigned this Feb 21, 2019
@dmt0
Copy link
Contributor

dmt0 commented Feb 21, 2019

Fix is merged and will be available after the next version bump :)

@dmt0
Copy link
Contributor

dmt0 commented Feb 25, 2019

Fixed released in v2.3.0. Gonna close this issue. Feel free to let us know if problems remain.

@dmt0 dmt0 closed this as completed Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants