feature suggestion .update function

[lgandecki]

Hello, I've been playing with this a bit, love it!

The functionality I'm missing is to do updates in a mongo update(matcher, changes) manner. I've built a simple solution for that stealing some code from minimongo. Would you be willing to take a PR? Or is a lack of update a design decision?

What I'm doing now is:

users.find(matchingQuery).exec().then(function (docs) {
        docs.forEach(function (doc, index) {
          const newDoc = { ...doc._data }
          modify(newDoc, setQuery);
          delete newDoc._rev;
          delete newDoc._id;
          Object.keys(newDoc).forEach((el) => {
            doc[el] = newDoc[el];
          });
          doc.save()
        });
});

setQuery can be {$set: {}}, or use $rename, $pullAll $pop $push $unset etc.
modify changes the newDoc according to the setQuery.

I'm thinking of making the modify a standalone package, that would take a doc and return a new one, that could be a dependency in rxdb, and then I'd just add a simple update method. If not needed - not a problem! I will just polish it a bit for my own use :)

[pubkey]

Hi @lgandecki
thanks for giving the idea.
I never added the update-functionality because it was not on my priority-list.
It would be great to have a PR which handles this. I also think the best way would be to have a equal api to mongodb.
Creating a standalone is great so one day we (or someone else) can reuse it for a plugin for pouchdb's pouch-find or any mango-based queryengine.

A comment on your current code:
Modifying the _data object is dangerous because it is used internally for the 'change-detection'.
You should clone it before the modification.

import {
    default as clone
} from 'clone';
users.find(matchingQuery).exec().then(function (docs) {
        docs.forEach(function (doc, index) {
          const newDoc = clone({ ...doc._data }); // <- use the clone here
          modify(newDoc, setQuery);
          delete newDoc._rev;
          delete newDoc._id;
          Object.keys(newDoc).forEach((el) => {
            doc[el] = newDoc[el];
          });
          doc.save()
        });
});

[lgandecki]

Thanks! I finally had some time to try to tackle this, here is the effect:
#153

[pubkey]

We should also add the update-function this to RxQuery, similar to RxQuery.remove
This would enable us to do sth like

await myCollection.find()
  .where('age')
  .gt(18)
  .update({$set: {firstName: 'new first name'}});

[lgandecki]

Alright :)
I will work on this and remove the update of dist files later today.

[lgandecki]

done! I also recreated the branch without changing dist files.

[pubkey]

Hi @lgandecki
I'm currently at reviewing the PR. I have some problems.
(I will update this comment a few times in the next hours to complete the list)

1. In the PR, the modifyjs-dependency must be added to the package.json
2. The following test is not working (shouldnt it?):

        it('unset a value', async() => {
            const c = await humansCollection.createPrimary(1);
            const doc = await c.findOne().exec();
            console.dir(doc.firstName);
            await doc.update({
                $unset: {
                    firstName: ''
                }
            });
            const updatedDoc = await c.findOne().exec();
            assert.equal(updatedDoc.firstName, '');
        });
// AssertionError: 'Darby' == ''

3. The build-size increases by 36,3kB (minified)

• (Run npm run disc in the rxdb-folder to reproduce)
• One problem is the big underscore-dependency which makes 15,7kB. As I have inspected, it's mostly used for _.each which can be replaced by es6 array-functions.

4. Also there are things in modifyjs, which I don't understand:

• Why does it contain a custom base64-implementation?
• Is the usage of ejson needed. If yes, why not use the npm-module?
  Maybe we can try to make modifyjs more modularized so that we can pick the right imports for the things we need for RxDB-update().

[lgandecki]

hey @pubkey Thansk for the feedback!

1. I messed up the package.json when I created a new branch out of develop and recreated changes, to overwrite the previous pr with a branch that doesn't have dist files updated.. :) My bad.

