Warn if id in rows is missing

[arheinjohnson]

I'm simply trying to use the onRowClick param and it doesn't seem to work, and when I look through the source code, it's not clear that this has been implemented?

<DataGrid
rows={rows}
columns={columns}
autoHeight
sortModel={sortModel}
pageSize={20}
onRowClick={() => console.log("Hello, World!")}
// disableSelectionOnClick={true}
/>

[arheinjohnson]

Fyi, the issue was that my ColDef did not include an 'id' field, nor did my rows. Once including those, onRowClick now works.

[oliviertassinari]

@arheinjohnson How did you figure the origin of the issue?

I think that there is an opportunity to improve the DX here. I think that we should either warn if the row id is missing or remove this requirement with a fallback on the index of the row. The latter won't work when performing server-side sorting or filtering.

@dtassone What do you think about keeping the ID as a requirement but console.error when a row doesn't have it?

Regarding the column, this sounds like a bug. I don't see any practical reason for forcing the definition of a column for the ID. We can open a new issue for this one. @dtassone do you confirm?

[dtassone]

The ID column is used internally to optimize rendering and performance. ATM if it is not provided, it creates issues.
I think we can fix it internally by providing a fallback id in case the user does not pass an id column

[oliviertassinari]

Regarding the column id, it seems to work without https://codesandbox.io/s/red-sun-49nyg?file=/demo.tsx. So I have opened #346 to simplify the docs where possible.

[oliviertassinari]

ID column

@dtassone Are we talking about the same thing? I'm making a reference to the entry in the columns array prop. Are you talking about the id the grid build for each column? I assume right now, it uses the field value to build its internal column id?

[oliviertassinari]

I have renamed the issue to focus on this problem: https://codesandbox.io/s/romantic-kilby-wl9cc?file=/demo.tsx. If you don't provide rows with an id. It fails everywhere and you have no tips around what's going wrong:

I think that we developers should expect a first error message with how the id property on each row is required.

[arheinjohnson]

I wouldn't say I found the origin of the issue but while reading through the docs found the reference to a rows id. Agreed you don't need an id column for this to work and I really only needed an id in the rows. Would definitely be helpful to get that in a message if id isn't entered by the developer into the rows.

[scollins33]

The ID column is used internally to optimize rendering and performance. ATM if it is not provided, it creates issues.
I think we can fix it internally by providing a fallback id in case the user does not pass an id column

@dtassone - sorry in advance if this is a time waster (I promise I looked through the docs twice).

My dataset does have a unique id property but it is not called "id". I don't want to map through and just add one as that is taxing. Am I misunderstanding what you're talking about or is there a prop I can pass to target a specific "unique id property"?

[dtassone]

@scollins33 ATM there is no property to let you map id with another field.

[oliviertassinari]

I don't want to map through and just add one as that is taxing.

@scollins33 What do you mean by "taxing"? From a developer experience or runtime perspective?

[scollins33]

@dtassone - Ok, thanks for the clarity. Would that be a potential feature request? I can write up an Issue if you guys want me to.

@oliviertassinari - runtime perspective. I'm working with our backend team to see if we can get our columns renamed in a more consistent way (just call it "id"!!) so I can avoid having a loop anywhere.

Thanks for the time guys :) really appreciate the work yall do

[dtassone]

@scollins33 yes it is a potential feature request, please feel free to open an issue. We will pick it up as soon as we can.
It shouldn't be a big one ;)