gregnb / mui-datatables

Datatables for React using Material-UI
MIT License
2.7k stars 931 forks source link

"Deprecated: Passing objects in as data is not supported, and will be prevented in a future release." #1040

Open dandrei opened 4 years ago

dandrei commented 4 years ago

Just upgraded to version 2.12.4 and I get the following:

Deprecated: Passing objects in as data is not supported, and will be prevented in a future release. Consider using ids in your data and linking it to external objects if you want to access object data from custom render functions.

I am currently using this option as a work-around in order to access the original object from the rendering function, as described in my reply to issue 475.

Other recently opened issues (like 1038) are also asking for a way to access the original row object.

Is this feature being considered for inclusion in the future?

Thanks.

RyanEwen commented 4 years ago

I really hope this becomes a supported case because this library seems really nice. I am playing with material-table, react-table, and mui-datatables.

mui-datatables appeals to me the most in terms of the features that are enabled out of the box and the overall presentation. But I might have to go with one of the others :(

gabrielliwerant commented 4 years ago

As discussed here https://github.com/gregnb/mui-datatables/issues/1011, the table has never officially supported passing object data for table cells, and doing so leads to a number of errors if not handled precisely in custom renders. If you're using a custom render, you're stepping into custom territory, and there's not any strong case I can see for accepting objects for this case. As explained in the issue, you can map something like ids back to arbitrary data/objects. They don't need to be passed into the table to get you the information you need.

gabrielliwerant commented 4 years ago

Closing this, as there is no planned support for passing arbitrary data structures into the table. As discussed in https://github.com/gregnb/mui-datatables/issues/1038, I would accept something that added dataIndex to the customBodyRender callback which would help alleviate some of these issues.

nathancharles commented 4 years ago

Since #1011 is closed, leaving a comment here. I agree that supporting objects for cell data would really create a more flexible and powerful framework.

It is not uncommon for a table cell to include more than just a number or string. For example, having a user avatar that has an image url, name and email is a pretty common use-case. Conceptually, and practically, it makes sense to have this data in one place, instead of some placeholder that will need to be mapped back. Mapping to some external data can be very clunky to implement. It makes sense that a user will want to be able to search any visible data in the table too, but this puts limitations on searching and require accessing an external data source to search the table.

Not having to re-map and restructure the data you need for every render can give some performance benefits as well. If there is some expensive operation, having to only do it once while constructing the table data is much less overhead rather than managing this on every cell re-render.

The concern that the render/search/filter methods could break doesn't seem very reasonable. All JS objects have a toString method that could be used to avoid breaking. Sure, you will get a poor display, or poor search/filter functionality, but as you said, you're stepping into custom territory and will have to implement more custom logic to support this (cell renderer, recursive search, etc). I'm sure there may even be more elegant solutions to this, but this is extremely low effort.

RyanEwen commented 4 years ago

Very relevant to https://github.com/gregnb/mui-datatables/issues/1038

jfederer commented 4 years ago

Perhaps people wouldn't be so disheartened by this backslide in capability if it was clear how to 'map it back'?

For example, I build my 'data' object for the table like so:

let data = currentUserEvents.map((event) => {
    if (event) {
    return [
        event.eventID,
        event.eventName,
        getQuestionValue(event.eventID, "sampleDate")
        getQuestionValue(event.eventID, "stationName") ? getQuestionValue(event.eventID, "stationName") : "N/A",
        event.shippedStatus,
        <Button onClick={() => {   //FIXME: passing objects is Depricated.  Will need to fix
            this.setState({ toEventSummary: true, SummaryEventID: event.eventID })
        }}>View Event Summary</Button>,
    ]
})

You'll notice that in the last column, I'd like to render a materialUI Button component that looks like a normal button and does something (something basic, but whatever) when clicked.

How would one re-do this in the 'proper' way?

gabrielliwerant commented 4 years ago

@jfederer The proper way to add a button to a column is to use customBodyRender. See custom-action-columns for an example.

keyvanm commented 4 years ago

Most data passed to the table are grabbed from an API in a json format, which is almost always an array of serialized objects. By removing support for passing objects as data, an overhead is added to the developers so now they need to start modifying their API data for this use case. If the feature is already there and working well, why remove it?

omacranger commented 4 years ago

Agreeing with the above individuals that passing the value of the current index from the data prop at the top level table into customBodyRender would be a great way to give us access to data without having to do filter / find / parsing indexes ourselves - especially since the table is already accessing that data. Example below of use case. posts array below is the same data that's being passed into the data attribute on the table itself.

Current method:

customBodyRender: (value, {rowIndex}) => {
    const post = posts[rowIndex];

    return (
        <Button
            component={Link}
            variant="outlined"
            size="small"
            to={{
                pathname: `/posts/${post.id}`,
            }}
        >
            View Post
        </Button>
    )
},

Preferred method:

customBodyRender: (value, {actualFullRowData}) => (
    <Button
        component={Link}
        variant="outlined"
        size="small"
        to={{
            pathname: `/posts/${actualFullRowData.id}`,
        }}
    >
        View Post
    </Button>
),  
keyvanm commented 4 years ago

@omacranger Be careful with using rowIndex to access the full row data. It breaks when you sort. As @gabrielliwerant said accessing original row data this way requires a dataIndex parameter which I believe is not implemented yet.

The best way to remedy this right now is to do a search for your row data using an id or a key, which is an expensive operation. The search operation being expensive is one of the reasons I believe having direct access to the original data is the best way to go about it.

omacranger commented 4 years ago

@keyvanm Ah, good call. I have a bunch of those options disabled by default right now (download, filter, sort) but I'll refactor that to handle it better. Appreciate the heads heads up.

gabrielliwerant commented 4 years ago

@keyvanm It's not an expensive operation if you normalize your data. Then it's a constant-time lookup for the id.

ToddHerron commented 4 years ago

I'm a React / Redux / Firebase / Firestore / MUI-DataTables newbie ...

My problem is that I need to sync Firestore database collection ('tasks') with a MUI-DataTable via Redux. The Firestore database stores timestamps as objects but I needed to format the dates to the users using toDate().

Here's the solution I came up with:


 // Create the mutable deep copy of the array when the data is available
 let tableData = tasks ? 
                            Array.from(Object.entries(Object.values(tasks)).map((item) => item[1])) 
                           : null

  // On render & re-render, ensure that the timestamp is a string.
    if (tasks) {
        tableData = tableData.map((rowItem) => {
            rowItem.createdAt =
                    typeof rowItem.createdAt === 'object' ? dayjs(rowItem.createdAt.toDate()).format(
                        'ddd MMM DD [at] h:mm a'
                    ) :
                    rowItem.createdAt
            return rowItem
        })
    }

The fix was to create a mutable deep copy (Array.from) of the array prop being synched with the Firestore collection and to the check on the initial component render and subsequent renders whether or not the timestamp had been converted from a type of object to string yet in the tableData array.

I thought I'd share this because it took me a while to figure it out. :)

roblangridge commented 4 years ago

If customBodyRender could include a dataIndex in the callback i believe that would solve a lot of use cases without needing to pass through the full data object.

For anyone who struggles with this, what i did was when generating the table data, i would always include the "data index" as the very last column. I wouldn't include it in the columns definition so it would never be show.

Then it's a case of the following in the customBodyRender callback

const dataIndex = tableMeta.rowData[tableMeta.rowData.length - 1];

Arkad82x commented 4 years ago

First of all, I love your datatable! But I have to admit I stumbled across the same issue of not having access to the rowData as object, which would be really convinient.

I solved that by wrapping the MUIDatatable component and adding an invisible column containing the id. Then I'm overriding the customBodyRender method of all columns that have one to add the rowDataAsObject as an additional paramter. This breaks if the objects don't have an id or an id column is added manually at any column other than the first. I'm not quite sure how bad this is in terms of performance, but maybe it helps someone.

const MUIDataTableWithObjects = ({ data, columns, ...props }) => {
    const dataMap = data.reduce((res, d) => (
        {...res, [d.id]: d}
    ), {});

    let columnsOverride = columns.map(column => ({
        ...column,
        options: {
            ...column.options,
            customBodyRender: column.options.customBodyRender ? (value, tableMeta, setValue) => {
                return column.options.customBodyRender(value, tableMeta, setValue, dataMap[tableMeta.rowData[0]]);
            }: undefined
        }
    }));

    if((!columnsOverride[0].name && columnsOverride[0] !== "id") || (columnsOverride[0].name && columnsOverride[0].name !== "id")) {
        columnsOverride = [{
            name: "id",
            options: {
                display: 'false'
            }
        }, ...columnsOverride];
    }

    return (
        <MUIDataTable
            columns={columnsOverride}
            data={data}
            {...props}
        />
    );
};

export default MUIDataTableWithObjects;
patorjk commented 4 years ago

As an FYI, I've taken on maintainer responsibilities for this project. I've dug into this issue. It stems from this https://github.com/gregnb/mui-datatables/issues/915 issue, which is pretty easy to resolve (though though there are other areas that also need to be touched up to ensure full object support). I think the ability to pass objects is something this library should support, and it clearly is something people find useful. In the next release, this coming Sunday, this restriction will be removed.

I'll also be introducing a customBodyRenderLite method which will be passed the dataIndex. This method will have better performance over the customBodyRender, though if you're interested in the details you can check out the release notes when they go up.

FernandoGOT commented 4 years ago

Hi, I'm testing this lib to replace my current one and so far it's very good.

I want to ask if you plan to return the full object of data, and not only an array with some of the value? Something like omacranger suggested, this is going to turn the data manipulation much easier and fast (to code)

patorjk commented 4 years ago

Hi, I'm testing this lib to replace my current one and so far it's very good.

I want to ask if you plan to return the full object of data, and not only an array with some of the value? Something like omacranger suggested, this is going to turn the data manipulation much easier and fast (to code)

What he suggested is now possible with the customBodyRenderLite method, which takes in the dataIndex.

I’m not sure it’d be worth it to pass the data too, though I could consider it. The main problem would be that the table currently doesn’t internally store it after it transforms the data, so it would have to be attached somewhere.

FernandoGOT commented 4 years ago

This was going to be a big help, since if I could access the object from data, I can get some props that isn't displayed in the table.

About the fact of the table storing the array, is it really necessary? I'm asking this because I'm passing the array to the table, can't we simply access this array?

Something like array[dataIndex] and return it in the tableMeta from customBodyRender or in a new prop like row. I still need to see the code, but I think that this way you don't need to save the array, just access the one that I pass from data, or you want to mount an object in the case of the data be an array of arrays?

patorjk commented 4 years ago

About the fact of the table storing the array, is it really necessary? I'm asking this because I'm passing the array to the table, can't we simply access this array?

Yes, the table transforms/normalizes the data it receives:

https://github.com/gregnb/mui-datatables/blob/master/src/MUIDataTable.js#L649

Something like array[dataIndex] and return it in the tableMeta from customBodyRender or in a new prop like row. I still need to see the code, but I think that this way you don't need to save the array, just access the one that I pass from data, or you want to mount an object in the case of the data be an array of arrays?

I'm a little confused why this is needed. customBodyRender gives you access to the row of transformed data via tableMeta.rowData (which is array[dataIndex] of the internal array). And if you want access to your raw data instead, customBodyRenderLite gives you the dataIndex. So you could you do something like this:

let columns = [{
    name: 'Col 1',
    options: {
        customBodyRenderLite: (dataIndex) => {
            return data[dataIndex].someValue;
        }
    }
}];

let data = [your data];
let options = {};

return (
    <MUIDataTable options={options} columns={columns} data={data} />
);
FernandoGOT commented 4 years ago

Yes, but this way I need to update my columns every time I update my data array, since the data[dataIndex].someValue is referring to the old variable, doesn't it cause unnecessary renders?

patorjk commented 4 years ago

It does not. If you update your data array, I'm assuming you want the data in the table updated too, and thus a re-render would occur anyway (the data is a prop to the table, if it changes, the table re-renders). If your data is staying the same but changing because of a parent re-render (and you've defined your data as a local variable), you can avoid a re-render by putting the data on the state.

If you define your columns and data in the same scope. You should be fine. If you're defining your columns in an external file, you could do something like this:

function getColumns(data) {
    return [
        {
            name: "Col",
            options: { 
                customBodyRenderLite: (dataIndex) => (data[dataIndex][0])
            }
        }
    ];
}

...

let columns = getColumns(data);

Additionally, the external data matches the internal data with only 2 exceptions:

FernandoGOT commented 4 years ago

Thanks for the reply, I'm testing things out here, and I'm going like this for now. I'm currently using this lib to show the data and use some of its features, I'm using a custom toolbar that I had here before.

I'd like to say that the onTableChange function as it is fantastic and helps a lot more than having all separated.

Just some suggestions:

  1. A way to block a column so that I can't change its order, this away I can make that column be always there, like the actions column be always the first (I can do it manually now, using the order array)
  2. The order array can be an array of the column index (numbers) or the column name (when data is an array of objects), this is going to make this much more intuitive to use
patorjk commented 4 years ago
  1. You mean like a fixed column? I think something like that could be useful. Alternatively, a "droppable" option could be added that would just mean columns that are dragged can't replace that column (and it would function the way you describe). I'd need this to think a little more about this though.
  2. I could see it being useful, though it could be done with a helper function that converted the index array to a name array.
FernandoGOT commented 4 years ago
  1. Yes, this is going to be useful, in some libs you can set a column to fixed on the left or right, but they are always the first or the last columns

  2. Yes, now that you told about it I'm going to make one here XD, but I truly think that this is going to turn this more intuitive for people starting with this type of lib