Patch handlePersisting(), and set correct structure during add, change, delete events.

[dashed]

This PR corrects the following issues:

• Modifying structure during add, change, or delete events doesn't work. It just reverts to the structure that fired the add, change, or delete events. This is an issue when updating value at a key path to anything but string, numbers and true.
• Deleting by key path (e.g Cursor.remove(key)) doesn't actually delete the key path from the structure. Instead, value at key path is set to undefined.
• Properly emit correct event (add, change, delete) when updating value at key path to falsey value (e.g. void 0, false, etc).
• Better heuristic on detecting existence of key path within a cursor by using notSetValue.

---

The PR stems from my attempts to emulate waitFor in flux based on cursor key paths:

var
immstruct = require('immstruct'),
Immutable = require('immutable');

var structure = immstruct({});

structure.on('swap', function(newStructure, oldStructure) {

    console.log('old struct:', oldStructure.toJS());
    console.log('new struct:', newStructure.toJS());
});

// This works
// structure.once('swap', function(newStructure, oldStructure) {
//     structure.cursor('profile').update('email', function() {
//         return 'bob@bob.com';
//     });
// });

// This doesn't work...
structure.once('add', function(path, newValue) {

    if(path.join('/') !== 'profile')
        return;

    structure.cursor(path).update('email', function() {
        return 'bob@bob.com';
    });

});

structure.cursor('profile').update(function() {
    return Immutable.fromJS({
        name: 'bob'
    });
});

console.log('final structure:', structure.cursor().toJS());
// output:
// old struct: {}
// new struct: { profile: { email: 'bob@bob.com' } }
// old struct: {}
// new struct: { profile: { name: 'bob' } }
// final structure: { profile: { name: 'bob' } }

[dashed]

One gotcha I found is that you cannot add a new key path with a value void 0 without setting notSetValue to anything but void 0. See: immutable-js/immutable-js#245

I think this is a bug immutable-js; I use alternative case for this within tests. Will amend test when issue is resolved.

[dashed]

Since immutable-js/immutable-js@335b312, if an identity function is set as an updater for Map.updateIn, value returned isn't commited; essentially if return value of updater === notSetValue.

Example:

var
Immutable = require('immutable');

var map1 = Immutable.Map({a: true});

var
notSetValue = 'red',
updater = function() {
    return notSetValue;
},
key = 'b'

var map2 = map1.update(key, notSetValue, updater);

console.log(map2.toJS());
// { a: true } <-- wrong?

I PR'd to relax this condition: immutable-js/immutable-js#246

---

This isn't a blocker for what I'm trying to do; but I'm hoping to smooth out the correctness 😄

[mikaelbr]

LGTM. Thanks! Good test coverage and the changes make sense. Especially the handlePersisting change.

[dashed]

Awesome thanks. Will this be pushed to npm anytime soon?

[mikaelbr]

It will be fairly soon. Just need to go over the code once more and test it out manually, see if there are any more changes we need to make and group that in the same patch release.

[dashed]

Alright, no problem. By the way, I'm mulling over the following test case:

immstruct/tests/structure_test.js

Lines 236 to 239 in c824b06

	// See: https://github.com/facebook/immutable-js/issues/245
	structure.cursor().update('3', true, function () {
	return void 0;
	});

I may consider dropping it until behaviour is resolved on immutable-js's end.

[mikaelbr]

It seems to me that it is testing the possibly only way for Immutable.js to solve it. Undefined might be tricky in this setting.

[dashed]

@mikaelbr This case isn't exclusive of undefined:

function pathExists(iterable, path) {

  if(iterable.getIn(path)) {
    return true;
  }
  var notSetValue = true;
  return !(iterable.getIn(path, notSetValue) === notSetValue);
}

assert(pathExists(cursor, path) === false);

// notSetValue is anything but void 0
notSetValue = 'default';
cursor.update(path, notSetValue, id => id);

assert(pathExists(cursor, path) === false);

[dashed]

By the way, immutable-js/immutable-js@8139d42 just landed. In the future, I'll PR to replace pathExists with Iterable.hasIn() when immutable API matures a bit more.

[mikaelbr]

Sounds good. Would be good not having immutable patches in immstruct code-base. I'm renaming pathExists to hasIn, to make the future change more apparent and making them more "semantically linked".

It seems however, the test you are talking about doesn't work in immutable@3.0.2, so I'm bumping the immutable dependency, making this a minor release instead of a patch release.

[mikaelbr]

➜  immstruct git:(master) npm publish
+ immstruct@1.2.1