Reordering items in list cursor will confuse updates on captured items.

[Gozala]

I believe following scenario will cause an issues that we have being observing in our application code:

Let's say this is more or less our root cursor model {items: [a, b, c]}:

1. Root component creates cursor for a c item.
2. Root component passes cursor for a c item down to a subcomponent.
3. Event causes order of items to be reversed (without causing component that c was passed to to be re-triggered).
4. Event in the subcomponent with captured c cursor makes an update on c.

Outcome of this steps is that a will be replaced with c causing us to have end up with {items: [c, b, c]}.

[Gozala]

I believe reason for this is that c._keyPath will be ['items', 2] when update on c is made it will hit this line: https://github.com/omniscientjs/immstruct/blob/master/src/structure.js#L117 where self.current.items is [c, b, a] and newRoot.items is [a, b, c] and path is ['items', 2] there for instead of performing update on the c update will be performed on a.

[Gozala]

In my understanding this could also cause issue when item isn't added to the tail of the list, but to the head or middle of the list.

[Gozala]

I think these are the lines causing the issue:
https://github.com/omniscientjs/omniscient/blob/master/shouldupdate.js#L159-L161

Here is a little demonstration of what I think is going on:

let root = Cursor.from(Immutable.fromJS({items: [{x: 3}, {x: 2}, {x: 1}]}))
let x1 = root.cursor('items').cursor(0);

let root2 = root.updateIn(['items'], items => items.reverse());
let x2 = root2.cursor('items').cursor(2);

x1.deref() === x2.deref() // => true
x1._keyPath === x2.__keyPath // => false

So what I believe is happening is Component rendering x1 first and then rendering x2 afterwards won't re-render since shouldComponentUpdate will return false. There for cursor with in the component will refer to x1 there for update on x1 will cause changes to be merged onto invalid __keypath and there for leading to the described issue.

I was really hoping that x1.equals(x2) would return false which would be another argument to delegate to .equals instead of doing custom comparison, but I'm afraid that's not the case and it also returns true.

[dashed]

I made a demo: http://jsbin.com/ramibe/3/edit?js,output
Unfortunately, I still don't see what the issue is.

This are the right invariants to make/assume:

x1.deref() === x2.deref() // => true
x1._keyPath === x2.__keyPath // => false

---

I added a "side effect" to fetch keys externally: http://jsbin.com/ramibe/4/edit?js,output

[Gozala]

Here I made an example to reproduce this issue:
http://jsbin.com/yunomateha/1/edit?js,output

Click "x" few times then "reverse" then "x" again, see what happens!

[dashed]

Okay. I definitely see it now.

Here's a fix: http://jsbin.com/bomoni/5/edit (translated to JSX for readability)

      <div>
      {items.toArray().map((item, idx) => {
        return <Item.jsx key={item.get('id')} _idx={idx} item={item} />
      })}
      </div>

---

The problem is that we should somehow mark a component as "dirty" for shouldComponentUpdate to know it should rerender. I'm unsure if this is something that omniscient can be solve internally.

Note that attempting to provide key value as a prop won't really work most of the time. Example: <Item.jsx key={item.get('id')} _idx={item.get('id')} item={item} /> Because there's no information that the component's "order" has change with respect to something external (e.g array).

[dashed]

Actually. You just need to add the order property to the key.

This works without manually passing an ambiguous prop:

      <div>
      {items.toArray().map((item, idx) => <Item.jsx key={item.get('id') + '.' + idx} item={item} />)}
      </div>

---

This is a non-issue with omniscient and/or immstruct.

[dashed]

I think this is what was happening:

