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

Cannot accept objects for cell data - Date #1011

Closed
wroughtec opened this issue Oct 17, 2019 · 17 comments · Fixed by #1013
Closed

Cannot accept objects for cell data - Date #1011

wroughtec opened this issue Oct 17, 2019 · 17 comments · Fixed by #1013

Comments

@wroughtec
Copy link

wroughtec commented Oct 17, 2019

Unfortunately the fix introduced in 2.12.2 to validate data is either a string or an int causes an issue with things like Dates which is seen as an object. Previous versions this was not an issue.

I can work around this issue by converting to a timestamp (to avoid borking sort) prior to hitting the view but either there should be a check if its an object and not a Date or document that raw dates can not be passed as I am sure this may catch a few people out

@gabrielliwerant
Copy link
Collaborator

Understood, will look into it.

@gabrielliwerant gabrielliwerant self-assigned this Oct 17, 2019
@gabrielliwerant
Copy link
Collaborator

So, when I use a date object as data in the table, I get the Uncaught Invariant Violation: Objects are not valid as a React child error, which is what the fix in 2.12.2 is preventing from occurring. Using date objects in the same way as other objects in the data should be giving you the same problems. So I'm actually not sure how you were able to do this before. Can you share the relevant bits of your code so I can understand how you were using it?

@gabrielliwerant gabrielliwerant added the needs verification For issues that can't be reproduced or are otherwise unclear label Oct 17, 2019
@wroughtec
Copy link
Author

Ok let me get up some examples that should 'hopefully' show how I have it working. I do wonder if its because I use the custom body render.

@wroughtec
Copy link
Author

wroughtec commented Oct 18, 2019

This shows how I have been doing it prior to 2.12.2 (have to confess never thought to do without a custom render as its something you would pretty much alway format):
https://tv8fd.csb.app/ - https://codesandbox.io/s/muidatatables-custom-toolbar-tv8fd

This is with 2.12.2:
https://70o59.csb.app/

Potentially (am thinking without looking at the code) the check should maybe only happen on those fields that do not have a custom render.

@drabelo
Copy link

drabelo commented Oct 18, 2019

Also getting this error ... Everything was working fine until today. I have a custom button as a cell and now it doesn't allow it. How do you fix that?

@wroughtec
Copy link
Author

do you have a demo? You may be able to use the customBodyRender

@lalong13
Copy link
Contributor

Also seeing this error. Using a moment object that can be formatted with customBodyRender.

@gabrielliwerant
Copy link
Collaborator

gabrielliwerant commented Oct 18, 2019

So, this type of functionality was never officially supported. Although on second glance at the docs, this isn't as clear is it could be. But only numbers, strings, and array data is accepted by the table.

It seems to work ok when you have a custom render function that knows just how to treat it, but without that, it breaks. The way to do this is to keep object data separate, if you have it, use an id of some kind in the actual table data, and then map and transform it how you wish in the custom render function.

@gabrielliwerant
Copy link
Collaborator

Here is a modification of your sandbox example to show what I mean (one possible way of avoiding objects in the table data but still getting the values you want): https://codesandbox.io/s/muidatatables-custom-toolbar-bsb81?fontsize=14

@alexcberk
Copy link

I have enough code relying on object data that 2.12.2 will break most of my tables. I'm using objects to create "a" tags in cells, add form elements, etc. @gabrielliwerant would you consider rolling this back or reserving this change for a minor or major version release?

@gabrielliwerant
Copy link
Collaborator

gabrielliwerant commented Oct 18, 2019

This is unfortunate because the table blows up if you don't handle your own object data. But it does seem like fixing that has potentially caused more issues than it solved at the moment. I will switch to a console error and make the documentation more clear.

Objects were never intended to be a supported data type for cell data, and does cause errors, so it will have to be prevented eventually. I recommend switching away from them when you can, but I will put it back for now.

@gabrielliwerant gabrielliwerant added invalid/out of scope bug and removed needs verification For issues that can't be reproduced or are otherwise unclear labels Oct 18, 2019
@gabrielliwerant
Copy link
Collaborator

Should be console error in 2.12.3.

@alexcberk
Copy link

Thanks, @gabrielliwerant. Understood, and we will start putting effort toward switching.

@aostrander-source
Copy link

Hi @gabrielliwerant , I notice that here, as in #1038 and #1040 , you consistently suggest passing an ID reference to the object as the recommended workaround. We ran into a related issue this morning and solved it by stringifying the object and having our customBodyRender start off by parsing it. Is there an issue you see with taking that route? Thanks

@patorjk
Copy link
Collaborator

patorjk commented Jun 16, 2020

@aostrander-source you can see my comment here - #1040 (comment), but in short, object support will be added this coming Sunday and the deprecation warning will be removed.

@aostrander-source
Copy link

@patorjk thanks for the reply. Given your last note on #1339 , it seems I will need to continue stringifying my object for sorting functionality (my schema and tests enforce having the property to be sorted by at the top of my object - hacky but I worked with what I had).

I could instead use a custom sort function as you suggest, but currently this is only an option at the table level. Implementing one function for the entire table that operates differently depending on the column index is overly complex when I only need custom functionality on the one column accepting this object. It also tightly couples functionality to column order, which is subject to change frequently/dynamically.

A hopefully simple solution would be a customSort column option. That would allow users in this situation to input a simpler function coupled with only that column, which makes sense for these objects and matches the pattern started by customBodyRender.

If you think ☝️ is a good idea, let me know and I can look into doing a PR.

@patorjk
Copy link
Collaborator

patorjk commented Jun 17, 2020

@aostrander-source I would be good with that as a PR. Someone did a similar idea in this PR: #951, however, it's out of date and Gabriel seemed to have issues with it so it was one of the few PRs I didn't really dive into when I did version 3.

However, it makes much more sense in light of this issue. As long as a columnSort didn't cause problems if customSort was set (I would expect the column sort to take precedence), I think we should be good to go.

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

Successfully merging a pull request may close this issue.

7 participants