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 
…CP: 23.5) (#8104)

Co-authored-by: Serhii Kulykov <[email protected]>
fix!: destroy chart instance when disconnected from the DOM (#7785)
  • Loading branch information
@DiegoCardoso @web-padawan
DiegoCardoso and web-padawan authored Nov 7, 2024
1 parent acfb2f8 commit ae8253c
Show file tree
Hide file tree
Showing 3 changed files with 276 additions and 87 deletions.
112 changes: 67 additions & 45 deletions packages/charts/src/vaadin-chart.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,16 @@ export function deepMerge(target, source) {
return target;
}

/**
* Convenience method for reading a value from a path.
*
* @param {string} path
* @param {object} object
*/
function get(path, object) {
return path.split('.').reduce((obj, property) => (obj ? obj[property] : undefined), object);
}

['exportChart', 'exportChartLocal', 'getSVG'].forEach((methodName) => {
/* eslint-disable no-invalid-this, prefer-arrow-callback */
Highcharts.wrap(Highcharts.Chart.prototype, methodName, function (proceed, ...args) {
Expand Down Expand Up @@ -544,33 +554,6 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {
};
}

/** @protected */
connectedCallback() {
super.connectedCallback();
this.__updateStyles();
beforeNextRender(this, () => {
// Detect if the chart had already been initialized. This might happen in
// environments where the chart is lazily attached (e.g Grid).
if (this.configuration) {
this.__reflow();
return;
}

const options = { ...this.options, ...this._jsonConfigurationBuffer };
this._jsonConfigurationBuffer = null;
this.__initChart(options);
this.__addChildObserver();
this.__checkTurboMode();
});
}

/** @protected */
ready() {
super.ready();

this.addEventListener('chart-redraw', this.__onRedraw.bind(this));
}

/**
* @return {!Options}
*/
Expand Down Expand Up @@ -953,6 +936,31 @@ class Chart extends ResizeMixin(ElementMixin(ThemableMixin(PolymerElement))) {
};
}

/** @protected */
connectedCallback() {
super.connectedCallback();
this.__updateStyles();
beforeNextRender(this, () => {
// Detect if the chart had already been initialized. This might happen in
// environments where the chart is lazily attached (e.g Grid).
if (this.configuration) {
this.__reflow();
return;
}

this.__resetChart();
this.__addChildObserver();
this.__checkTurboMode();
});
}

/** @protected */
ready() {
super.ready();

this.addEventListener('chart-redraw', this.__onRedraw.bind(this));
}

/**
* Implements resize callback from `ResizeMixin`
* to reflow when the chart element is resized.
Expand Down Expand Up @@ -1093,11 +1101,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 @@ -1194,11 +1215,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 @@ -1407,6 +1424,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 @@ -1455,7 +1477,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 @@ -1464,7 +1486,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 @@ -1478,7 +1500,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 @@ -1513,7 +1535,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 @@ -1539,7 +1561,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 @@ -1552,7 +1574,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 @@ -1563,7 +1585,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 @@ -1572,7 +1594,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 @@ -1585,7 +1607,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 @@ -1616,7 +1638,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 @@ -1634,7 +1656,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 @@ -1661,7 +1683,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 @@ -1672,7 +1694,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 ae8253c

Please sign in to comment.