From 0f297ad77d94eb37879f0bcaad0fddd4b3315a1f Mon Sep 17 00:00:00 2001 From: etimberg Date: Sat, 9 Feb 2019 19:33:04 -0500 Subject: [PATCH 1/6] Handle a frozen `dataset.data` array. --- docs/developers/updates.md | 2 +- src/core/core.datasetController.js | 4 +++- test/specs/core.datasetController.tests.js | 16 ++++++++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/docs/developers/updates.md b/docs/developers/updates.md index f515a3a3f64..cca62a28126 100644 --- a/docs/developers/updates.md +++ b/docs/developers/updates.md @@ -4,7 +4,7 @@ It's pretty common to want to update charts after they've been created. When the ## Adding or Removing Data -Adding and removing data is supported by changing the data array. To add data, just add data into the data array as seen in this example. +Adding and removing data is supported by changing the data array. To add data, just add data into the data array as seen in this example. However, if `dataset.data` has been frozen using `Object.freeze()`, adding or removing data will not update the chart. ```javascript function addData(chart, label, data) { diff --git a/src/core/core.datasetController.js b/src/core/core.datasetController.js index 6a7d36f196b..c5834e4dd99 100644 --- a/src/core/core.datasetController.js +++ b/src/core/core.datasetController.js @@ -225,7 +225,9 @@ helpers.extend(DatasetController.prototype, { unlistenArrayEvents(me._data, me); } - listenArrayEvents(data, me); + if (data && !Object.isFrozen(data)) { + listenArrayEvents(data, me); + } me._data = data; } diff --git a/test/specs/core.datasetController.tests.js b/test/specs/core.datasetController.tests.js index 5ab9dd5f31f..b8ebdc11b19 100644 --- a/test/specs/core.datasetController.tests.js +++ b/test/specs/core.datasetController.tests.js @@ -38,6 +38,22 @@ describe('Chart.DatasetController', function() { }); }); + it('should handle a frozen data object', function() { + function createChart() { + var data = Object.freeze([0, 1, 2, 3, 4, 5]); + acquireChart({ + type: 'line', + data: { + datasets: [{ + data: data + }] + } + }); + } + + expect(createChart).not.toThrow(); + }); + it('should synchronize metadata when data are inserted or removed', function() { var data = [0, 1, 2, 3, 4, 5]; var chart = acquireChart({ From 47c56fcc2b7cb8c70708014c3f015b8eb82dbbf5 Mon Sep 17 00:00:00 2001 From: etimberg Date: Sun, 10 Feb 2019 08:32:45 -0500 Subject: [PATCH 2/6] Remove the documentation change --- docs/developers/updates.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developers/updates.md b/docs/developers/updates.md index cca62a28126..f515a3a3f64 100644 --- a/docs/developers/updates.md +++ b/docs/developers/updates.md @@ -4,7 +4,7 @@ It's pretty common to want to update charts after they've been created. When the ## Adding or Removing Data -Adding and removing data is supported by changing the data array. To add data, just add data into the data array as seen in this example. However, if `dataset.data` has been frozen using `Object.freeze()`, adding or removing data will not update the chart. +Adding and removing data is supported by changing the data array. To add data, just add data into the data array as seen in this example. ```javascript function addData(chart, label, data) { From e9bd48003578a17d8d43207b2e5a7eec5554b513 Mon Sep 17 00:00:00 2001 From: etimberg Date: Sun, 10 Feb 2019 08:37:27 -0500 Subject: [PATCH 3/6] Update the test to include destroying the chart --- test/specs/core.datasetController.tests.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/specs/core.datasetController.tests.js b/test/specs/core.datasetController.tests.js index b8ebdc11b19..a58bf670ffc 100644 --- a/test/specs/core.datasetController.tests.js +++ b/test/specs/core.datasetController.tests.js @@ -41,7 +41,7 @@ describe('Chart.DatasetController', function() { it('should handle a frozen data object', function() { function createChart() { var data = Object.freeze([0, 1, 2, 3, 4, 5]); - acquireChart({ + var chart = acquireChart({ type: 'line', data: { datasets: [{ @@ -49,6 +49,9 @@ describe('Chart.DatasetController', function() { }] } }); + + // Tests that the unlisten path also works for frozen objects + chart.destroy(); } expect(createChart).not.toThrow(); From cadd32630b29d4a8a8483615231822a7f93edf48 Mon Sep 17 00:00:00 2001 From: etimberg Date: Sun, 10 Feb 2019 15:42:59 -0500 Subject: [PATCH 4/6] Handle sealed objects & test more cases --- src/core/core.datasetController.js | 2 +- test/specs/core.datasetController.tests.js | 89 +++++++++++++++++----- 2 files changed, 73 insertions(+), 18 deletions(-) diff --git a/src/core/core.datasetController.js b/src/core/core.datasetController.js index c5834e4dd99..afd021c1db1 100644 --- a/src/core/core.datasetController.js +++ b/src/core/core.datasetController.js @@ -225,7 +225,7 @@ helpers.extend(DatasetController.prototype, { unlistenArrayEvents(me._data, me); } - if (data && !Object.isFrozen(data)) { + if (data && Object.isExtensible(data)) { listenArrayEvents(data, me); } me._data = data; diff --git a/test/specs/core.datasetController.tests.js b/test/specs/core.datasetController.tests.js index a58bf670ffc..3f6a3e46f85 100644 --- a/test/specs/core.datasetController.tests.js +++ b/test/specs/core.datasetController.tests.js @@ -38,23 +38,78 @@ describe('Chart.DatasetController', function() { }); }); - it('should handle a frozen data object', function() { - function createChart() { - var data = Object.freeze([0, 1, 2, 3, 4, 5]); - var chart = acquireChart({ - type: 'line', - data: { - datasets: [{ - data: data - }] - } - }); - - // Tests that the unlisten path also works for frozen objects - chart.destroy(); - } - - expect(createChart).not.toThrow(); + describe('inextensible data', function() { + it('should handle a frozen data object', function() { + function createChart() { + var data = Object.freeze([0, 1, 2, 3, 4, 5]); + var chart = acquireChart({ + type: 'line', + data: { + datasets: [{ + data: data + }] + } + }); + + chart.data.datasets[0].data = Object.freeze([5, 4, 3, 2, 1, 0]); + chart.update(); + + // Tests that the unlisten path also works for frozen objects + chart.destroy(); + } + + expect(createChart).not.toThrow(); + }); + + it('should handle a sealed data object', function() { + function createChart() { + var data = [0, 1, 2, 3, 4, 5]; + Object.seal(data); + var chart = acquireChart({ + type: 'line', + data: { + datasets: [{ + data: data + }] + } + }); + + var data2 = [5, 4, 3, 2, 1, 0]; + Object.seal(data2) + chart.data.datasets[0].data = data2; + chart.update(); + + // Tests that the unlisten path also works for frozen objects + chart.destroy(); + } + + expect(createChart).not.toThrow(); + }); + + it('should handle an unextendable data object', function() { + function createChart() { + var data = [0, 1, 2, 3, 4, 5]; + Object.preventExtensions(data); + var chart = acquireChart({ + type: 'line', + data: { + datasets: [{ + data: data + }] + } + }); + + var data2 = [5, 4, 3, 2, 1, 0]; + Object.preventExtensions(data2) + chart.data.datasets[0].data = data2; + chart.update(); + + // Tests that the unlisten path also works for frozen objects + chart.destroy(); + } + + expect(createChart).not.toThrow(); + }); }); it('should synchronize metadata when data are inserted or removed', function() { From 14c18ef72e23f593ff1b51cd30de0d5f5f2ee199 Mon Sep 17 00:00:00 2001 From: etimberg Date: Sun, 10 Feb 2019 21:12:46 -0500 Subject: [PATCH 5/6] Update tests and fix lint issues --- test/specs/core.datasetController.tests.js | 32 +++++++++------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/test/specs/core.datasetController.tests.js b/test/specs/core.datasetController.tests.js index 3f6a3e46f85..382b1032343 100644 --- a/test/specs/core.datasetController.tests.js +++ b/test/specs/core.datasetController.tests.js @@ -50,21 +50,20 @@ describe('Chart.DatasetController', function() { }] } }); - + chart.data.datasets[0].data = Object.freeze([5, 4, 3, 2, 1, 0]); chart.update(); - + // Tests that the unlisten path also works for frozen objects chart.destroy(); } - + expect(createChart).not.toThrow(); }); it('should handle a sealed data object', function() { function createChart() { - var data = [0, 1, 2, 3, 4, 5]; - Object.seal(data); + var data = Object.seal([0, 1, 2, 3, 4, 5]); var chart = acquireChart({ type: 'line', data: { @@ -73,23 +72,20 @@ describe('Chart.DatasetController', function() { }] } }); - - var data2 = [5, 4, 3, 2, 1, 0]; - Object.seal(data2) - chart.data.datasets[0].data = data2; + + chart.data.datasets[0].data = Object.seal([5, 4, 3, 2, 1, 0]); chart.update(); - + // Tests that the unlisten path also works for frozen objects chart.destroy(); } - + expect(createChart).not.toThrow(); }); it('should handle an unextendable data object', function() { function createChart() { - var data = [0, 1, 2, 3, 4, 5]; - Object.preventExtensions(data); + var data = Object.preventExtensions([0, 1, 2, 3, 4, 5]); var chart = acquireChart({ type: 'line', data: { @@ -98,16 +94,14 @@ describe('Chart.DatasetController', function() { }] } }); - - var data2 = [5, 4, 3, 2, 1, 0]; - Object.preventExtensions(data2) - chart.data.datasets[0].data = data2; + + chart.data.datasets[0].data = Object.preventExtensions([5, 4, 3, 2, 1, 0]); chart.update(); - + // Tests that the unlisten path also works for frozen objects chart.destroy(); } - + expect(createChart).not.toThrow(); }); }); From dd980a88ba6f4840392f0c7f8af27b0eee0da355 Mon Sep 17 00:00:00 2001 From: etimberg Date: Mon, 11 Feb 2019 07:10:50 -0500 Subject: [PATCH 6/6] Check that the test setup is correct --- test/specs/core.datasetController.tests.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/test/specs/core.datasetController.tests.js b/test/specs/core.datasetController.tests.js index 382b1032343..8b5d6dec2e0 100644 --- a/test/specs/core.datasetController.tests.js +++ b/test/specs/core.datasetController.tests.js @@ -42,6 +42,8 @@ describe('Chart.DatasetController', function() { it('should handle a frozen data object', function() { function createChart() { var data = Object.freeze([0, 1, 2, 3, 4, 5]); + expect(Object.isExtensible(data)).toBeFalsy(); + var chart = acquireChart({ type: 'line', data: { @@ -51,7 +53,9 @@ describe('Chart.DatasetController', function() { } }); - chart.data.datasets[0].data = Object.freeze([5, 4, 3, 2, 1, 0]); + var dataset = chart.data.datasets[0]; + dataset.data = Object.freeze([5, 4, 3, 2, 1, 0]); + expect(Object.isExtensible(dataset.data)).toBeFalsy(); chart.update(); // Tests that the unlisten path also works for frozen objects @@ -64,6 +68,8 @@ describe('Chart.DatasetController', function() { it('should handle a sealed data object', function() { function createChart() { var data = Object.seal([0, 1, 2, 3, 4, 5]); + expect(Object.isExtensible(data)).toBeFalsy(); + var chart = acquireChart({ type: 'line', data: { @@ -73,7 +79,9 @@ describe('Chart.DatasetController', function() { } }); - chart.data.datasets[0].data = Object.seal([5, 4, 3, 2, 1, 0]); + var dataset = chart.data.datasets[0]; + dataset.data = Object.seal([5, 4, 3, 2, 1, 0]); + expect(Object.isExtensible(dataset.data)).toBeFalsy(); chart.update(); // Tests that the unlisten path also works for frozen objects @@ -86,6 +94,8 @@ describe('Chart.DatasetController', function() { it('should handle an unextendable data object', function() { function createChart() { var data = Object.preventExtensions([0, 1, 2, 3, 4, 5]); + expect(Object.isExtensible(data)).toBeFalsy(); + var chart = acquireChart({ type: 'line', data: { @@ -95,7 +105,9 @@ describe('Chart.DatasetController', function() { } }); - chart.data.datasets[0].data = Object.preventExtensions([5, 4, 3, 2, 1, 0]); + var dataset = chart.data.datasets[0]; + dataset.data = Object.preventExtensions([5, 4, 3, 2, 1, 0]); + expect(Object.isExtensible(dataset.data)).toBeFalsy(); chart.update(); // Tests that the unlisten path also works for frozen objects