Initial implementation #1

alecgibson · 2018-08-20T09:20:24Z

This is an initial implementation of ShareDB's MilestoneDB. The
bare bones of MongoMilestoneDB connection logic are based on the
sharedb-mongo database adapter.

This adapter is a slight departure from other ShareDB code, as it is
written in ES6. This is because this code is server-side-only, so we
don't need to support legacy browsers and all LTS Node versions support
ES6. The move should ease maintenance and understanding of asynchronous
code through the use of Promises, which the mongodb library
supports out of the box.

This library notably uses v2 of mongodb instead of the newer v3. This
is to stay consistent with sharedb-mongo, so in theory the same config
(or style of config) can be used in both sharedb-mongo and in
sharedb-milestone-mongo.

We also introduce usage of eslint instead of jshint, which
is used in other projects. eslint supports a much, much wider range of
style rules and helps to maintain a uniform coding style.

Dependencies

Add a NoOpMilestoneDB implementation sharedb#244

alecgibson · 2018-08-20T09:54:33Z

I'm happy to discuss what language this should be written in. I've written it in ES6 as the most modern vanilla JS that LTS Node supports (ideally I'd have liked ES7 for async/await).

Other options include:

Keep it in ES5, which would force us back into callbacks. This would be more consistent with the rest of ShareDB, but it also forces us to do things like basically reimplement a Promise, which we obviously get for free in ES6. I also think it's particularly weird to not use the Promises returned directly by mongodb.
Use a transpiler, which would let us use ES7, or even move to TypeScript. I personally love TypeScript, but I realise some people think that this can raise the maintenance burden for people switching between libraries in the same org. I personally think a good IDE goes a long way to basically removing this burden, but I also switch between Vanilla and TypeScript on basically an hourly basis, so perhaps I'm more used to it than some. I can definitely see an argument for staying Vanilla on an open source library, especially since I've sometimes looked at eg CoffeeScript libraries and had no idea what's going on.

This is an initial implementation of ShareDB's [`MilestoneDB`][1]. The bare bones of `MongoMilestoneDB` connection logic are based on the [`sharedb-mongo`][2] database adapter. This adapter is a slight departure from other ShareDB code, as it is written in ES6. This is because this code is server-side-only, so we don't need to support legacy browsers and all LTS Node versions support ES6. The move should ease maintenance and understanding of asynchronous code through the use of `Promise`s, which the [`mongodb`][3] library supports out of the box. This library notably uses v2 of `mongodb` instead of the newer v3. This is to stay consistent with `sharedb-mongo`, so in theory the same config (or style of config) can be used in both `sharedb-mongo` and in `sharedb-milestone-mongo`. We also introduce usage of [`eslint`][4] instead of [`jshint`][5], which is used in other projects. `eslint` supports a much, much wider range of style rules and helps to maintain a uniform coding style. [1]: share/sharedb#236 [2]: share/sharedb-mongo [3]: mongodb.github.io/node-mongodb-native [4]: https://eslint.org/ [5]: http://jshint.com/

ericyhwang

From the ShareDB PR review meeting:

ericyhwang · 2018-08-28T16:17:18Z

lib/mongo-milestone-db.js

+      mongo = options.mongo;
+    }
+
+    options = options || {};


Since we're deleting things off of the options object several lines down, let's do a shallow clone of options.

NB: The motivation for deleting these options is because the Mongo client validates them. In fact, if you pass in the option validateOptions, then your mongo connection will actually fail if we don't tidy these up.

https://github.com/mongodb/node-mongodb-native/blob/bd4fb531a7f599bb6cf50ebbab3b986f191a7ef8/lib/mongo_client.js#L53-L57

I'll add a comment in the code for this, too.

ericyhwang · 2018-08-28T16:28:02Z

lib/mongo-milestone-db.js

+    delete options.mongo;
+    delete options.interval;
+    delete options.disableIndexCreation;
+    this._mongo = MongoMilestoneDB._connect(mongo, options);


_mongoPromise to indicate it's a promise

ericyhwang · 2018-08-28T16:31:56Z

