-
-
Notifications
You must be signed in to change notification settings - Fork 137
Error while plotting after React update #52
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
Comments
Thanks for the bug report! I'll try to replicate and add some guards here so that we don't try to render on a |
Is this still an issue with version 2.0.0? I haven't been able to replicate it... |
disclaimer1: I'm somewhat of a react newbie dsiclaimer2: This solution appeared to be incomplete for our project: it lead to some new problems. Last thursday, I got a project to repair/improve which had exactly this bug: I solved (=circumvented) this as follows: the returned component (parent) in my
Versions:
|
@paulboocock thanks for the confirmation! Would it be possible for you to show me a minimal example that reproduces this, possibly a CodePen or something? I can't reproduce it on my end and I'm not clear on what you mean by "If I conditionally render the Plot, say I remove it from the DOM whilst the API is being called to show a loading screen" ... How are you removing from the DOM, and which API are you referring to? |
This seems to happen when you update the props before the render is ready. For example, in our app we calculate the dimensions once the page has been loaded, but this sometimes causes the Plot to be rendered twice: once with the incorrect dimensions, which fails with the mentioned error, and once with the correct dimensions. Since the second render succeeds, the UI looks correct even though an error is logged in the console. |
I too am running into this issue. Has anybody figured out a good way to catch it? |
Hello there, Versions: |
Bump |
@antoinerg can you take a look at this please? |
Edit: After testing, I realized this only works on the initial render. Subsequent renders, when the LoadingSpinner component is rendered in place of the PlotlyChart component, the error will reappear.
import React, {Component} from 'react';
import PlotlyChart from './PlotlyChart';
import LoadingSpinner from './LoadingSpinner';
export class Dashboard extends Component {
/* ... */
render() {
/* ... */
return (
<div>
{this.props.loading ? (
<LoadingSpinner />
) : (
<PlotlyChart divId="dashboardChart" />
)}
</div>
);
}
}
import React, {Component, Suspense} from 'react';
import LoadingSpinner from './LoadingSpinner';
const PlotlyChart = React.lazy(() => import('./PlotlyChart'));
export class Dashboard extends Component {
/* ... */
render() {
/* ... */
return (
<div>
{this.props.loading ? (
<LoadingSpinner />
) : (
<Suspense fallback={<div />}>
<PlotlyChart divId="dashboardChart" />
</Suspense>
)}
</div>
);
}
}
|
Would be great to see an integrated solution |
@dmt0 can you grab this one please? |
This could probably be fixed easily. We can add a check here: https://github.com/plotly/react-plotly.js/blob/master/src/factory.js#L116 to see whether |
Same problem here. |
@dmt0 Here's a minimal example that reproduces the error: The error will occur on initial render, and everytime the # of days is changed. |
https://reactjs.org/docs/refs-and-the-dom.html#callback-refs
It looks like someone didn't properly account for when unmounting will set the ref to |
Bump! |
If anyone wants to contribute a fix, we'd be happy to accept a pull request :) |
@brammitch Unfortunately this doesn't seem to solve this issue for me :( |
@brammitch Thanks for the quick response! Is that the difference or am I missing something else? |
I haven't had time to write a proper PR and test case around it, but I'm pretty certain this is due to the async happening on this line: https://github.com/plotly/react-plotly.js/blob/master/src/factory.js#L115 In all other circumstances React will guarantee that the ref is handled (and so Something like this: componentWillUnmount() {
this.unmounting = true;
// (existing code…)
} and this: this.p = this.p
.then(() => {
if (!this.el) {
if (this.unmounting) {
const error = new Error('Cannot plot while component is unmounting');
error.reason = 'unmounting';
throw error;
} else {
throw new Error('Cannot plot due to missing element reference');
}
}
return Plotly.react(this.el, {
data: nextProps.data,
layout: nextProps.layout,
config: nextProps.config,
frames: nextProps.frames,
});
})
.then(() => this.syncEventHandlers(nextProps))
.then(() => this.syncWindowResize(nextProps))
.then(() => this.figureCallback(nextProps.onUpdate))
.catch(err => {
if (err.reason === 'unmounting') {
// Cannot plot while unmounting
return;
}
console.error('Error while plotting:', err);
this.props.onError && this.props.onError(err);
}); should (in theory) handle and prevent the code from attempting to call |
Actually, now that I look over it, I thought the one inside |
Looking at @brammitch's first code example, the error comes out of both Regarding @vdh solution above, I'm worried that if we use a flag like |
@dmt0 What exactly are the "some things" you're worried about needing to be dereferenced? I would assume the existing code in The only thing the Is it important for the |
Yep, all looks good, made a PR based on your solution, thanks @vdh :) |
When will this merge be published to npm? 😬 |
Fixed released in v2.3.0. Gonna close this issue. Feel free to let us know if problems remain. |
Hi, I am still getting this errors in some circumstances. I tried to debug my situation and come to finding that library is not checking if a component is mounted in promise handlers in Short story What happened in my situation in details (I've put some checkpoints in comments in bellow snippet): updatePlotly(shouldInvokeResizeHandler, figureCallbackFunction, shouldAttachUpdateEvents) {
// updatePlotly: checkpoint 1
this.p = this.p
.then(() => {
if (!this.el) {
let error;
if (this.unmounting) {
error = new Error('Component is unmounting');
error.reason = 'unmounting';
} else {
error = new Error('Missing element reference');
}
throw error;
}
// updatePlotly: checkpoint 2
return Plotly.react(this.el, {
data: this.props.data,
layout: this.props.layout,
config: this.props.config,
frames: this.props.frames,
});
})
.then(() => this.syncWindowResize(shouldInvokeResizeHandler) // updatePlotly: checkpoint 3)
.then(this.syncEventHandlers)
.then(() => this.figureCallback(figureCallbackFunction))
.then(shouldAttachUpdateEvents ? this.attachUpdateEvents : () => {})
.catch(err => {
if (err.reason === 'unmounting') {
return;
}
console.error('Error while plotting:', err); // eslint-disable-line no-console
if (this.props.onError) {
this.props.onError(err);
}
});
Possible solution // ...
.then(() => {
if(!this.unmounting) {
return this.syncWindowResize(shouldInvokeResizeHandler))
}
}
// ... Alternative solution |
@dmt0 Can you look at my comment above? Sholud I open new issue? |
I opened a new issue #199. |
Under certain circumstances, after a React update, the
<Plot ../>
component will throw the following error:I added some logging into
factory.js
to try and debug (modifiedfactory.js
attached below). Here's the output:My hypothesis is that if Reacts attempts to update the component quickly enough, the promise scheduled in
willComponentUpdate
(https://github.com/plotly/react-plotly.js/blob/master/src/factory.js#L100) may be called after React callsgetRef
withnull
and before it gets called again with the newly rendered div.factory.js.txt
The text was updated successfully, but these errors were encountered: