Skip to content
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

Error re-rendering Graph when no `config` prop is set #81

Closed
LonelyPrincess opened this issue Jun 28, 2018 · 4 comments
Labels
bug

Comments

@LonelyPrincess
Copy link
Collaborator

@LonelyPrincess LonelyPrincess commented Jun 28, 2018

I was stuck with this error for a while, until I finally found out what was causing it. Details on the error and the current workaround are listed bellow, just in case it can be of use to someone else who's struggling with the same problem.

Problem description

By the time I got the error, my Graph component was looking like this:

<Graph
    id="network-graph"
    data={this.props.data}
    onClickNode={this.props.onNodeClick || this.onNodeClick}
    onClickLink={this.props.onEdgeClick || this.onEdgeClick} />

When I run the application for the first time, the graph would render perfectly on the screen. However, when I updated its data in my component state and the graph was to be rendered for a second time, I got the following error message:

Uncaught TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Object.isDeepEqual (utils.js:52)
    at Graph.componentWillReceiveProps (Graph.js:390)
    at callComponentWillReceiveProps (react-dom.development.js:11527)
    at updateClassInstance (react-dom.development.js:11719)
    at updateClassComponent (react-dom.development.js:13153)
    at beginWork (react-dom.development.js:13824)
    at performUnitOfWork (react-dom.development.js:15863)
    at workLoop (react-dom.development.js:15902)
    at HTMLUnknownElement.callCallback (react-dom.development.js:100)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:138)
    at invokeGuardedCallback (react-dom.development.js:187)
    at replayUnitOfWork (react-dom.development.js:15310)
    at renderRoot (react-dom.development.js:15962)
    at performWorkOnRoot (react-dom.development.js:16560)
    at performWork (react-dom.development.js:16482)
    at performSyncWork (react-dom.development.js:16454)
    at interactiveUpdates$1 (react-dom.development.js:16719)
    at interactiveUpdates (react-dom.development.js:2150)
    at dispatchInteractiveEvent (react-dom.development.js:4532)

Workaround

After hours trying to figure it out, I finally discovered that adding the config prop to the Graph component prevented the error from happening.

<Graph
    id="network-graph"
    data={this.props.data}
    config={{}}
    onClickNode={this.props.onNodeClick || this.onNodeClick}
    onClickLink={this.props.onEdgeClick || this.onEdgeClick} />

Since I wasn't overriding anything in the default config, I just hadn't considered adding it before. In order to solve this issue, I was forced to pass down an empty object in the config prop.

After doing so, the graph would always successfully render whenever there was a change in the data.

Steps to reproduce the issue:

This issue can be easily reproduced in the sandbox example bundled with this project.

  1. In the Sandbox.jsx file, remove / comment the config property in the constant graphProps defined in the render method (currently, line 295).
const graphProps = {
    id: 'graph',
    data,
    //config: this.state.config,
    onClickNode: this.onClickNode,
    onClickLink: this.onClickLink,
    onMouseOverNode: this.onMouseOverNode,
    onMouseOutNode: this.onMouseOutNode,
    onMouseOverLink: this.onMouseOverLink,
    onMouseOutLink: this.onMouseOutLink
};
  1. Open the updated sandbox example in a browser.

  2. Click any of the buttons to dynamically add or remove a node (the + and - buttons above of the graph). This action will trigger the error immediatly.

Additional notes

The project README file mentions that only id and data are required, and that both config and any of the callback methods are entirely optional. This is true for the first time the graph is rendered, but right now it seems that the config object must be present to update the view later on.

The solution for this issue would be to initialize the config value in the component state to an empty object, which will be used by default in case no config prop has been passed down to the Graph component.

The componentWillReceiveProps handler seems to be a good place to start. If you'd like, @danielcaldas, I can take a closer look at this later and submit a pull request.

For the time being, users must explicitly set a config prop even if they're not overriding any of the default settings.

@danielcaldas

This comment has been minimized.

Copy link
Owner

@danielcaldas danielcaldas commented Jun 28, 2018

Hey there @LonelyPrincess, nice report, you identified with good precision the issue. Regarding the solution for this, componentWillReceiveProps is the proper place to handle this we need to check if the client provided a config (in nextProps.config) and simply fallback to an empty object if that isn't the case.

Fell free to submit a PR for this, nice job!

Thanks!

@danielcaldas danielcaldas added the bug label Jun 28, 2018
LonelyPrincess added a commit to LonelyPrincess/react-d3-graph that referenced this issue Jun 28, 2018
When the user didn't add the 'config' prop to the 'Graph' component (which should be possible, as it's an optional field), the graph failed to render whenever its data changed.

The component would work the first time around, but when it received new data, it threw an error as described [in this issue](danielcaldas#81) and the view would not update.

The root of the problem was located in the `componentWillReceiveProps` method of the `Graph` component. When defining the constants `configUpdated` and `transform`, the case in which `newProps.config` was set to either null or undefined was not being considered. This caused errors when calling `isDeepEqual` and also when trying to get the `panAndZoom` property during `transform` initialization.
LonelyPrincess added a commit to LonelyPrincess/react-d3-graph that referenced this issue Jun 28, 2018
When the user didn't add the 'config' prop to the 'Graph' component (which should be possible, as it's an optional field), the graph failed to render whenever its data changed.

The component would work the first time around, but when it received new data, it threw an error as described [in this issue](danielcaldas#81) and the view would not update.

The root of the problem was located in the `componentWillReceiveProps` method of the `Graph` component. When defining the constants `configUpdated` and `transform`, the case in which `newProps.config` was set to either null or undefined was not being considered. This caused errors when calling `isDeepEqual` and also when trying to get the `panAndZoom` property during `transform` initialization.
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

@LonelyPrincess LonelyPrincess commented Jun 28, 2018

No prob! I'm glad to be of assistance! 😊

I've just uploaded two commits addressing this same issue in my fork repo:

  • The first one is pretty much what you described. If the received props don't include config, we work with an empty object instead. This approach keeps most of the code exactly as you had it before.

  • The second approach is slightly different, but it should be more efficient since we will avoid some variables and operations which serve no purpose if the config property is not set and we want to stick to the defaults.

Could you tell me which of them you like better, @danielcaldas? I'll submit the PR for the solution you choose. 👍

@danielcaldas

This comment has been minimized.

Copy link
Owner

@danielcaldas danielcaldas commented Jul 1, 2018

I prefer the first one, it is cleaner than the second one, the performance gain with the second option is not substantial since we check for empty object with isObjectEmpty before proceeding with the more expensive operations.

Nice one 👍

danielcaldas added a commit that referenced this issue Jul 2, 2018
When the user didn't add the 'config' prop to the 'Graph' component (which should be possible, as it's an optional field), the graph failed to render whenever its data changed.

The component would work the first time around, but when it received new data, it threw an error as described [in this issue](#81) and the view would not update.

The root of the problem was located in the `componentWillReceiveProps` method of the `Graph` component. When defining the constants `configUpdated` and `transform`, the case in which `newProps.config` was set to either null or undefined was not being considered. This caused errors when calling `isDeepEqual` and also when trying to get the `panAndZoom` property during `transform` initialization.
@danielcaldas

This comment has been minimized.

Copy link
Owner

@danielcaldas danielcaldas commented Jul 2, 2018

Fixed with #84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.