Griddle
Griddle copied to clipboard
ColumnDefinition customComponent receiving immutable value prop
Griddle version
1.9.0
Expected Behavior
Griddle passes the same data it was provided to custom row components
Actual Behavior
Custom column component receives mutated value prop
Steps to reproduce
const data = [
{ id: 2, ts: XDate(376437856763), count: 3 },
{ id: 3, ts: XDate(764937032355), count: 7 },
]
const Timestamp = ({ value }) => <div>{value.toDateString()}</div>
const List = () => (<Griddle data={data}>
<RowDefinition>
<ColumnDefinition id="ts" customComponent={Timestamp} />
<ColumnDefinition id="count" />
</RowDefinition>
</Griddle>)
Pull request with failing test or storybook story with issue
WIll try and add one
The above (shortened) example results in an error where toDateString
is not defined. The actual value
prop passed to <Timestamp/>
appears to be an immutable Map wrapping a native date. And the only way to print a date is to use value.toJS()[0].toString()
.
This appears to be an issue with the way immutable is handling XDate
's although I know immutable can handle XDate
's because in our app we convert all fetched BSON $date objects to XDate's in our api logic and then store that after converting with fromJS()
. When a state slice is converted toJS()
the dates are still wrapped XDate's with all methods intact.
Either way this seems to be a bug with Griddle
hi @bristoljon, fyi current version of griddle is 1.10.1, but for some reason in npm is the older version 1.9.0. it was not very pleasant for me, because one bug I struggled with was solved in version 1.10.1 and I wasted time on it.
Hi @radziksh, thanks for the advice. Where did you find 1.10.1
? Latest release on github is 1.8.0
and npm latest is 1.9.0
which is kind of weird in itself, normally it's the npm version that's out of date!
Hi, @bristoljon - I find it into package.json file on github https://github.com/GriddleGriddle/Griddle/blob/master/package.json
or you can see it via command:
npm show griddle-react versions --json
result:
...
"1.6.0",
"1.8.0",
"1.8.1",
"1.9.0",
"1.10.0",
"1.10.1" <- last one
Upgrading to 1.10.1
didn't fix the issue but I have read the docs and believe now this might not be a bug:
From http://griddlegriddle.github.io/Griddle/docs/api/:
Griddle has a data prop available. The data prop is the array of data that Griddle should render. This data should be an array of JSON objects or a class.
Unsure what is meant by class here but array of JSON objects
implies that instances of other classes are not allowed which would explain why they are being lost before getting passed to the customComponent
.
Still it seems this limitation shouldn't be necessary though if I'm able to store and retrieve XDate's from my immutable redux store in my app. Will try and explore the code to see what the difference is in meantime.
I've found it in dataUtils.js
there is a customised implementation of fromJS
- fromJSGreedy
which excludes Date instances from conversion but an XDate (or any other instance) would just be converted as an object to a Map:
function isImmutableConvertibleValue(value) {
return typeof value !== 'object' || value === null || value instanceof Date;
}
// From https://github.com/facebook/immutable-js/wiki/Converting-from-JS-objects#custom-conversion
function fromJSGreedy(js) {
return isImmutableConvertibleValue(js) ? js :
Array.isArray(js) ?
Immutable.Seq(js).map(fromJSGreedy).toList() :
Immutable.Seq(js).map(fromJSGreedy).toMap();
}
I guess the default immutable fromJS
implementation doesn't convert XDate's so they come out as they went in. I basically just need to add || value instanceof XDate
to the end above but that would mean adding XDate as a dependency of griddle-react
..
Perhaps providing you're own fromJS
implementation could be added to the list of customisable features instead? Seems like it must be a fairly common use case or maybe it's literally just me messing around with XDate's..!
Perhaps providing you're own
fromJS
implementation could be added to the list of customisable features instead? Seems like it must be a fairly common use case or maybe it's literally just me messing around with XDate's..!
fromJS
isn't all that interesting, but isImmutableConvertibleValue
(with a better name?) seems like a great candidate for plugability, similar to how sortMethod
can be overridden.
At a glance, sortMethod
is the only util-type function that's currently accepted. We could continue accepting these as top-level props, or introduce a new prop (utils
?) to more explicitly document the available hooks.
The other option would be to make a smarter type
mechanism. Date
handling is currently hard-coded, but there would seem to be many advantages to making this a real extensibility point, e.g.
const types = [
{
// for <ColumnDefinition type="..." />
type: 'date',
// for type inference
match: value => value instanceof Date,
// for default printing; prop for customComponent
format: value => value.toLocaleDateString(),
},
{
type: 'xdate',
match: value => value instanceof XDate,
format: value => value.toDateString(),
},
{
type: 'xtime',
format: value => value.toTimeString(),
},
];
return (
<Griddle types={types} data={data}>
// ...
</Griddle>
);
Yeah both solutions would be helpful. From my perspective a method to over ride isImmutableConvertibleValue
would be the simplest but it seems like that's exposing an implementation detail to users that might not be familiar with immutable and shouldn't really need to care about it.
Although I guess renaming it something like isJSONCompatibleValue
and clarifying in the docs that any non-JSON values will be mutated unless this prop is provided could go some way to helping that. But I can't help but feel that this should JustWork™ and rowData
should be exactly what was passed in to data
by default.
I haven't looked at the implementation details too much so maybe this is a stupid question but what if data
was only shallowly converted to a List
so that Row containers can still benefit from reference equality checking but the data itself is still the same?