2. I was only changing properties that existed in object returned from modifyjs. So.. if something wasn't there, it didn't get updated. Obvious now :) I changed the code, it's a bit more messy, if you have an idea how to tackle this differently let me know

3/4) I guess the size is so big and it has some weird things like internal ejson, base64 etc because I wanted to keep it as much as possible in line with minimongo code, to allow easier updates. But, now when I think about, I won't need to keep it up to date with minimongo too much, as long as the modifyjs functionality work. I can refactor the whole code and make it much smaller. I will work on that and let you know.

[lgandecki]

@pubkey
I've rethought the package and basically rewrote it (the unit tests were super helpful :-) )
It no longer tries to follow minimongo code, and because of that it's much smaller and cleaner.

At this point the npm run disc shows 11 kb added, but 3.7 kb of them are deepEqual and clone, the same ones you use in your project. Any ideas how to make rxdb use just one copy of them, instead of two?

The pr is updated

[pubkey]

@lgandecki Sorry, I messed up your PR.
I had to revert it, the following test is not working:

it('unset a value', async() => {
  const c = await humansCollection.createPrimary(1);
  const query = c.find();
  await query.update({
    $unset: {
      firstName: ''
    }
   });
  const docs = await query.exec();
  for (let doc of docs)
    assert.equal(doc._data.firstName, undefined);
    c.database.destroy();
});

[pubkey]

TODO:

• Add to index.d.ts
• Add to documentation
• Check why deepEqual and clone are not used from rxdb
• Add to CHANGELOG
• Add tests for each function with $set $unset

[lgandecki]

@pubkey I guess this was caused by my misunderstanding of how the .save() behaves.
I was doing changes on the this object directly, I switched to changing fields in this._data and it seems to work for more (maybe all, finally? ;) ) cases.

35ba865

It is a bit weird that deleting the field directly from the RxDocument would work in some cases, but not in the others. Good thing you caught this :-)

Also, I switched the unsetting to age, which isn't a required field on the "human" human schema.

What's next? Do you want to split the remaining work? Not sure what you mean by "tests for each function with $set $unset"

[pubkey]

Hi @lgandecki thank you for the new PR.

With the tests-task, I mean that for each of the 3 functions [RxCollection.update, RxQuery.update, RxDocument.update] we need 2 tests which use $set and $unset. This is just to make sure everything works as expected.

I will investigate about the deepEqual/clone-problem this afternoon. I have a clue on what could be wrong.

[pubkey]

@lgandecki I think the problem with the deepEqual/clone-import is, that your main in the package.json leads to the build-bundle which imports things like var clone = _interopDefault(require('clone')); it also includes all dependencies. The main should point to a es5-transpiled version of your code, so that the rxdb-bundler or any other bundler requiring modifyjs can later chose how to import things by itself.

[lgandecki]

Ok, thanks! I will take a look at this and adding the tests tomorrow.

[lgandecki]

@pubkey
Actually, looks it's all good in current version. It was requiring it separately because I was including it through npm link.. Once I removed the node_modules and reinstalled everything I can see modifyjs taking 7.4 kbps after the npm run disc. I hope that's acceptable? :-)

Working on the tests now..

[lgandecki]

Added the missing tests. Documentation and Changelog is missing, not sure if you prefer to change them yourself? :)
Also, would you want some help with setting up circleci? I think it would be great to run tests on every commit, especially since you have such a good coverage.

[lgandecki]

@pubkey is there anything more I can do to help this get merged?

[pubkey]

@lgandecki sorry for not replying. I have this merge in mind for the last 7 days and everyday something came in between.
I'm trying to run some tests today/tomorrow because I want to have this feature in version 4.1.0 which should come soon.

[pubkey]

Further changes by me:

• Removed RxCollection.update because RxCollection.remove does also not exist and the logic belongs into RxQuery.
• Added one test for $inc
• Fix Bug+Test when update on RxQuery.findOne with null-result.
• Added to Docs
• Added to changelog