From 8e7bde25c46c3c76c9c10119e93d2ea744e13f70 Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Tue, 9 Jun 2020 00:49:55 +0300 Subject: [PATCH] Remove data checks and always re-parse data (#7458) * Remove data checks and always re-parse data * Fix removed helper namespace --- docs/docs/getting-started/v3-migration.md | 1 + src/core/core.datasetController.js | 80 +++------------------- src/helpers/helpers.core.js | 31 --------- test/specs/core.datasetController.tests.js | 24 +++++++ test/specs/helpers.core.tests.js | 23 ------- 5 files changed, 33 insertions(+), 126 deletions(-) diff --git a/docs/docs/getting-started/v3-migration.md b/docs/docs/getting-started/v3-migration.md index 8405fa728..8476b4948 100644 --- a/docs/docs/getting-started/v3-migration.md +++ b/docs/docs/getting-started/v3-migration.md @@ -250,6 +250,7 @@ The following properties and methods were removed: * `helpers.addEvent` * `helpers.aliasPixel` +* `helpers.arrayEquals` * `helpers.configMerge` * `helpers.findIndex` * `helpers.findNextWhere` diff --git a/src/core/core.datasetController.js b/src/core/core.datasetController.js index a5626b899..48b2c7968 100644 --- a/src/core/core.datasetController.js +++ b/src/core/core.datasetController.js @@ -1,5 +1,5 @@ import Animations from './core.animations'; -import {isObject, merge, _merger, isArray, valueOrDefault, mergeIf, arrayEquals} from '../helpers/helpers.core'; +import {isObject, merge, _merger, isArray, valueOrDefault, mergeIf} from '../helpers/helpers.core'; import {resolve} from '../helpers/helpers.options'; import {getHoverColor} from '../helpers/helpers.color'; import {sign} from '../helpers/helpers.math'; @@ -233,10 +233,7 @@ export default class DatasetController { this._config = undefined; this._parsing = false; this._data = undefined; - this._dataCopy = undefined; - this._dataModified = false; this._objectData = undefined; - this._labels = undefined; this._scaleStacked = {}; this.initialize(); @@ -339,7 +336,6 @@ export default class DatasetController { } /** - * @return {boolean} whether the data was modified * @private */ _dataCheck() { @@ -352,50 +348,17 @@ export default class DatasetController { // the internal meta data accordingly. if (isObject(data)) { - // Object data is currently monitored for replacement only - if (me._objectData === data) { - return false; - } me._data = convertObjectDataToArray(data); - me._objectData = data; - } else { - if (me._data === data && !me._dataModified && arrayEquals(data, me._dataCopy)) { - return false; - } - + } else if (me._data !== data) { if (me._data) { // This case happens when the user replaced the data array instance. unlistenArrayEvents(me._data, me); } - - // Store a copy to detect direct modifications. - // Note: This is suboptimal, but better than always parsing the data - me._dataCopy = data.slice(0); - - me._dataModified = false; - if (data && Object.isExtensible(data)) { listenArrayEvents(data, me); } me._data = data; } - return true; - } - - /** - * @private - */ - _labelCheck() { - const me = this; - const iScale = me._cachedMeta.iScale; - const labels = iScale ? iScale.getLabels() : me.chart.data.labels; - - if (me._labels === labels) { - return false; - } - - me._labels = labels; - return true; } addElements() { @@ -418,13 +381,12 @@ export default class DatasetController { buildOrUpdateElements() { const me = this; - const dataChanged = me._dataCheck(); - const labelsChanged = me._labelCheck(); - const scaleChanged = me._scaleCheck(); const meta = me._cachedMeta; const dataset = me.getDataset(); let stackChanged = false; + me._dataCheck(); + // make sure cached _stacked status is current meta._stacked = isStacked(meta.vScale, meta); @@ -440,7 +402,7 @@ export default class DatasetController { // Re-sync meta data in case the user replaced the data array or if we missed // any updates and so make sure that we handle number of datapoints changing. - me._resyncElements(dataChanged || labelsChanged || scaleChanged || stackChanged); + me._resyncElements(); // if stack changed, update stack values for the whole dataset if (stackChanged) { @@ -708,22 +670,6 @@ export default class DatasetController { } } - /** - * @private - */ - _scaleCheck() { - const me = this; - const meta = me._cachedMeta; - const iScale = meta.iScale; - const vScale = meta.vScale; - const cache = me._scaleStacked; - return !cache || - !iScale || - !vScale || - cache[iScale.id] !== iScale.options.stacked || - cache[vScale.id] !== vScale.options.stacked; - } - /** * @return {number|boolean} * @protected @@ -1049,7 +995,7 @@ export default class DatasetController { /** * @private */ - _resyncElements(changed) { + _resyncElements() { const me = this; const meta = me._cachedMeta; const numMeta = meta.data.length; @@ -1057,17 +1003,12 @@ export default class DatasetController { if (numData > numMeta) { me._insertElements(numMeta, numData - numMeta); - if (changed && numMeta) { - // _insertElements parses the new elements. The old ones might need parsing too. - me.parse(0, numMeta); - } } else if (numData < numMeta) { meta.data.splice(numData, numMeta - numData); meta._parsed.splice(numData, numMeta - numData); - me.parse(0, numData); - } else if (changed) { - me.parse(0, numData); } + // Re-parse the old elements (new elements are parsed in _insertElements) + me.parse(0, Math.min(numData, numMeta)); } /** @@ -1113,7 +1054,6 @@ export default class DatasetController { _onDataPush() { const count = arguments.length; this._insertElements(this.getDataset().data.length - count, count); - this._dataModified = true; } /** @@ -1121,7 +1061,6 @@ export default class DatasetController { */ _onDataPop() { this._removeElements(this._cachedMeta.data.length - 1, 1); - this._dataModified = true; } /** @@ -1129,7 +1068,6 @@ export default class DatasetController { */ _onDataShift() { this._removeElements(0, 1); - this._dataModified = true; } /** @@ -1138,7 +1076,6 @@ export default class DatasetController { _onDataSplice(start, count) { this._removeElements(start, count); this._insertElements(start, arguments.length - 2); - this._dataModified = true; } /** @@ -1146,7 +1083,6 @@ export default class DatasetController { */ _onDataUnshift() { this._insertElements(0, arguments.length); - this._dataModified = true; } } diff --git a/src/helpers/helpers.core.js b/src/helpers/helpers.core.js index 6a5f52581..01fa51f34 100644 --- a/src/helpers/helpers.core.js +++ b/src/helpers/helpers.core.js @@ -131,37 +131,6 @@ export function each(loopable, fn, thisArg, reverse) { } } -/** - * Returns true if the `a0` and `a1` arrays have the same content, else returns false. - * @see https://stackoverflow.com/a/14853974 - * @param {Array} a0 - The array to compare - * @param {Array} a1 - The array to compare - * @returns {boolean} - */ -export function arrayEquals(a0, a1) { - let i, ilen, v0, v1; - - if (!a0 || !a1 || a0.length !== a1.length) { - return false; - } - - for (i = 0, ilen = a0.length; i < ilen; ++i) { - v0 = a0[i]; - v1 = a1[i]; - - if (v0 instanceof Array && v1 instanceof Array) { - if (!arrayEquals(v0, v1)) { - return false; - } - } else if (v0 !== v1) { - // NOTE: two different object instances will never be equal: {x:20} != {x:20} - return false; - } - } - - return true; -} - /** * Returns true if the `a0` and `a1` arrays have the same content, else returns false. * @param {Array} a0 - The array to compare diff --git a/test/specs/core.datasetController.tests.js b/test/specs/core.datasetController.tests.js index 0e2d83a5f..c78b96a05 100644 --- a/test/specs/core.datasetController.tests.js +++ b/test/specs/core.datasetController.tests.js @@ -369,6 +369,30 @@ describe('Chart.DatasetController', function() { expect(meta.data[0].x).toEqual(firstX); }); + // https://github.com/chartjs/Chart.js/issues/7445 + it('should re-synchronize metadata when data is objects and directly altered', function() { + var data = [{x: 'a', y: 1}, {x: 'b', y: 2}, {x: 'c', y: 3}]; + var chart = acquireChart({ + type: 'line', + data: { + labels: ['a', 'b', 'c'], + datasets: [{ + data, + fill: true + }] + } + }); + + var meta = chart.getDatasetMeta(0); + + expect(meta.data.length).toBe(3); + const y3 = meta.data[2].y; + + data[0].y = 3; + chart.update(); + expect(meta.data[0].y).toEqual(y3); + }); + it('should re-synchronize metadata when scaleID changes', function() { var chart = acquireChart({ type: 'line', diff --git a/test/specs/helpers.core.tests.js b/test/specs/helpers.core.tests.js index aa687784e..9b613d142 100644 --- a/test/specs/helpers.core.tests.js +++ b/test/specs/helpers.core.tests.js @@ -241,29 +241,6 @@ describe('Chart.helpers.core', function() { }); }); - describe('arrayEquals', function() { - it('should return false if arrays are not the same', function() { - expect(helpers.arrayEquals([], [42])).toBeFalsy(); - expect(helpers.arrayEquals([42], ['42'])).toBeFalsy(); - expect(helpers.arrayEquals([1, 2, 3], [1, 2, 3, 4])).toBeFalsy(); - expect(helpers.arrayEquals(['foo', 'bar'], ['bar', 'foo'])).toBeFalsy(); - expect(helpers.arrayEquals([1, 2, 3], [1, 2, 'foo'])).toBeFalsy(); - expect(helpers.arrayEquals([1, 2, [3, 4]], [1, 2, [3, 'foo']])).toBeFalsy(); - expect(helpers.arrayEquals([{a: 42}], [{a: 42}])).toBeFalsy(); - }); - it('should return false if arrays are not the same', function() { - var o0 = {}; - var o1 = {}; - var o2 = {}; - - expect(helpers.arrayEquals([], [])).toBeTruthy(); - expect(helpers.arrayEquals([1, 2, 3], [1, 2, 3])).toBeTruthy(); - expect(helpers.arrayEquals(['foo', 'bar'], ['foo', 'bar'])).toBeTruthy(); - expect(helpers.arrayEquals([true, false, true], [true, false, true])).toBeTruthy(); - expect(helpers.arrayEquals([o0, o1, o2], [o0, o1, o2])).toBeTruthy(); - }); - }); - describe('_elementsEqual', function() { it('should return true if arrays are the same', function() { expect(helpers._elementsEqual( -- 2.47.2