Fixes #10 (Now compatible with immutable library)

[leonaves]

Okay, so this is a bit of a bold move, but as I have mentioned here, I don't really see why there is a separate node for Iterable objects. Yes, this monitor does show the type of the object in the state, but I think that has limited value. I also think the amount of effort to get this to work with immutable properly would be mammoth (I got some way towards making the JSONDiff module work, when I realised that the whole "pin" feature is incompatible as well.

I think this solution (converting to pure JS) is the cleanest, and provides immediate support for the immutable library without a huge undertaking. If you want to go back and do it properly another time, fair enough, but for now I think this is the right way to go.

[alexkuz]

Thank you, first of all! I don't know though, converting whole Iterable object might be quite heavy (technically, it might be of infinite length). I have a PR here that deals with huge collections, so I really want this monitor to be efficient.

You're right about pin though - but I would rather just disable it in Iterable for now.

Speaking of Immutable - yes, maybe we should convert its Iterable objects to plain JS. But we shouldn't do it with any Iterable object (and definitely not call toJS() on it, as it's Immutable-specific method, I believe).
But, again, I think it's better to leave it as Immutable object and use .get() when we need it.

[leonaves]

Some good points; for one, I'm clearly conflating iterables (objects that have Symbol.iterator) and Iterables (https://facebook.github.io/immutable-js/docs/#/Iterable). With that point in mind, maybe there's a way we can use JSONIterableNode from react-json-tree in the JSONDiff module? I just submitted a PR (alexkuz/react-json-tree#36) that exports the getChildNodes function from that which might help in this situation? I don't think using .get() or .toJS() is the solution (I did toy with using .toObject() to convert as you expand for a more efficient conversion but I think that has the same problem). I think we should take advantage of the fact that it is an iterable somehow, as JSONIterableNode does, and iterate over it that way.

[alexkuz]

How about creating react-json-immutable-tree, that wraps react-json-tree and is aware of Immutable?

[leonaves]

What I would worry is, as you've made me realise, this PR is very immutable specific (with the toJS call), but react-json-tree manages to handle immutable objects without being specific to the immutable library, so it would be nice to manage to do that.

I've been looking over the source for react-json-tree a bit more and I think the issue with the pin feature is that react-json-tree doesn't handle key paths well for immutable objects. This might be easily fixable.

I think the harder problem is with the JSONDiff module, because it actually inserts into the object to track the diffs. Because immutable implements the iterable interface it's not too complicated to display the values of you consider it, but changing them is a bit tougher.

[leonaves]

Okay, so with regards to the pin feature, this is the basic problem. You use the key path feature you implemented here (alexkuz/react-json-tree#32) to navigate to the correct nested part of the state. However, if you get a key path for an iterable object (e.g. ["customers", 3, "name"]), you have no way of telling that this is an iterable, and so can only navigate to the path you want as if it were an object. Perhaps if labelRenderer also got the type? Or is this becoming a bit of a property passing nightmare? I feel it couldn't hurt to give labelRenderer the type, as getItemString gets passed it?

[leonaves]

The alternative is that react-json-tree becomes responsible for navigating to a particular path of an object, using the keyPath property passed in? Thoughts?

[alexkuz]

you have no way of telling that this is an iterable

Sorry, I don't follow, why is that? As I'm traversing through original state in getInspectedState, I can just check it's type and use .get() if it's an associative Immutable, right (or, if it's not, iterate until we get to the key)?

[leonaves]

Ah yeah fair enough, but it still seems like unnecessary repeated logic when react-json-tree already knows the type?

[leonaves]

Okay, take your pick:

https://gist.github.com/leonaves/b1d4ae5a7ef6d8c58c7a
https://gist.github.com/leonaves/b0def6d3ae8ae422b25d
https://gist.github.com/leonaves/08a8c368e0b86de24569

[alexkuz]

I would keep it simple, like this:

function isIterable(obj) { /* check for Symbol.iterator */ }

function iterateToKey(obj, key) { // maybe there's a better way, dunno
  let idx = 0;
  for (let val of obj) {
    if (idx === key) { return val; }
    idx++;
  }
}

...

if (Immutable.isAssociative(s)) {
  return s.get(key);
} else if (isIterable(s)) {
 return iterateToKey(s, key);
} else {
 return s && s[key];
}

[leonaves]

Ah yeah, isAssociative is a better check than isIterable, however, bear in mind that your version of "iterateToKey" will not support Map type iterables, such as the built in Map object (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map) — "a for...of loop returns an array of [key, value] for each iteration", which is the reason for this if statement in my function:

if (Array.isArray(entry)) {
    if (entry[0] === key) return entry[1];
} else {
    if (i > key) return; if (i === key) return entry;
} i++;

Also if (i > key) return; should increase efficiency in that case as you don't continue looping a numerically keyed iterable if you've gone past the key you want.

[leonaves]

Figured I'd close this as I'm not expecting it to be merged as is.