Repository: gcanti/tcomb (js)

Issue:
t.update: cannot $merge and $remove at once
`update` returns immediately after processing the first top level command, so something like this is not possible: `const result = update(obj, { $merge: {a: [1, 2, 3] }, $remove: ['b'] })`

(Obviously I could use `const result = update(obj, { a: {$set: [1, 2, 3] }, $remove: ['b'] })`, but I'm trying to boil down to a simple example, we could want to merge multiple keys).

Here is a rough spec that shows this behavior, together with a 'fixed' version of `update` (`update2`) that passes. I'm not keen about the assumption that `value` has changed though, but comparing before and after values doesn't work as `$remove` (and several other commands), modifies the value object passed in.

```
import t from 'tcomb'

var isObject = require('tcomb/lib/isObject');
var getShallowCopy = require('tcomb/lib/getShallowCopy');

function update2(instance, spec) {
  if (process.env.NODE_ENV !== 'production') {
    assert(isObject(spec), function () { return 'Invalid argument spec ' + assert.stringify(spec) + ' supplied to function update(instance, spec): expected an object containing commands'; });
  }

  var value = getShallowCopy(instance);
  var isChanged = false;
  for (var k in spec) {
    if (spec.hasOwnProperty(k)) {
      if (t.update.commands.hasOwnProperty(k)) {
        value = t.update.commands[k](spec[k], value);
        isChanged = true
      }
      else {
        var newValue = update2(value[k], spec[k]);
        isChanged = isChanged || ( newValue !== value[k] );
        value[k] = newValue;
      }
    }
  }
  return isChanged ? value : instance;
}

describe('update', () => {
  const obj = {a: [1, 2], b: true}

  const methods = {tcomb: t.update, update2: update2}

  Object.keys(methods).forEach( (key) => {
    describe(`using ${key}`, () => {
      const update = methods[key]

      it('can $push', () => {
        const result = update(obj, { a: { $push: [3] } })
        expect(result).to.have.property('a').that.is.an('array').that.is.length(3)
        expect(result).to.have.property('b').that.equal(true)
      })

      it('can $remove', () => {
        const result = update(obj, { $remove: ['b'] })
        expect(result).to.have.property('a').that.is.an('array').that.is.length(2)
        expect(result).to.not.have.property('b')
      })

      it('can $push and $remove at once', () => {
        const result = update(obj, { a: { $push: [3] }, $remove: ['b'] })
        expect(result).to.have.property('a').that.is.an('array').that.is.length(3)
        expect(result).to.not.have.property('b')
      })

      it('can $merge and $remove at once', () => {
        const result = update(obj, { $merge: {a: [1, 2, 3] }, $remove: ['b'] })
        expect(result).to.have.property('a').that.is.an('array').that.is.length(3)
        expect(result).to.not.have.property('b')
      })

      it('can $set and $remove at once', () => {
        const result = update(obj, { a: {$set: [1, 2, 3] }, $remove: ['b'] })
        expect(result).to.have.property('a').that.is.an('array').that.is.length(3)
        expect(result).to.not.have.property('b')
      })
    })
  })
})
```


Pull request:
Fix #170 - t.update: cannot $merge and $remove at once

Patch:
diff --git a/lib/update.js b/lib/update.js
index 21e9fbe..e055bf3 100644
--- a/lib/update.js
+++ b/lib/update.js
@@ -18,7 +18,8 @@ function update(instance, spec) {
   for (var k in spec) {
     if (spec.hasOwnProperty(k)) {
       if (update.commands.hasOwnProperty(k)) {
-        return update.commands[k](spec[k], value);
+        value = update.commands[k](spec[k], value);
+        isChanged = true;
       }
       else {
         var newValue = update(value[k], spec[k]);


Test patch:
diff --git a/test/update.js b/test/update.js
index 462c3c6..7d32e11 100644
--- a/test/update.js
+++ b/test/update.js
@@ -95,6 +95,13 @@ describe('t.update(instance, spec)', function () {
     assert.deepEqual(actual, [1, 3, 2, 4]);
   });
 
+  it('can $merge and $remove at once', function () {
+    var instance = {a: [1, 2], b: true};
+    var actual = update(instance, { $merge: {a: [1, 2, 3] }, $remove: ['b'] });
+    assert.deepEqual(instance, {a: [1, 2], b: true});
+    assert.deepEqual(actual, {a: [1, 2, 3]});
+  });
+
   it('should not change the reference when no changes occurs', function () {
     var p1 = {x: 0, y: 1};
     var p2 = update(p1, {});
@@ -218,6 +225,11 @@ describe('t.update(instance, spec)', function () {
       var updated = update(instance, {$remove: ['a']});
       assert.deepEqual(updated, {b: 2});
     });
+  
+    it('can $merge and $remove at once', function () {
+      var updated = update(instance, { $merge: {a: [1, 2, 3] }, $remove: ['b'] });
+      assert.deepEqual(updated, {a: [1, 2, 3]});
+    });
 
   });