omniscientjs / immstruct

Immutable data structures with history for top-to-bottom properties in component based libraries like React. Based on Immutable.js
374 stars 21 forks source link

`handleSwap` and `handlePersisting` incorrectly use previous state. #45

Closed dashed closed 9 years ago

dashed commented 9 years ago

handleSwap and handlePersisting incorrectly use previous state

This is bad when a stale cursor updates, and a stale "previous" state (e.g oldData) is passed instead of the actual previous state.

I'll eventually PR.

Can be fixed by doing something like this:

// Map changes to update events (delete/change/add).
function handlePersisting (emitter, fn) {
  return function (newData, oldData, path) {
    var previous = emitter.current;
    var newStructure = fn.apply(fn, arguments);
    if(newData === previous) return newStructure;
    var info = analyze(newData, previous, path);

    if (info.eventName) {
      emitter.emit.apply(emitter, [info.eventName].concat(info.arguments));
    }
    return newStructure;
  };
}

// ...

// Emit swap event on values are swapped
function handleSwap (emitter, fn) {
  return function (newData, oldData, keyPath) {
    var previous = emitter.current;
    var newStructure = fn.apply(fn, arguments);
    if(newData === previous) return newStructure;

    emitter.emit('swap', newStructure, previous, keyPath);
    possiblyEmitAnimationFrameEvent(emitter, newStructure, previous, keyPath);

    return newStructure;
  };
}
mikaelbr commented 9 years ago

@Dashed : This is fixed, no?

dashed commented 9 years ago

@mikaelbr I just glanced at master. It isn't fixed yet.


To elaborate, the issue is that when stale cursors update, then assert(emitter.current !== oldRoot). oldRoot should be ignored; and it's incorrect behaviour to be passing that as "previous state" to any observers.

emitter.current should be the "previous state".

I've been busy with other stuff and I haven't been able to get around to PR'ing or working on immstruct/cursor stuff. The fix is easy, but just a test case for it.

I don't rely on this feature, so this bug is low priority.

mikaelbr commented 9 years ago

Did you have a failing test-case for this?

dashed commented 9 years ago

@mikaelbr don't have an exact test in mind. But I think one can create a stale cursor, and update it to see that emitter.current isn't being passed to listeners.