If the key was just key={item.get('id'), React doesn't know about the ordering of the DOM element, and has accidentally marked the component to not be re-rendered. See: http://facebook.github.io/react/docs/reconciliation.html#keys

This is noticeable that if you turn on debug: component.debug();, there is no shouldComponentUpdate information indicating that the components were reversed after pressing x several times.

[Gozala]

I think this is what was happening:

If the key was just key={item.get('id'), React doesn't know about the ordering of the DOM element, and has accidentally marked the component to not be re-rendered. See: http://facebook.github.io/react/docs/reconciliation.html#keys

This is noticeable that if you turn on debug: component.debug();, there is no shouldComponentUpdate information indicating that the components were reversed after pressing x several times.

I don't agree. As pointed out the reason react decided to not re-render is because shouldComponentUpdate for the <Item key=x> was false which is because the way cursors are compared (see my earlier comments with links to actual lines). If isEqualCursor took __keyPath changes into account shouldComponentUpdate would have returned true and there for re-render would have being triggered.

Also note that this is not an issue with plain react as there this.props is updated regardless if re-render is triggered or not. There for this.props.cursor would point to right thing.

[Gozala]

As of encoding id and index as key that would have worked worked around this issue with specific case, but this is not the actual code that I have problems with it's distilled to bare minimum to illustrate issue (in my case items needs to be ordered by id in one part of UI and sorted by some other logic in other part of UI there for encoding index isn't really a solution that could be generalised).

[Gozala]

The problem is that we should somehow mark a component as "dirty" for shouldComponentUpdate to know it should rerender. I'm unsure if this is something that omniscient can be solve internally.

One way to solve that would be to update implementation of isEqualCursor to take relocation of cursors into account, for example changing it to something like code below would cause re-located nodes to be re-rendered:

function isEqualCursor (a, b) {
    return _unCursor(a) === _unCursor(b) &&
               a.__keyPath === b.__keyPath;
 }

Although that is still not an ideal solution (yet better than no solution) as component's render will be re-triggered in order for it to return equal DOM tree. As I already mentioned in a few different threads, react itself updates this.props regardless of what shouldComponentUpdate returned, that way even though DOM isn't generated this.props.... will be up to date when event listener is invoked.

Doing same in omniscient is little more tricky as this.props are likely captured in the render function context & there is no JS api to update enclosed variables. There for only other options I can think of are:

1. Create proxy between params passed-in and params given to render so that they could be updated to point to a new props without re-triggering render function. This is more or less what we end up doing for event handlers on statics in hot swap static functions without causing re-render #67 #68
2. Make cursors similar to ones you get in Om. To my understanding same thing happens to cursors in Om as what happens with this.props in react. Meaning .deref() value is swapped to reflect new inputs passed to a component regardless weather re-render was triggered or not.

Probably it would make sense to just do a simple fix to a isEqualCursor to resolve the issue by re-rendering and try to tackle more general solution for a next major release of omniscient.

[dashed]

Alright, I'm inclined to see that this may be feasible.

Here's a demo using your proposal: http://jsbin.com/veveme/7/edit?js,output

Using:

  function isEqualCursor (a, b) {
    var
    n = a._keyPath.length - 1,
    m = n >= 0 ? n : 0;
    return _unCursor(a) === _unCursor(b) &&
       n === b._keyPath.length &&
       a._keyPath[m] === b._keyPath[m];
  }

I didn't do full array comparison.

---

For stronger comparison, I'll consider hashing the keypaths every time a new cursor is produced at a new keypath for custom cursor types.

[Gozala]

I have added another version of the example illustrating that adding index to a key is not a general solution that going to work in all cases:

http://jsbin.com/mebewequsa/2/edit?js,output

[dashed]

Conceptual fix for #89 (comment) by manually passing item._keyPath as a prop: http://jsbin.com/tovipa/2/edit

[torgeir]

Do we want to go down the path of relying on cursor implementation details to make this work? Is the issue really that we are closing over props when creating event handlers inside render?

Edit: @Gozala how did your immutablejs issue of adding keypath to cursors' equals fan out?

[torgeir]

I definitely see that this would be an issue, but fixing the keypath feels like fixing the symptom, not the problem. Also, it probably doesnt belong in immutable.js equals, as it's comparing values, and two cursors' value ought to be equal even if they belong in separate structures. Don't really know how we'd go about fixing it though..

[torgeir]

..if we still suggest closing over cursors in render for event handlers. Moving handlers to members on the component would circumvent the issues, though I agree it's not as nice as just inlining them in render

const Item = component({
    onClick: function () {
      this.props.item.update(i => {
        return i.set('value', i.get('value') + 1)
      });
    }
  }, 
  function Item ({item}) {
    return <button key={item.get('name')} onClick={this.onClick}>
      {item.get('name')} - {item.get('value')}
    </button>;
});

const Items = component({
    onClick: function () {
      this.props.items.update(is => is.reverse())
    }
  },
  function Items ({items}) {
    const reverse = <button onClick={this.onClick}>Reverse</button>;
    return <div>
      {reverse}
      {items.toArray().map(item =>
       Item({ key: item.get('name'), item }))}
    </div>;
});

Edit: runnable example http://goo.gl/RckY2W

[torgeir]

I'm guessing this also ought to work

const Item = component(function Item ({item}) {
  const onClick = () => this.props.item.update(i => i.set('value', i.get('value') + 1));
  return <button key={item.get('name')} onClick={onClick}>
    {item.get('name')} - {item.get('value')}
  </button>;
});

Edit: Come to think of it, is this because props is a new object on each render? Otherwise you'd think at least this would work

const Item = component(function Item (props) {
  const onClick = () => props.item.update(i => i.set('value', i.get('value') + 1)); // no this
  ...

[Gozala]

..if we still suggest closing over cursors in render for event handlers. Moving handlers to members on the component would circumvent the issues, though I agree it's not as nice as just inlining them in render

Yes that will work, although I really would like to avoid depending on this.

[Gozala]

I'm guessing this also ought to work

const Item = component(function Item ({item}) {
const onClick = () => this.props.item.update(i => i.set('value', i.get('value') + 1));
return <button key={item.get('name')} onClick={onClick}>
{item.get('name')} - {item.get('value')}
;
});
Edit: Come to think of it, is this because props is a new object on each render? Otherwise you'd think at least this would work

const Item = component(function Item (props) {
const onClick = () => props.item.update(i => i.set('value', i.get('value') + 1)); // no this
...

This probably going to work but you will end up re-runnig Item per each item in the list which is what I was trying to avoid :(

[mikaelbr]

It might be possible to abstract away depending on this. Just a thought, but by having omniscient handle this instead (by some sort of decoration), the this interaction might be less bitter.

[Gozala]

I think proper solution for this problem would be to do whatever we end up implementing for event handler. Meaning that we do swap event handlers even if render has not run, I think something along these lines could be done for cursors as well.

[mikaelbr]

That's what I was thinking. Having a decoration for event handlers. We've been discussing this some back and forth, without having concluded with a one-fit-all solution, but we need a concrete, goto, way of doing event handlers. One solution might be decorators (just thinking of the top of my head now)

MyComponent({
  cursor: myCursor,
  onClick: eventHandler(({cursor}) => cursor.set('foo', 'bar'))
});

Where eventHandler decorator always pass the updated cursor by using this. Might not be the best idea, but something that might be possible.

[Gozala]

Disclamer: I think it is very likely that project I'm working on is going to drop cursors in favour of solution inspired by elm which is illustrated in this jsbin:
http://jsbin.com/kulujubobi/1/edit?js,console,output

There has being several reasons, I'm going to quote some of the relevant discussion below:

The reason we use immutable data structures for a several reasons:

1. To make it easy to reason about app state by avoiding out of sync situations, caused by mutations from one part of code that was not noticed by other part of the code.
2. Immutable data structures make it really easy to represent whole application state as a single data structure, and updates could be made deep down in the nested structure that would produce whole new structure as the root with common parts shared with previous. In other words making deep down updates very convenient.
3. By leveraging immutable data structures we take advantage of predefined & optimised shouldComponentUpdate to gain performance boost by not re-triggering components who's state has not changed, this is explained in further details by PureRenderMixin

Although since UI interaction need to cause state changes that somehow need to initiate re-render with another updated state, we employed Cursors that is part of Immutable.js library that allows to present sections of the actual immutable data structure such that transformations of it will notify a renderer of a transformed state so it could re-render with updated state. Unfortunately use of Cursors caused some ambiguity (for developers & the system):

1. It is not obvious when cursors should be used over plain data structures. Although attempt was made to make it more clear.
2. Cursors have a read+write semantics and when passed around it is not clear if they intended to be read from or to be written into.
3. Result of render purely depends on input meant for read anything write related does not need to re-render, but since cursors are read+write ones intended for write would cause re-renders as well. To avoid that we established event handler passing pattern which makes things worse by having two different ways to update.
4. Reordering items cursor points to does not affect cursors equality check that later causes issue that performs update on a structure on an out of date location. This is very likely to be fixed but even if it will it would cause re-render even if not really necessary because as pointed out earlier cursors have read+write semantics.
5. Attempts to do async rendering failed given that state captured by previous render is out of date until next render. This can be resolved by separating read / write concerns as I'll describe later.
6. Most times same functions can be used on both cursors and plain data structures, but outcome will be different. When applied to a immutable data structure new structure will be returned but when applied to cursor in addition re-renders will be initiated. Further more composed transformation functions will cause multiple re-renders there for we tend to do cursor.update(action) instead to trigger just a single update.

Now that problem has being explained (cursors complect read & write) I think the solution requires separating read & write inputs. Which can be achieved by:

1. Passing only immutable data structures (not cursors) for inputs that will affect output of the render.
2. Use different mechanism for sending updates to the renderrer.

I have prototyped & illustrated this idea in the above mentioned issues, that you can find here:
http://jsbin.com/rodanajeya/3/edit?js,console,output

In nutshell what is changed is that along with input additional parameter edit is passed to a component that can be invoked with a function that takes current state and returns new state like if you want to increment counter you'd do edit(state => state.set('counter', state.get('counter'))) or more simply edit(state => state.update('counter', inc)) which solves all the issues described above.

It is also really simple (as you can see in jsbin) to define helper functions that would create sub-edit functions to pass around scoped versions of edit to a subcomponents. So above counter update can be simplified further as:

const  editCounter = subedit(edit, 'counter')
// in other component
editCounter(inc)

There for all operations will be expressed as transformations over immutable data structures, in order to cause a re-render operation (transformation functions) will be just send to a renderer that will apply it to the current state to produce new state and re-render.

[torgeir]

Yes that will work, although I really would like to avoid depending on this.

Me too.

[torgeir]

Saw the elm-like approach from your immutable.js issue post, I was intrigued! I've always thought of the cursors of immutable.js as kind of noisy

[Gozala]

@torgeir I'm also considering use of CSP channels as an alternative to the illustrated solution, although I have not took time to evaluate pros and cons yet. Likely we'll start with what I've illustrated and maybe consider channels in parallel.

[torgeir]

The Renderer approach ought to work well with omniscient as of today btw http://goo.gl/CRJBst, my only concern atm is sub components need to much detail in creating edit-functions for their children down the hierarchy, on every level binding their inner workings to the data structure, even more than with cursors, but with some good helpers you might come a long way

Edit: changed the link, messed up the keys in the first example

[gilbox]

I know this thread is a couple months old, but I am fascinated by this approach. How has it been working out @Gozala ?