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.

Sign up for GitHub

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

Jump to bottom

fix(line): Ensure null check to avoid Cannot set properties of null (setting 'hoverState') error #20563

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(line): Ensure null check to avoid Cannot set properties of null (setting 'hoverState') error #20563

wants to merge 2 commits into from
+27 −1

Conversation

svonderohe
Copy link

@svonderohe svonderohe commented Dec 5, 2024 •
edited
Loading

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

This PR fixes the following error:

Uncaught TypeError: Cannot set properties of null (setting 'hoverState')
    at setStatesFlag (states.js:117:1)
    at __webpack_modules__../node_modules/echarts/lib/chart/line/LineView.js.LineView._changePolyState (LineView.js:763:1)
    at Symbol.changePolyState [as onHoverStateChange] (LineView.js:667:1)
    at doChangeHoverState (states.js:72:1)
    at singleLeaveBlur (states.js:93:1)
    at states.js:311:1
    at __webpack_modules__../node_modules/zrender/lib/graphic/Group.js.Group.traverse (Group.js:131:1)
    at __webpack_modules__../node_modules/zrender/lib/graphic/Group.js.Group.traverse (Group.js:133:1)
    at states.js:310:1
    at Global.js:511:1

By removing a redundant call to setStatesFlag missing a null/undefined check

Fixed issues

I haven't created a separate issue, but I can if you would like

Details

Before: What was the problem?

There can be a race condition between the tooltip and instance.dispatchAction({type: 'downplay'}) that causes the chart to freeze with an exception

image

After: How does it behave after the fixing?

There is no longer an error

Document Info

One of the following should be checked.

  • This PR doesn't relate to document changes
  • The document should be updated later
  • The document changes have been made in apache/echarts-doc#xxx

Misc

ZRender Changes

  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

N.A.

Others

Merging options

  • Please squash the commits into a single one when merging.

Other information

@echarts-bot ECharts Bot
Copy link

echarts-bot bot commented Dec 5, 2024

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

svonderohe
svonderohe commented Dec 5, 2024
View reviewed changes
src/chart/line/LineView.ts
setStatesFlag(this._polyline, toState);
polygon && setStatesFlag(polygon, toState);
Copy link
Author

@svonderohe svonderohe Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the first call to setStatesFlag is not intentional. Removing the call fixes the error I was encountering since the second call has a null check

@svonderohe svonderohe changed the title Fix "Cannot set properties of null (setting 'hoverState')" fix(line): Ensure null check to avoid "Cannot set properties of null (setting 'hoverState')" error Dec 5, 2024
@svonderohe svonderohe changed the title fix(line): Ensure null check to avoid "Cannot set properties of null (setting 'hoverState')" error fix(line): Ensure null check to avoid Cannot set properties of null (setting 'hoverState') error Dec 5, 2024
@plainheart
Copy link
Member

Thanks for your contribution. Can you add a test case for this fix?

svonderohe
svonderohe commented Dec 6, 2024
View reviewed changes
test/ut/spec/chart/line/LineView.test.ts
Comment on lines +21 to +24
it('should not error on state change when polygon is not set', function () {
lineView._polygon = null;
expect(() => lineView._changePolyState('emphasis')).not.toThrow();
});
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case would fail before the change in this PR

svonderohe
svonderohe commented Dec 6, 2024
View reviewed changes
test/ut/spec/chart/line/LineView.test.ts
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is the right place to put the tests, if it needs to move please let me know

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better if you can provide a reproduction demo. Please refer to
https://github.com/apache/echarts/wiki/How-to-make-a-pull-request#add-a-test-case

@svonderohe
Copy link
Author

Thanks @plainheart, I have added a couple test cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plainheart plainheart plainheart left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
PR: awaiting review PR: doc unchanged PR: first-time contributor size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@svonderohe @plainheart