[DataGrid] Throw if rows id is missing

[dtassone]

Closes #342

[oliviertassinari]

I think that we should add a new test case, to no break this by mistake in the future, this is a new constraint.

[oliviertassinari]

Be aware of: https://ag-grid.com/javascript-grid-infinite-scrolling/#selection

Selection works on the rows in infinite scrolling by using the ID of the row node. If you do not provide IDs for the row nodes, the index of the row node will be used. Using the index of the row breaks down when (server-side) filtering or sorting, as these change the index of the rows. For this reason, if you do not provide your own IDs, then selection is cleared if sort or filter is changed.

If we go down the road of allowing rows without ID, there will be work later on. If you are happy with the implication, I'm happy too. I was leaning toward not supporting it until we hit a significant pushback (to keep things as simple as possible, to no make the same mistakes as ag-Grid, if its one) but no strong point of view.

[dtassone]

I know that's why I didn't support rows without id originally.
But javascript user can bypass the ts constraint, so I can throw an error if a row doesn't have an id or a warning and use the index. In the doc we can put a warning as they did.

[dtassone]

BTW this is just in javascript as in TS, the rows prop enforce an id column

[oliviertassinari]

BTW this is just in javascript as in TS, the rows prop enforce an id column

I don't understand. What do you mean?

[dtassone]

BTW this is just in javascript as in TS, the rows prop enforce an id column

I don't understand. What do you mean?

I mean that as TS enforce types and the rows react prop is of a type that requires an Id

[oliviertassinari]

@dtassone Ok, so if I understand it correctly. TypeScript forces an id. We are going in the direction to make an id required until we see a significant push back from the users.

Then, I don't understand the diff in the pull request. Why don't we throw if there is a row with a missing id and add a test case for it? The type definitions should match the implementation, no?

[dtassone]

@dtassone Ok, so if I understand it correctly. TypeScript forces an id. We are going in the direction to make an id required until we see a significant push back from the users.

Then, I don't understand the diff in the pull request. Why don't we throw if there is a row with a missing id and add a test case for it? The type definitions should match the implementation.

The type definition always matches the implementation as it's generated with TS.
But if you use Javascript you can pass whatever you want as there is no type checking.

[oliviertassinari]

For the test case, you can use https://github.com/mui-org/material-ui/blob/3ba31434e251fded9829c1cebfdfea5f5e86ce9e/packages/material-ui/src/styles/colorManipulator.test.js#L220 as an example.

[oliviertassinari]

I have tried to write an end-to-end test but it doesn't work with the current infra.

diff --git a/packages/grid/data-grid/src/DataGrid.test.tsx b/packages/grid/data-grid/src/DataGrid.test.tsx
index 15fa8ace..c13f60e8 100644
--- a/packages/grid/data-grid/src/DataGrid.test.tsx
+++ b/packages/grid/data-grid/src/DataGrid.test.tsx
@@ -83,6 +83,25 @@ describe('<DataGrid />', () => {
         expect(cell).to.have.text('Addidas');
       });
     });
+
+    it.only('should warn if the rows has no id', () => {
+      const rows = [
+        {
+          brand: 'Nike',
+        },
+      ];
+
+      expect(() => {
+        render(
+          <div style={{ width: 300, height: 300 }}>
+            {/* @ts-expect-error */}
+            <DataGrid rows={rows} columns={defaultProps.columns} />
+          </div>,
+        );
+      }).to.throw(
+        'Material-UI: The data grid component requires all rows to have a unique id property.',
+      );
+    });
   });

   describe('warnings', () => {

I don't understand why.

Now, there is a way around it. We could console.error instead of throwing an error. If displayed first, it would have the same effect. It would also allow to strip the message from production, helping with bundle size.