ColumnDefinition customComponent receiving immutable value prop

[bristoljon]

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

[radziksh]

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.

[bristoljon]

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!

[radziksh]

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

[bristoljon]

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.

[bristoljon]

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..!

[dahlbyk]

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>
);

[bristoljon]

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?