lib/mongo-milestone-db.js

+
+    if (!callback) {
+      callback = (error) => {
+        error ? this.emit('error', error) : this.emit('save', wasSaved, collectionName, snapshot);


(Nate) Ternary is a bit surprising here, I try to not use it unless I'm using the value. Switch to if-then here

ericyhwang · 2018-08-28T16:35:15Z

lib/mongo-milestone-db.js

+      };
+    }
+
+    if (!snapshot) return process.nextTick(callback, null, wasSaved, collectionName, snapshot);


Precondition checking - Let's call back with an error here if there's no snapshot or collection name.

collectionName and snapshot shouldn't be necessary here, as above, you're using them from the closure.

NB: To ensure consistency across database adapters, I've added these precondition test cases to the core set of tests: share/sharedb#244

ericyhwang · 2018-08-28T16:45:25Z

lib/mongo-milestone-db.js

+  }
+
+  saveMilestoneSnapshot(collectionName, snapshot, callback) {
+    let wasSaved = false;


From discussion to simplify API:

Change the base MilestoneDB to return errors

Add a new no-op (null) MilestoneDB for Share to use by default

Remove the wasSaved from this API

Done. See here: share/sharedb#244

ericyhwang · 2018-08-28T16:47:42Z

lib/mongo-milestone-db.js

+    return this._collection(collectionName)
+      .then((collection) => {
+        const query = { id: snapshot.id, v: snapshot.v };
+        const doc = MongoMilestoneDB._shallowClone(snapshot);


See if we can remove this shallow clone, if we can't remove it, mention why we need to (Mongo driver?)

alecgibson · 2018-08-29T12:30:07Z

@ericyhwang I've addressed the issues we discussed in the meeting. Note that this PR now has a dependency on an update to the Driver API in ShareDB core: share/sharedb#244

ericyhwang · 2018-08-29T20:35:30Z

lib/mongo-milestone-db.js

+  static _shallowClone(object) {
+    if (typeof object !== 'object') return object;
+
+    const out = {};


This can be written as:

return Object.assign({}, object)

Object.assign copies enumerable own properties from the second argument (then the third arg, fourth arg, etc. if present) onto the first object. Otherwise, if doing a manual loop, a object.hasOwnProperty() check would be needed to avoid copying over enumerable prototype properties on object.

Ha! I somehow completely forgot that I was in ES6 🤦🏻‍♂️

ericyhwang · 2018-08-29T20:44:09Z

lib/mongo-milestone-db.js

+          // collection this won't be a problem, but this is a dangerous mechanism.
+          // Perhaps we should only warn instead of creating the indexes, especially
+          // when there is a lot of data in the collection.
+          return collection.createIndex({id: 1, v: 1}, {background: true, unique: true});


I'm wondering, will it get confusing to have both _id and id properties on these Mongo documents?

One idea is to use d, to match what the Mongo ops collections use to indicate the Share doc id. I'm open to other ideas, or staying with id if we end up deciding that's fine.

I mostly like using id directly because it avoids any need for mapping the snapshots when reading/writing, but if you feel strongly we can change it. I think it quickly becomes clear which id is which when you poke around in the data at all (if you are so inclined). And if you're poking around, you're hopefully both:

familiar with Mongo, and therefore unsurprised by an _id field containing an ObjectID

familiar with ShareDB, and therefore unsurprised by an id field corresponding to the ShareDB doc ID

What do you think?

From Share PR meeting:

Nate prefers to use d as the name

See if we can use projection to rename it to id when reading

alecgibson · 2018-08-30T07:54:00Z

@ericyhwang I've addressed the Object.assign thing. I guess we just need to discuss the id vs _id thing. I'm personally happy to leave it, but if you feel strongly, I can map id to d.

alecgibson · 2018-09-06T07:36:43Z

@ericyhwang I've bumped the ShareDB version so this no longer points at a fork, and I've added the id -> d mapping we discussed.

Unfortunately, I don't think the Mongo lets you use string values for renaming projections (there's a bunch of code like this floating around that just changes values to 1), so I've just added a couple of methods for mapping to and from the database representation.

In order to stay consistent with ops in `sharedb-mongo`, this change maps a snapshot's `id` field to `d` when storing it in Mongo, and then maps it back on fetch. This should also avoid confusion with having both an `_id` field and an `id` field.

ericyhwang

LGTM! Feel free to merge and publish it yourself

alecgibson · 2018-09-13T08:43:19Z

Released as v0.1.0. @ericyhwang and @nateps I've added you both as owners of the package.

alecgibson force-pushed the initial-implementation branch 2 times, most recently from ddb4ee9 to 6e20dcc Compare August 20, 2018 09:24

alecgibson mentioned this pull request Aug 20, 2018

Add Milestone Snapshots share/sharedb#236

Merged

alecgibson force-pushed the initial-implementation branch from 6e20dcc to 7988cb4 Compare August 20, 2018 09:31

alecgibson force-pushed the initial-implementation branch from 7988cb4 to bb7258b Compare August 20, 2018 14:02

alecgibson force-pushed the initial-implementation branch from bb7258b to 3c77eb3 Compare August 20, 2018 15:41

ericyhwang reviewed Aug 28, 2018

View reviewed changes

alecgibson force-pushed the initial-implementation branch from 13b5d53 to a83e5c8 Compare August 29, 2018 12:26

alecgibson changed the title ~~Initial implementation~~ [DO NOT MERGE] Initial implementation Aug 29, 2018

alecgibson mentioned this pull request Aug 29, 2018

Use yarn share/sharedb#241

Closed

ericyhwang reviewed Aug 29, 2018

View reviewed changes

alecgibson force-pushed the initial-implementation branch from a83e5c8 to 372759c Compare August 30, 2018 07:52

Review markups

07b1cf6

alecgibson force-pushed the initial-implementation branch from 65bbae4 to 6f4e428 Compare September 6, 2018 07:29

alecgibson changed the title ~~[DO NOT MERGE] Initial implementation~~ Initial implementation Sep 6, 2018

Store Snapshot id as d in Mongo

cc68116

In order to stay consistent with ops in `sharedb-mongo`, this change maps a snapshot's `id` field to `d` when storing it in Mongo, and then maps it back on fetch. This should also avoid confusion with having both an `_id` field and an `id` field.

alecgibson force-pushed the initial-implementation branch from 6f4e428 to cc68116 Compare September 6, 2018 07:38

ericyhwang approved these changes Sep 12, 2018

View reviewed changes

alecgibson merged commit 33a38e7 into master Sep 13, 2018

alecgibson deleted the initial-implementation branch September 13, 2018 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation #1

Initial implementation #1

alecgibson commented Aug 20, 2018 •

edited

Loading

alecgibson commented Aug 20, 2018

ericyhwang left a comment

ericyhwang Aug 28, 2018

alecgibson Aug 29, 2018

ericyhwang Aug 28, 2018

ericyhwang Aug 28, 2018

ericyhwang Aug 28, 2018

alecgibson Aug 29, 2018

ericyhwang Aug 28, 2018

alecgibson Aug 29, 2018

ericyhwang Aug 28, 2018

alecgibson commented Aug 29, 2018

ericyhwang Aug 29, 2018

alecgibson Aug 30, 2018

ericyhwang Aug 29, 2018

alecgibson Aug 30, 2018

ericyhwang Sep 5, 2018

alecgibson commented Aug 30, 2018

alecgibson commented Sep 6, 2018

ericyhwang left a comment

alecgibson commented Sep 13, 2018

Initial implementation #1

Initial implementation #1

Conversation

alecgibson commented Aug 20, 2018 • edited Loading

Dependencies

alecgibson commented Aug 20, 2018

ericyhwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgibson commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgibson commented Aug 30, 2018

alecgibson commented Sep 6, 2018

ericyhwang left a comment

Choose a reason for hiding this comment

alecgibson commented Sep 13, 2018

alecgibson commented Aug 20, 2018 •

edited

Loading