Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

catch errors thrown in diff function #9

Closed
wants to merge 1 commit into from
Closed

catch errors thrown in diff function #9

wants to merge 1 commit into from

Conversation

tyler-johnson
Copy link
Contributor

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
Copy link
Member

LGTM. Thanks!

nolanlawson pushed a commit that referenced this pull request Aug 10, 2015
@tyler-johnson @nolanlawson
(#9) - catch errors thrown in diff function
374ff54
@nolanlawson
Copy link
Member

374ff54

nolanlawson added a commit that referenced this pull request Aug 10, 2015
@nolanlawson
Revert "(#9) - catch errors thrown in diff function"
8411f09 
This reverts commit 374ff54.
@nolanlawson
Copy link
Member

@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:

�[0m  1) test.mapreduce.js-upsert should throw an error if the doc errors:
�[0m�[31m     TypeError: undefined is not a function�[0m�[90m
      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)
�[0m
�[0m  2) test.mapreduce.js-upsert should fulfill if the diff returns false:
�[0m�[31m     TypeError: undefined is not a function�[0m�[90m
      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)
�[0m
�[0m  3) test.mapreduce.js-upsert should error if it can't put:
�[0m�[31m     TypeError: undefined is not a function�[0m�[90m
      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)
�[0m

@tyler-johnson
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Ah gotcha. OK, well I have another suggestion for resolving this issue: pouchdb/pouchdb#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
Copy link
Contributor Author

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

@nolanlawson nolanlawson reopened this Sep 5, 2015
@nolanlawson
Copy link
Member

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
Copy link
Member

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
Copy link
Member

c5848b0

@nolanlawson
Copy link
Member

Crap, this broke the build: #14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tyler-johnson @nolanlawson