Skip to content

Commit

Permalink
fix!: destroy chart instance when disconnected from the DOM (#7785)
Browse files Browse the repository at this point in the history
  • Loading branch information
@web-padawan
web-padawan authored Sep 11, 2024
1 parent d4202cf commit 554b1e1
Show file tree
Hide file tree
Showing 3 changed files with 240 additions and 63 deletions.
55 changes: 34 additions & 21 deletions packages/charts/src/vaadin-chart.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import Pointer from 'highcharts/es-modules/Core/Pointer.js';
import Highcharts from 'highcharts/es-modules/masters/highstock.src.js';
import { defineCustomElement } from '@vaadin/component-base/src/define.js';
import { ElementMixin } from '@vaadin/component-base/src/element-mixin.js';
import { get } from '@vaadin/component-base/src/path-utils.js';
import { ResizeMixin } from '@vaadin/component-base/src/resize-mixin.js';
import { ThemableMixin } from '@vaadin/vaadin-themable-mixin/vaadin-themable-mixin.js';
import { inflateFunctions } from './helpers.js';
Expand Down Expand Up @@ -923,9 +924,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {
return;
}

const options = { ...this.options, ...this._jsonConfigurationBuffer };
this._jsonConfigurationBuffer = null;
this.__initChart(options);
this.__resetChart();
this.__addChildObserver();
this.__checkTurboMode();
});
Expand Down Expand Up @@ -1078,11 +1077,24 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {
/** @protected */
disconnectedCallback() {
super.disconnectedCallback();

if (this.configuration) {
this.configuration.destroy();
this.configuration = undefined;
}

if (this._childObserver) {
this._childObserver.disconnect();
}
}

/** @private */
__resetChart() {
const initialOptions = { ...this.options, ...this._jsonConfigurationBuffer };
this.__initChart(initialOptions);
this._jsonConfigurationBuffer = null;
}

/**
* Search for axis with given `id`.
*
Expand Down Expand Up @@ -1179,11 +1191,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {
}

if (resetConfiguration) {
const initialOptions = { ...this.options, ...this._jsonConfigurationBuffer };

this.__initChart(initialOptions);

this._jsonConfigurationBuffer = null;
this.__resetChart();
return;
}

Expand Down Expand Up @@ -1393,6 +1401,11 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {
}, object);
}

/** @private */
__hasConfigurationBuffer(path) {
return get(path, this._jsonConfigurationBuffer) !== undefined;
}

/** @private */
__updateOrAddCredits(credits) {
if (this.configuration.credits) {
Expand Down Expand Up @@ -1441,7 +1454,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__updateCategories(categories, config) {
if (categories === undefined || !config) {
if (categories === undefined || !config || this.__hasConfigurationBuffer('xAxis.categories')) {
return;
}

Expand All @@ -1450,7 +1463,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__updateCategoryMax(max, config) {
if (max === undefined || !config) {
if (max === undefined || !config || this.__hasConfigurationBuffer('xAxis.max')) {
return;
}

Expand All @@ -1464,7 +1477,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__updateCategoryMin(min, config) {
if (min === undefined || !config) {
if (min === undefined || !config || this.__hasConfigurationBuffer('xAxis.min')) {
return;
}

Expand Down Expand Up @@ -1499,7 +1512,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__updateCategoryPosition(categoryPosition, config) {
if (categoryPosition === undefined || !config) {
if (categoryPosition === undefined || !config || this.__hasConfigurationBuffer('chart.inverted')) {
return;
}

Expand All @@ -1525,7 +1538,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__hideLegend(noLegend, config) {
if (noLegend === undefined || !config) {
if (noLegend === undefined || !config || this.__hasConfigurationBuffer('legend')) {
return;
}

Expand All @@ -1538,7 +1551,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__updateTitle(title, config) {
if (title === undefined || !config) {
if (title === undefined || !config || this.__hasConfigurationBuffer('title')) {
return;
}

Expand All @@ -1549,7 +1562,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__tooltipObserver(tooltip, config) {
if (tooltip === undefined || !config) {
if (tooltip === undefined || !config || this.__hasConfigurationBuffer('tooltip')) {
return;
}

Expand All @@ -1558,7 +1571,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__updateType(type, config) {
if (type === undefined || !config) {
if (type === undefined || !config || this.__hasConfigurationBuffer('chart.type')) {
return;
}

Expand All @@ -1571,7 +1584,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__updateSubtitle(subtitle, config) {
if (subtitle === undefined || !config) {
if (subtitle === undefined || !config || this.__hasConfigurationBuffer('subtitle')) {
return;
}

Expand Down Expand Up @@ -1602,7 +1615,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__stackingObserver(stacking, config) {
if (stacking === undefined || !config) {
if (stacking === undefined || !config || this.__hasConfigurationBuffer('plotOptions.series.stacking')) {
return;
}

Expand All @@ -1620,7 +1633,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__chart3dObserver(chart3d, config) {
if (chart3d === undefined || !config) {
if (chart3d === undefined || !config || this.__hasConfigurationBuffer('chart.options3d')) {
return;
}

Expand All @@ -1647,7 +1660,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__polarObserver(polar, config) {
if (polar === undefined || !config) {
if (polar === undefined || !config || this.__hasConfigurationBuffer('chart.polar')) {
return;
}

Expand All @@ -1658,7 +1671,7 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {

/** @private */
__emptyTextObserver(emptyText, config) {
if (emptyText === undefined || !config) {
if (emptyText === undefined || !config || this.__hasConfigurationBuffer('lang.noData')) {
return;
}

Expand Down
42 changes: 0 additions & 42 deletions packages/charts/test/chart-element.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,48 +367,6 @@ describe('vaadin-chart', () => {
});
});

describe('reattach', () => {
let wrapper, inner, chart;

beforeEach(async () => {
wrapper = fixtureSync(`
<div>
<vaadin-chart>
<vaadin-chart-series value="[0,1,2]"></vaadin-chart-series>
<vaadin-chart-series value="[3,2,1]"></vaadin-chart-series>
</vaadin-chart>
<div id="inner"></div>
</div>
`);
chart = wrapper.querySelector('vaadin-chart');
inner = wrapper.querySelector('#inner');
await oneEvent(chart, 'chart-load');
});

it('should keep chart configuration when attached to a new parent', async () => {
expect(chart.configuration.series.length).to.be.equal(chart.childElementCount);
inner.appendChild(chart);
await oneEvent(chart, 'chart-redraw');
expect(chart.configuration.series.length).to.be.equal(chart.childElementCount);
});

it('should apply configuration update when attached to a new parent', async () => {
chart.updateConfiguration({ title: { text: 'Awesome title' }, credits: { enabled: false } });
await oneEvent(chart, 'chart-redraw');
expect(chart.$.chart.querySelector('.highcharts-title').textContent).to.equal('Awesome title');
expect(chart.$.chart.querySelector('.highcharts-credits')).to.be.null;

wrapper.removeChild(chart);
chart.updateConfiguration({ subtitle: { text: 'Awesome subtitle' }, credits: { enabled: true, text: 'Vaadin' } });

inner.appendChild(chart);
await oneEvent(chart, 'chart-redraw');
expect(chart.$.chart.querySelector('.highcharts-title').textContent).to.equal('Awesome title');
expect(chart.$.chart.querySelector('.highcharts-subtitle').textContent).to.equal('Awesome subtitle');
expect(chart.$.chart.querySelector('.highcharts-credits').textContent).to.equal('Vaadin');
});
});

describe('RTL', () => {
let chart;

Expand Down
Loading

0 comments on commit 554b1e1

Please sign in to comment.