Add a check to Grid#_configColumns that produces a console warning wh…

…en a column definition is shared between grids.
dojo · Jan 23, 2018 · 2d0e895 · 2d0e895
1 parent 34e85b8
commit 2d0e895
Show file tree

Hide file tree

Showing 4 changed files with 107 additions and 16 deletions.
diff --git a/Grid.js b/Grid.js
@@ -476,7 +476,14 @@ define([
 				}
 
 				// add grid reference to each column object for potential use by plugins
-				column.grid = this;
+				if (!column.grid) {
+					column.grid = this;
+				} else {
+					if (column.grid !== this) {
+						console.warn('Sharing column definition objects with multiple grids is not supported.',
+							column.grid, this);
+					}
+				}
 				subRow.push(column); // make sure it can be iterated on
 			}
 

diff --git a/test/intern/core/columns.js b/test/intern/core/columns.js
@@ -63,6 +63,84 @@ define([
 			grid.renderArray(orderedData.items);
 			runClassNameTests();
 		});
+
+		test.suite('sharing column definitions', function () {
+			var warnCalled = false;
+			var warnMessage;
+			var systemWarn;
+			var grids;
+			var expectedWarnMessage = 'Sharing column definition objects with multiple grids is not supported.';
+
+			test.beforeEach(function () {
+				grids = [];
+				systemWarn = console.warn;
+				warnCalled = false;
+				warnMessage = null;
+				console.warn = function (message) {
+					warnMessage = message;
+					warnCalled = true;
+				};
+			});
+
+			test.afterEach(function () {
+				console.warn = systemWarn;
+				for (var i = 0, l = grids.length; i < l; i++) {
+					grids[i].destroy();
+				}
+			});
+
+			test.test('detect warning during construction', function () {
+				var columns = {
+					col1: 'Column 1',
+					col3: 'Column 3',
+					col5: 'Column 5'
+				};
+				grids.push(new Grid({
+					columns: columns
+				}));
+				grids.push(new Grid({
+					columns: columns
+				}));
+				assert.isTrue(warnCalled);
+				assert.strictEqual(warnMessage, expectedWarnMessage);
+			});
+
+			test.test('detect warning during column setting', function () {
+				var columns1 = {
+					col1: 'Column 1',
+					col3: 'Column 3',
+					col5: 'Column 5'
+				};
+				var columns2 = {
+					col1: 'Column 1',
+					col3: 'Column 3',
+					col5: 'Column 5'
+				};
+				grids.push(new Grid({
+					columns: columns1
+				}));
+				grids.push(new Grid({
+					columns: columns2
+				}));
+				assert.isFalse(warnCalled);
+				grids[1].set('columns', columns1);
+				assert.isTrue(warnCalled);
+				assert.strictEqual(warnMessage, expectedWarnMessage);
+			});
+
+			test.test('no warning when same columns set again', function () {
+				var columns = {
+					col1: 'Column 1',
+					col3: 'Column 3',
+					col5: 'Column 5'
+				};
+				grids.push(new Grid({
+					columns: columns
+				}));
+				grids[0].set('columns', columns);
+				assert.isFalse(warnCalled);
+			});
+		});
 	});
 
 	test.suite('columnSets', function () {

diff --git a/test/intern/core/setClass.js b/test/intern/core/setClass.js
@@ -45,21 +45,23 @@ define([
 
 		test.test('Grids + initially-defined classes', function () {
 			// Build three grids
-			var columns = {
+			function getColumns() {
+				return {
 					order: 'step', // give column a custom name
 					name: {},
 					description: { label: 'what to do', sortable: false }
-				},
-				gridC = window.gridC = new Grid({
+				};
+			}
+			var gridC = window.gridC = new Grid({
 					'class': 'c',
-					columns: columns
+					columns: getColumns()
 				}),
 				gridCN = window.gridCN = new Grid({
 					'class': 'cn',
-					columns: columns
+					columns: getColumns()
 				}),
 				gridDOM = window.gridDOM = new Grid({
-					columns: columns
+					columns: getColumns()
 				}, domConstruct.create('div', { 'class': 'dom' }));
 
 			// Check the classes on each List.domNode

diff --git a/test/intern/mixins/Keyboard.js b/test/intern/mixins/Keyboard.js
@@ -14,12 +14,16 @@ define([
 	'../addCss!'
 ], function (test, assert, OnDemandList, OnDemandGrid, Keyboard, ColumnSet,
 		declare, domConstruct, on, query, createSyncStore, genericData) {
-	var handles = [],
-		columns = {
+
+	function getColumns() {
+		return {
 			col1: 'Column 1',
 			col3: 'Column 3',
 			col5: 'Column 5'
-		},
+		};
+	}
+
+	var handles = [],
 		testStore = createSyncStore({ data: genericData }),
 		item = testStore.getSync(1),
 		grid,
@@ -180,7 +184,7 @@ define([
 	test.suite('Keyboard (Grid + cellNavigation:true)', function () {
 		test.before(function () {
 			grid = new (declare([OnDemandGrid, Keyboard]))({
-				columns: columns,
+				columns: getColumns(),
 				sort: 'id',
 				collection: testStore
 			});
@@ -329,7 +333,7 @@ define([
 	test.suite('Keyboard focus preservation', function () {
 		test.before(function () {
 			grid = new (declare([OnDemandGrid, Keyboard]))({
-				columns: columns,
+				columns: getColumns(),
 				sort: 'id',
 				collection: testStore
 			});
@@ -410,7 +414,7 @@ define([
 
 		test.before(function () {
 			grid = new (declare([OnDemandGrid, Keyboard]))({
-				columns: columns,
+				columns: getColumns(),
 				sort: 'id',
 				collection: testStore
 			});
@@ -503,7 +507,7 @@ define([
 		test.before(function () {
 			grid = new (declare([OnDemandGrid, Keyboard]))({
 				cellNavigation: false,
-				columns: columns,
+				columns: getColumns(),
 				sort: 'id',
 				collection: testStore
 			});
@@ -537,7 +541,7 @@ define([
 		test.beforeEach(function () {
 			grid = new (declare([ OnDemandGrid, Keyboard ]))({
 				collection: store,
-				columns: columns
+				columns: getColumns()
 			});
 			document.body.appendChild(grid.domNode);
 			grid.startup();
@@ -573,7 +577,7 @@ define([
 			var store = createSyncStore({ data: genericData });
 			grid = new (declare([ OnDemandGrid, Keyboard ]))({
 				collection: store,
-				columns: columns
+				columns: getColumns()
 			});
 			document.body.appendChild(grid.domNode);
 			grid.startup();