Fix TypeScript type for Column render function

[sjdemartini]

Related Issue

n/a

Description

This avoids Parameter 'rowData' implicitly has an 'any' type. TypeScript errors which were caused by the previous function union (rather than intersection), where no valid render function could be supplied by users of the MaterialTable component.

This problem and solution are described at length here: https://stackoverflow.com/a/58632009/4543977

Example

For instance:

  type RenderOriginal =
    | ((data: RowData, type: "row") => React.ReactNode) // The normal render of the table cell
    | ((data: string, type: "group") => React.ReactNode); // The render for the group wording
  const renderOrig: RenderOriginal = (rowData, renderType) => {
    if (typeof rowData === "string") {
      return rowData;
    }
    return <div>{rowData.myField}</div>;
  };

results in Parameter 'rowData' implicitly has an 'any' type.ts error and Parameter 'type' implicitly has an 'any' type..

Whereas the following does not have those issues:

  type RenderFixed = ((data: RowData, type: "row") => React.ReactNode) & // The normal render of the table cell
    ((data: string, type: "group") => React.ReactNode); // The render for the group wording
  const renderFixed: RenderFixed = (rowData, renderType) => {
    if (typeof rowData === "string") {
      return rowData;
    }
    return <div>{rowData.myField}</div>;
  };

A union isn't appropriate since we need render to be able to handle both versions of the function, not one or the other (so & rather than |). Note that it would be nice if the parameters were more constrained to be tied to one another so that if renderType==="row" then TS would infer that rowData is of type RowData, but it seems that's not possible in TS based on the above linked Stack Overflow explanation and my own testing (without using the variable arguments approach, which doesn't seem appropriate). But at least this approach works and doesn't result in a TS error no matter what you do for your render method.

Related PRs

n/a

Impacted Areas in Application

Fixes the main columns prop when using a custom render function, when using TypeScript.

Additional Notes

n/a

[Domino987]

Yes this looks better, was was working on it and am trying to get the correct type narrowing. It would be possible by using an object like: render: ({data, type}) => type === "row" ? "string": "object", but this would increase the friction of the changes. What do you think? @sjdemartini

[sjdemartini]

Thanks for discussing @Domino987! I'd be open to the object-based argument and wouldn't mind it being a breaking change, personally. Either way works for me!

---

By the way, I believe there's a somewhat similar typing issue for customSort, which currently is typed as:

  customSort?: (
    data1: RowData,
    data2: RowData,
    type: 'row' | 'group',
    sortDirection?: 'desc' | 'asc'
  ) => number;

However, when type === "group", it's not actually RowData for data1, but instead is RowData[field] where field is the field name key you specified for the column—i.e., it is the value for that field rather than the entire row data. That seems difficult to specify types for, and makes it harder and more verbose to define that function since both conditions have to be handled, so I wonder whether it could be updated to just pass RowData regardless of type.

[Domino987]

Yes that sounds like a reasonable thing, since the current implementation is years old from before the merge, its time to update it and to get more concise and stable params, will change it. But ill keep it as two params, since we just move it to be an object for the first and no narrowing is needed.

[Domino987]

@sjdemartini There is one more question, which data object should be passed to the grouped row render function, the first one?

[sjdemartini]

which data object should be passed to the grouped row render function, the first one?

Hm, it seems to me like it would always be easier to get RowData rather than a string, value, etc, since you can manipulate it however you see fit (whereas if you don't have access to the row data, you're limited in what you can render).

[Domino987]

Yes I am aware but which rowdata would you expect. Which item from data should be passed

[sjdemartini]

Ah, I see. I don't personally use the grouping functionality, so I'm not very familiar with the nuances/use-case. I guess when type==='group', the render function is being called for what the group "header" should be (as opposed to an underlying row within the group), which is why it wasn't passing an individual row's data? In that case, it seems there'd need to be some more complex types or use of generics to specify what the type of the value would be, but perhaps it should instead just use any, like:

{
  data: RowData;
  type: 'row';
} | {
  data: any;
  type: 'group';
}

but I'll defer to you. Passing a particular row's data does feel a bit strange/arbitrary in that case. I don't have any strong opinion about the grouping functionality, just want to make sure these functions can more easily be written and be valid in TS.

Maybe another option would be to have render for rows, and a separate renderGroup function for groups? The render function today necessarily has to have different conditional functionality if it's going to be passed different arguments/values, so having a single function doesn't seem like a big benefit.