Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dandrei opened this issue Oct 30, 2019 · 25 comments

Comments

@dandrei
Copy link

dandrei commented Oct 30, 2019

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
Copy link

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
Copy link
Collaborator

As discussed here #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
Copy link
Collaborator

Closing this, as there is no planned support for passing arbitrary data structures into the table. As discussed in #1038, I would accept something that added dataIndex to the customBodyRender callback which would help alleviate some of these issues.

@nathancharles
Copy link
Contributor

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
Copy link

Very relevant to #1038

@jfederer
Copy link

jfederer commented Dec 2, 2019

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
Copy link
Collaborator

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

@keyvanm
Copy link

keyvanm commented Dec 26, 2019

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
Copy link

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
Copy link

keyvanm commented Dec 27, 2019

@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
Copy link

@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
Copy link
Collaborator

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

@ToddHerron
Copy link

ToddHerron commented Jan 15, 2020

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
Copy link

roblangridge commented Apr 22, 2020

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
Copy link

Arkad82x commented Apr 28, 2020

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
Copy link
Collaborator

patorjk commented Jun 16, 2020

As an FYI, I've taken on maintainer responsibilities for this project. I've dug into this issue. It stems from this #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.

@patorjk patorjk removed the wontfix Support for this request is not planned at this time label Jun 16, 2020
@FernandoGOT
Copy link

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
Copy link
Collaborator

patorjk commented Jul 7, 2020

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
Copy link

FernandoGOT commented Jul 7, 2020

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
Copy link
Collaborator

patorjk commented Jul 8, 2020

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
Copy link

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
Copy link
Collaborator

patorjk commented Jul 8, 2020

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:

  • If you used the Delete Row icon on the Select Toolbar to remove data. (though you can keep things consistent by using the onRowsDelete callback)
  • If you use the updateValue method in customBodyRender. (but if you choose to do this, you can update your external data anyway).

@FernandoGOT
Copy link

FernandoGOT commented Jul 8, 2020

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
Copy link
Collaborator

patorjk commented Jul 9, 2020

  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
Copy link

  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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests