Merge pull request #81 from CrowdStrike/protect-against-invaid-column…

…-configs

Protect against invalid column configs

.changeset/slow-eagles-switch.md

@@ -0,0 +1,11 @@
+---
+'ember-headless-table': patch
+---
+
+Prevent hard-to-debug issues that occur with incorrect column configs.
+One such way problems can occur is when the `key` property is duplicated
+for multiple column configs. 
+
+This is now eagerly prevented via dev-time Error. 
+All the column config validity checking code is removed in production builds
+via `@embroider/macros` `macroCondition(isDevelopingApp())`.

ember-headless-table/package.json

@@ -93,6 +93,7 @@
     "@babel/runtime": "^7.17.8",
     "@ember/string": "^3.0.0",
     "@embroider/addon-shim": "^1.0.0",
+    "@embroider/macros": "1.10.0",
     "ember-modifier": "^3.2.7",
     "ember-resources": "^5.4.0",
     "ember-tracked-storage-polyfill": "^1.0.0",

ember-headless-table/src/-private/table.ts

@@ -4,6 +4,7 @@ import { assert } from '@ember/debug';
 import { action } from '@ember/object';
 import { guidFor } from '@ember/object/internals';
 
+import { isDevelopingApp, macroCondition } from '@embroider/macros';
 import { modifier } from 'ember-modifier';
 import { Resource } from 'ember-resources/core';
 import { map } from 'ember-resources/util/map';
@@ -52,24 +53,28 @@ export class Table<DataType = unknown> extends Resource<Signature<DataType>> {
 
   /**
    * @private
+   *
+   * Unused for now, may be used in the future.
+   * This data is collected along with the scrollContainerWidth, (which is currently in use)
    */
   @tracked scrollContainerHeight?: number;
 
   /**
    * @private
+   *
+   * Used to help determine how much space we can give to columns.
+   * As we generate widths for columns, the columns' widths must
+   * add up to about this number.
    */
   @tracked scrollContainerWidth?: number;
 
   /**
    * @private
+   *
+   * Lazy way to delay consuming arguments until they are needed.
    */
   @tracked declare args: { named: Signature<DataType>['Named'] };
 
-  /**
-   * @private
-   */
-  defaultColumnConfig = DEFAULT_COLUMN_CONFIG;
-
   /**
    * @private
    */
@@ -229,10 +234,33 @@ export class Table<DataType = unknown> extends Resource<Signature<DataType>> {
 
       if (!configFn) return [];
 
-      return configFn() ?? [];
+      let result = configFn() ?? [];
+
+      if (macroCondition(isDevelopingApp())) {
+        /**
+         * Assertions for a column config to be valid:
+         * - every key must be unique
+         */
+        let keys = new Set();
+        let allKeys = result.map((columnConfig) => columnConfig.key);
+
+        result.forEach((columnConfig) => {
+          if (keys.has(columnConfig.key)) {
+            throw new Error(
+              `Every column key in the table's column config must be unique. ` +
+                `Found duplicate entry: ${columnConfig.key}. ` +
+                `All keys used: ${allKeys}`
+            );
+          }
+
+          keys.add(columnConfig.key);
+        });
+      }
+
+      return result;
     },
     map: (config) => {
-      return new Column<DataType>(this, { ...this.defaultColumnConfig, ...config });
+      return new Column<DataType>(this, { ...DEFAULT_COLUMN_CONFIG, ...config });
     },
   });
 

ember-headless-table/types/index.d.ts

@@ -1,3 +1,2 @@
 // Installs the types for TS, no-op in JS
 import 'ember-cached-decorator-polyfill';
-

test-app/tests/unit/table-test.ts

@@ -75,4 +75,25 @@ module('Unit | -private | table', function (hooks) {
       assert.strictEqual(table.columns[position]?.key, key);
     });
   });
+
+  test('columns: each key must be unique', async function (assert) {
+    const table = headlessTable(this, {
+      columns: () =>
+        [
+          { key: 'firstName', name: 'First name' },
+          { key: 'role', name: 'Role' },
+          { key: 'favouritePet', name: 'Favourite Pet' },
+          { key: 'firstName', name: 'Last name (typo)' },
+        ] as ColumnConfig[],
+      data: () => [],
+    });
+
+    assert.throws(
+      () => {
+        table.columns.values();
+      },
+      /Every column key in the table's column config must be unique. Found duplicate entry: firstName/,
+      'expected error received'
+    );
+  });
 });