search

pouchdb / upsert

PouchDB plugin for upsert() and putIfNotExists() functions
Apache License 2.0
149 stars 25 forks source link

catch errors thrown in diff function #9

Closed tyler-johnson closed 8 years ago

tyler-johnson commented 9 years ago

I noticed that errors being thrown in the diff function were crashing my program. I moved the upsert logic from the get() callback into a Promise chain. This fixed the issue and made the code a bit cleaner, IMO.

nolanlawson commented 9 years ago

LGTM. Thanks!

nolanlawson commented 9 years ago

374ff542aff468eaf8375033d637e069fec34dc4

nolanlawson commented 9 years ago

@tyler-johnson Sorry, but I had to revert and unpublish this, because it broke a test in pouchdb itself. My bad for not running the test suite before merging. Here's the error:

  1) test.mapreduce.js-upsert should throw an error if the doc errors:
     TypeError: undefined is not a function
      at Object.upsert.get (/home/travis/build/pouchdb/pouchdb/tests/unit/test.mapreduce.js:15:9)
      at upsertInner (/home/travis/build/pouchdb/pouchdb/node_modules/pouchdb-upsert/index.js:19:13)
      at Object.upsert (/home/travis/build/pouchdb/pouchdb/node_modules/pouchdb-upsert/index.js:61:17)
      at module.exports (/home/travis/build/pouchdb/pouchdb/lib/mapreduce/upsert.js:6:17)
      at Context.<anonymous> (/home/travis/build/pouchdb/pouchdb/tests/unit/test.mapreduce.js:13:12)
      at callFn (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runnable.js:250:21)
      at Test.Runnable.run (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runnable.js:243:7)
      at Runner.runTest (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:373:10)
      at /home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:451:12
      at next (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:298:14)
      at /home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:308:7
      at next (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:246:23)
      at Object._onImmediate (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:275:5)
      at processImmediate [as _immediateCallback] (timers.js:363:15)

  2) test.mapreduce.js-upsert should fulfill if the diff returns false:
     TypeError: undefined is not a function
      at Object.upsert.get (/home/travis/build/pouchdb/pouchdb/tests/unit/test.mapreduce.js:22:9)
      at upsertInner (/home/travis/build/pouchdb/pouchdb/node_modules/pouchdb-upsert/index.js:19:13)
      at Object.upsert (/home/travis/build/pouchdb/pouchdb/node_modules/pouchdb-upsert/index.js:61:17)
      at module.exports (/home/travis/build/pouchdb/pouchdb/lib/mapreduce/upsert.js:6:17)
      at Context.<anonymous> (/home/travis/build/pouchdb/pouchdb/tests/unit/test.mapreduce.js:20:12)
      at callFn (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runnable.js:250:21)
      at Test.Runnable.run (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runnable.js:243:7)
      at Runner.runTest (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:373:10)
      at /home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:451:12
      at next (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:298:14)
      at /home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:308:7
      at next (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:246:23)
      at Object._onImmediate (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:275:5)
      at processImmediate [as _immediateCallback] (timers.js:363:15)

  3) test.mapreduce.js-upsert should error if it can't put:
     TypeError: undefined is not a function
      at Object.upsert.get (/home/travis/build/pouchdb/pouchdb/tests/unit/test.mapreduce.js:31:9)
      at upsertInner (/home/travis/build/pouchdb/pouchdb/node_modules/pouchdb-upsert/index.js:19:13)
      at Object.upsert (/home/travis/build/pouchdb/pouchdb/node_modules/pouchdb-upsert/index.js:61:17)
      at module.exports (/home/travis/build/pouchdb/pouchdb/lib/mapreduce/upsert.js:6:17)
      at Context.<anonymous> (/home/travis/build/pouchdb/pouchdb/tests/unit/test.mapreduce.js:29:12)
      at callFn (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runnable.js:250:21)
      at Test.Runnable.run (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runnable.js:243:7)
      at Runner.runTest (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:373:10)
      at /home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:451:12
      at next (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:298:14)
      at /home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:308:7
      at next (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:246:23)
      at Object._onImmediate (/home/travis/build/pouchdb/pouchdb/node_modules/mocha/lib/runner.js:275:5)
      at processImmediate [as _immediateCallback] (timers.js:363:15)

tyler-johnson commented 9 years ago

So it seems like the error stems from the fact that tests are creating a fake database with a custom get() that uses callbacks in order to test the functionality of upsert. Can we modify those to use Promises instead?

For reference, https://github.com/pouchdb/pouchdb/blob/master/tests/unit/test.mapreduce.js#L13-L17

nolanlawson commented 9 years ago

If the PR broke the functionality of callbacks, then that's a bug, because this lib is supposed to support both callbacks and promises...

Unless it's some other issue, in which case, yes, let's go ahead and fix the test. :)

tyler-johnson commented 9 years ago

Upsert still supports callbacks. The test code is actually taking on the role of db.get(), instead of using PouchDB's get() and does not return a promise, it only accepts callbacks. Hence why the code works before, but not after.

nolanlawson commented 9 years ago

Ah gotcha. OK, well I have another suggestion for resolving this issue: https://github.com/pouchdb/pouchdb/issues/4144

If we make it so PouchDB is no longer depending upon pouchdb-upsert, then we no longer need to worry about tests breaking in PouchDB when something changes in this module.

tyler-johnson commented 9 years ago

Now that pouchdb/pouchdb#4144 has landed, can we get this one merged as well?

nolanlawson commented 9 years ago

Well unfortunately this is a breaking change, because it will break for anybody still running PouchDB <4.0.1 out there (because we were using semver ^s in those versions to update this lib).

I'm all for merging this and the other PRs that have been stalled, but yeah, it will require a new major version.

nolanlawson commented 8 years ago

This is definitely safe to merge now. I'm not concerned about breaking PouchDB <4.0.1; people should be updating. But I'll publish this as a major release anyway.

nolanlawson commented 8 years ago

c5848b0701f43c6b54307bd98feef035948a9303

nolanlawson commented 8 years ago

Crap, this broke the build: https://github.com/pouchdb/upsert/issues/14