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

Fix cleaning up metasets #9656

Merged
merged 4 commits into from Oct 23, 2021
Merged

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Sep 16, 2021

I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced.

As of #7030, this._metasets should always be defined, so checking whether it's undefined is no longer necessary.

This is a minimal fix for #9653; as discussed there, it may also be worth updating updateHoverStyle.

I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced.

This is a minimal fix for chartjs#9653; as discussed there, it may also be worth updating `updateHoverStyle`.

As of chartjs#7030, `this._metasets` should always be defined, so checking whether it's undefined is no longer necessary.
@kurkle
Copy link
Member

kurkle commented Sep 17, 2021

Very well described issue and PR 👍

This makes sense. A regression test would be good, have you already tried creating one?

@kurkle kurkle added this to the Version 3.6.0 milestone Sep 17, 2021
@etimberg etimberg linked an issue Sep 17, 2021 that may be closed by this pull request
etimberg
etimberg previously approved these changes Sep 17, 2021
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Change looks good. I agree with adding a test

@etimberg
Copy link
Member

@joshkel have you had a chance to look at adding a test?

@joshkel
Copy link
Contributor Author

joshkel commented Oct 13, 2021

I apologize for the extreme delay in following up on this.

I looked briefly at adding some tests. The existing metasets tests seem to exercise _destroyDatasetMeta pretty well. I did add a test to verify that Chart.destroy in particular cleans up metasets; I'm not sure if that's helpful for not.

I can try and add a test covering the specific scenario from #9653 tomorrow.

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I had never used toHaveSize, so looked it up.

It'd be good to have a test for the specific issue.

});
it('cleans up metasets when the chart is destroyed', function() {
this.chart.destroy();
expect(this.chart._metasets.length).toHaveSize(0);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(this.chart._metasets.length).toHaveSize(0);
expect(this.chart._metasets).toHaveSize(0);

toHaveSize(expected)
expect the actual size to be equal to the expected, using array-like length or object keys size.

array = [1,2];
expect(array).toHaveSize(2);

https://jasmine.github.io/api/edge/matchers.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. After taking another look, .toEqual([]) seems more explicit than .toHaveSize(0), so I went with that. (I'm too used to Jest and TypeScript; if there's a better way to write these tests, please let me know.)

@etimberg etimberg requested a review from kurkle October 23, 2021 14:41
@kurkle kurkle merged commit aac0bef into chartjs:master Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hover with adding and removing datasets may fail
3 participants