From b970cdd27e7052bbbb293e9dfa6a96cd82ac8bd1 Mon Sep 17 00:00:00 2001 From: Ben Date: Tue, 23 May 2017 20:33:12 -0400 Subject: [PATCH 1/5] Output a warning whenever user tries to use datasets without a unique key --- src/index.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index ad7d68e42..4b7d9e459 100644 --- a/src/index.js +++ b/src/index.js @@ -6,7 +6,7 @@ import isEqual from 'lodash.isequal'; class ChartComponent extends React.Component { static getLabelAsKey = d => d.label; - + static propTypes = { data: PropTypes.oneOfType([ PropTypes.object, @@ -155,8 +155,14 @@ class ChartComponent extends React.Component { // use the key provider to work out which series have been added/removed/changed const currentDatasetKeys = currentDatasets.map(this.props.datasetKeyProvider); const nextDatasetKeys = nextDatasets.map(this.props.datasetKeyProvider); + const shouldWarn = !currentDatasetKeys.every(d => typeof d !== "undefined") || !nextDatasetKeys.every(d => typeof d !== "undefined"); const newDatasets = nextDatasets.filter(d => currentDatasetKeys.indexOf(this.props.datasetKeyProvider(d)) === -1); + if (shouldWarn && !this.hasWarned) { + this.hasWarned = true; // Only warn once per chart so console isn't spammed with warnings + console.error('[react-chartjs-2] Warning: Each dataset need a unique key. By default, the "label" property on each dataset is used. Alternatively, you may provide a "datasetKeyProvider" as a prop that returns a unique key.'); + } + // process the updates (via a reverse for loop so we can safely splice deleted datasets out of the array for (let idx = currentDatasets.length - 1; idx >= 0; idx -= 1) { const currentDatasetKey = this.props.datasetKeyProvider(currentDatasets[idx]); From 70e91d9e4a0fda966df137373f1c8b80ceddf73f Mon Sep 17 00:00:00 2001 From: Ben Date: Tue, 23 May 2017 20:38:14 -0400 Subject: [PATCH 2/5] grammar fix --- src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index 4b7d9e459..bca0ccbd0 100644 --- a/src/index.js +++ b/src/index.js @@ -160,7 +160,7 @@ class ChartComponent extends React.Component { if (shouldWarn && !this.hasWarned) { this.hasWarned = true; // Only warn once per chart so console isn't spammed with warnings - console.error('[react-chartjs-2] Warning: Each dataset need a unique key. By default, the "label" property on each dataset is used. Alternatively, you may provide a "datasetKeyProvider" as a prop that returns a unique key.'); + console.error('[react-chartjs-2] Warning: Each dataset needs a unique key. By default, the "label" property on each dataset is used. Alternatively, you may provide a "datasetKeyProvider" as a prop that returns a unique key.'); } // process the updates (via a reverse for loop so we can safely splice deleted datasets out of the array From ea08fa1d745d327de3e251e12d3fca05f35d92dd Mon Sep 17 00:00:00 2001 From: Ben Date: Fri, 7 Jul 2017 10:56:57 -0400 Subject: [PATCH 3/5] only show warning in dev enviroment --- src/index.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/index.js b/src/index.js index bca0ccbd0..5a57a8259 100644 --- a/src/index.js +++ b/src/index.js @@ -4,6 +4,21 @@ import ReactDOM from 'react-dom'; import Chart from 'chart.js'; import isEqual from 'lodash.isequal'; +// Gets React's enviroment +// https://stackoverflow.com/a/25922668 +const env = (() => { + try { + React.createClass({}); + } catch(e) { + if (e.message.indexOf('render') >= 0) { + return 'dev'; // A nice, specific error message + } else { + return 'prod'; // A generic error message + } + } + return 'prod'; // should never happen, but play it safe. +})(); + class ChartComponent extends React.Component { static getLabelAsKey = d => d.label; @@ -155,10 +170,10 @@ class ChartComponent extends React.Component { // use the key provider to work out which series have been added/removed/changed const currentDatasetKeys = currentDatasets.map(this.props.datasetKeyProvider); const nextDatasetKeys = nextDatasets.map(this.props.datasetKeyProvider); - const shouldWarn = !currentDatasetKeys.every(d => typeof d !== "undefined") || !nextDatasetKeys.every(d => typeof d !== "undefined"); + const shouldWarn = !currentDatasetKeys.every(d => typeof d !== 'undefined') || !nextDatasetKeys.every(d => typeof d !== 'undefined'); const newDatasets = nextDatasets.filter(d => currentDatasetKeys.indexOf(this.props.datasetKeyProvider(d)) === -1); - if (shouldWarn && !this.hasWarned) { + if (shouldWarn && !this.hasWarned && env === 'dev') { this.hasWarned = true; // Only warn once per chart so console isn't spammed with warnings console.error('[react-chartjs-2] Warning: Each dataset needs a unique key. By default, the "label" property on each dataset is used. Alternatively, you may provide a "datasetKeyProvider" as a prop that returns a unique key.'); } From 6802aa5e08e52e68c03b080c2f398d49fe7eee5b Mon Sep 17 00:00:00 2001 From: Ben Date: Sat, 3 Feb 2018 18:17:38 -0500 Subject: [PATCH 4/5] Rebased, changed NODE_ENV to get from process.env, moved code to checkDatasets function --- src/index.js | 36 ++++++++++------- test/__tests__/Chart_spec.js | 75 ++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 14 deletions(-) diff --git a/src/index.js b/src/index.js index f8fa3ee5a..cd642b18d 100644 --- a/src/index.js +++ b/src/index.js @@ -5,20 +5,7 @@ import isEqual from 'lodash/isEqual'; import find from 'lodash/find'; import keyBy from 'lodash/keyBy'; -// Gets React's enviroment -// https://stackoverflow.com/a/25922668 -const env = (() => { - try { - React.createClass({}); - } catch(e) { - if (e.message.indexOf('render') >= 0) { - return 'dev'; // A nice, specific error message - } else { - return 'prod'; // A generic error message - } - } - return 'prod'; // should never happen, but play it safe. -})(); +const NODE_ENV = (typeof process !== 'undefined') && process.env && process.env.NODE_ENV class ChartComponent extends React.Component { static getLabelAsKey = d => d.label; @@ -159,6 +146,25 @@ class ChartComponent extends React.Component { return data; } + checkDatasets(datasets) { + const isDev = NODE_ENV !== 'production' && NODE_ENV !== 'prod'; + const usingCustomKeyProvider = this.props.datasetKeyProvider !== ChartComponent.getLabelAsKey; + const multipleDatasets = datasets.length > 1; + + if (isDev && multipleDatasets && !usingCustomKeyProvider) { + let shouldWarn = false; + datasets.forEach((dataset) => { + if (!dataset.label) { + shouldWarn = true; + } + }); + + if (shouldWarn) { + console.error('[react-chartjs-2] Warning: Each dataset needs a unique key. By default, the "label" property on each dataset is used. Alternatively, you may provide a "datasetKeyProvider" as a prop that returns a unique key.'); + } + } + } + updateChart() { const {options} = this.props; @@ -174,6 +180,7 @@ class ChartComponent extends React.Component { // seamless transitions let currentDatasets = (this.chartInstance.config.data && this.chartInstance.config.data.datasets) || []; const nextDatasets = data.datasets || []; + this.checkDatasets(currentDatasets); const currentDatasetsIndexed = keyBy( currentDatasets, @@ -185,6 +192,7 @@ class ChartComponent extends React.Component { this.chartInstance.config.data.datasets = nextDatasets.map(next => { const current = currentDatasetsIndexed[this.props.datasetKeyProvider(next)]; + if (current && current.type === next.type) { // The data array must be edited in place. As chart.js adds listeners to it. current.data.splice(next.data.length); diff --git a/test/__tests__/Chart_spec.js b/test/__tests__/Chart_spec.js index 320b0a62c..15b5629a5 100644 --- a/test/__tests__/Chart_spec.js +++ b/test/__tests__/Chart_spec.js @@ -222,4 +222,79 @@ describe('', () => { expect(dataFn.calledWith(canvas)).to.equal(true); }); }); + + describe('checkDatasets', () => { + let consoleStub = null; + beforeEach(() => { + consoleStub = sinon.stub(global.console, 'error'); + }); + + afterEach(() => { + consoleStub.restore(); + consoleStub = null; + }); + + it('should log error to console if datasets don\'t have a label', () => { + const wrapper = mountComponent({ data: {} }); + wrapper.setProps({ + data: { + datasets: [ + { + _id: '238940890234809234', + data: [10, 20, 10, 20, 10, 20, 10] + }, + { + _id: '098340598345839455', + data: [50, 100, 50, 100, 50, 100, 50] + } + ] + } + }); + wrapper.update(); + + expect(consoleStub.callCount).to.equal(1); + }); + + it('should not log error to console if all datasets have a label', () => { + const wrapper = mountComponent({ data: {} }); + wrapper.setProps({ + data: { + datasets: [ + { + label: 'My first dataset', + data: [10, 20, 10, 20, 10, 20, 10] + }, + { + label: 'My second dataset', + data: [50, 100, 50, 100, 50, 100, 50] + } + ] + } + }); + wrapper.update(); + + expect(consoleStub.callCount).to.equal(0); + }); + + it('should not log error to console if a custom datasetKeyProvider is provided', () => { + const wrapper = mountComponent({ datasetKeyProvider: (d) => d._id }); + wrapper.setProps({ + data: { + datasets: [ + { + _id: '238940890234809234', + data: [10, 20, 10, 20, 10, 20, 10] + }, + { + _id: '098340598345839455', + data: [50, 100, 50, 100, 50, 100, 50] + } + ] + } + }); + wrapper.update(); + + expect(consoleStub.callCount).to.equal(0); + }); + }); }); From 9a6d79bc3a72ddf3f74bf33d2991679f7a6801b8 Mon Sep 17 00:00:00 2001 From: Ben Date: Sat, 3 Feb 2018 18:19:49 -0500 Subject: [PATCH 5/5] linting fixes --- src/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/index.js b/src/index.js index cd642b18d..92c7ac863 100644 --- a/src/index.js +++ b/src/index.js @@ -5,7 +5,7 @@ import isEqual from 'lodash/isEqual'; import find from 'lodash/find'; import keyBy from 'lodash/keyBy'; -const NODE_ENV = (typeof process !== 'undefined') && process.env && process.env.NODE_ENV +const NODE_ENV = (typeof process !== 'undefined') && process.env && process.env.NODE_ENV; class ChartComponent extends React.Component { static getLabelAsKey = d => d.label; @@ -256,7 +256,7 @@ class ChartComponent extends React.Component { } ref = (element) => { - this.element = element + this.element = element; } render() {