Update mocha and fix 2 tests

[gkubisa]

See https://github.com/mochajs/mocha/releases/tag/v5.0.2.

The tests were failing because mocha@5.0.2+ crashes on unhandled exceptions resulting from unhandled "error" events. Earlier versions ignored those exceptions.

@ericyhwang this PR would also enable us to use the latest mocha version in sharedb-mongo.

[coveralls]

Coverage decreased (-0.07%) to 96.424% when pulling d5a03f3 on Teamwork:update-mocha into 762e05d on share:master.

[coveralls]

Coverage decreased (-80.6%) to 15.854% when pulling 14e5180 on Teamwork:update-mocha into 762e05d on share:master.

[gkubisa]

The recent change to shredb-mingo-memory (declaring sharedb as a peer dependency, as it should be) revealed an issue with a circular dependency between sharedb and sharedb-mingo-memory. The issue affects only running of sharedb tests. I worked around it in .travis.yml by creating a symlink to sharedb in a place where node would look for it, however, I don't think it's the right thing to do in the long term.

To be consistent with the other drivers and to avoid the circular dependency I think we should not use sharedb-mingo-memory in the sharedb unit tests.

Alternatively we could merge sharedb-mingo-memory into sharedb repo. We could still publish sharedb-mingo-memory as a separate npm package. This seems a little messy though.

@ericyhwang @nateps what are your thoughts on this?

[ericyhwang]

Looking into it a bit more, sharedb-mingo-memory requires sharedb here, so it should actually have sharedb as a normal dependency, instead of a peerDependency. That'll also match what sharedb-mongo does.

I'll do some testing of that approach and create a sharedb-mingo-memory PR if it does end up fixing the issue when running sharedb tests.

[gkubisa]

@ericyhwang no, it's actually important that it is a peer dependency because it guarantees that sharedb-mingo-memory extends MemoryDB from the ShareDB used by the application. If it's just a normal dependency, node could load 2 different versions of ShareDB at the same time. This could lead to a very hard to diagnose issue, if the 2 versions of ShareDB are not compatible.

sharedb-mongo should also have sharedb in peer dependencies and this PR share/sharedb-mongo#64 makes that change.

We can discuss it further, if it's still not clear but sharedb as a peer dependency is correct, so please don't change it.

[gkubisa]

The real problem is a circular dependency between sharedb and sharedb-mingo-memory, not a peer dependency.

[ericyhwang]

Ah, yeah, never mind - I'd originally just read the NPM docs on peerDependencies, which talks about plugins that don't necessarily require their host modules.

I've now found the original blog post about peer dependencies, which does say:

Even for plugins that do have such direct dependencies, probably due to the host package supplying utility APIs, specifying the dependency in the plugin's package.json would result in a dependency tree with multiple copies of the host package

What particular issue were you running into with the ShareDB tests? Was it from this sharedb-mingo-memory require not finding 'sharedb' on NPM 3+?

[gkubisa]

That's probably it. It could also be this https://github.com/share/sharedb-mingo-memory/blob/v1.0.1/test/test.js#L10. Basically anything in sharedb-mingo-memory which "requires" sharedb.

When running sharedb tests, node looks for node_modules/sharedb, however, npm install does not install sharedb because it installs packages for sharedb.

[ericyhwang]

Yup, I tried installing and running the sharedb tests on my machine with NPM 5, and it failed on https://github.com/share/sharedb-mingo-memory/blob/v1.0.1/index.js#L132. So it's not just broken on Travis. We could use a pretest script to do the symlinking and a posttest script to remove it.

Nate's in meetings pretty much all day, so I haven't had a chance to ask him if the sharedb tests truly need sharedb-mingo-memory. I guess we could try switching over to sharedb.MemoryDB and see what breaks.

[ericyhwang]

When switching to MemoryDB, the last few query-subscribe tests fail:
https://github.com/share/sharedb/blob/v1.0.0-beta.8/test/client/query-subscribe.js#L360-L444

They test that subscriptions with filter conditions will correctly update themselves when the underlying docs change. The problem is that for queries, MemoryDB will always return all docs in the collection, ignoring sort and filter conditions.

One option for removing the dependency on sharedb-mingo-memory would be to have db-memory.js add barebones filter/sort logic on top of the MemoryDB it uses - just enough logic to support the filter/sort that those test cases need. Reason for not doing it in MemoryDB itself is because I think it'd be more confusing for MemoryDB to support only a few particular query conditions, compared to today's situation of supporting no sort/filter.

Thoughts?

I'm not sure I'll have time today to try that out, and I'd anyways like to get @nateps's thoughts on the decoupling first before putting work into it.

[curran]

Totally agree with @gkubisa 's points re: peer dependency.

[ericyhwang]

As I mentioned in https://github.com/share/sharedb/pull/227/files#diff-354f30a63fb0907d4ad57269548329e3R8, after thinking over it a bit more, I'm OK with this workaround for now to unblock the failing tests, as the workaround is just isolated to Travis.

Can you add a comment above about this workaround?

[gkubisa]

ln -s .. node_modules/sharedb creates a link to sharedb in node_modules, so that node can find it when sharedb is loaded by package name in sharedb-mingo-memory.

The problem is a circular dependency between sharedb and sharedb-mingo-memory. npm install does not install sharedb in node_modules because it is redundant considering that the dependencies are installed for sharedb, however, sharedb-mingo-memory tries to load sharedb by package name, so node looks for it in node_modules.

[curran]

Oh man what a mess. Why does ShareDB depend on sharedb-mingo-memory at all? Intuitively it should not.

[gkubisa]

I totally agree with @curran .

[nateps]

OK, cool. We'll merge this to get travis working in the meantime and then we can address decoupling subsequently.