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.

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

(#2545) - throw errors instead of returning them #2546

Closed
wants to merge 1 commit into from

Conversation

calvinmetcalf
Copy link
Member

strictly speaking this is a breaking change as you can't use error name to identify the errors, but error.name means something kinda specific in js so hopefully thats ok.

@nolanlawson
Copy link
Member

Looks like this causes errors in pouchdb-server. Might have to open a parallel pull req in express-pouchdb as well.

calvinmetcalf added a commit to calvinmetcalf/express-pouchdb that referenced this pull request Aug 1, 2014
calvinmetcalf added a commit to calvinmetcalf/express-pouchdb that referenced this pull request Aug 1, 2014
@nolanlawson
Copy link
Member

Not being able to identify errors by their name actually breaks quite a bit of the code. There are a lot of places where we check err.name === 'not_found', e.g. in upsert.js, setup.js, and create-view.js (in mapreduce).

I would much rather we have a single commit that fixes all the problems with non-JS-style errors rather than the piecemeal thing we've been doing so far.

calvinmetcalf added a commit to pouchdb/mapreduce that referenced this pull request Aug 4, 2014
@calvinmetcalf
Copy link
Member Author

I would like to do it in one swoop as well, but I doubt I will have time for that and those gigantic pulls that change every file tend to be very tricky.

@nolanlawson
Copy link
Member

Fair enough. Also there doesn't seem to be anywhere that we're trying to detect these particular errors. Let's merge the express-pouchdb one, get a green check here, and then +1.

@calvinmetcalf
Copy link
Member Author

yeah I tried to get all of the files on friday when I had some time, but ended up doing a huge commit to detached head of the wrong branch

calvinmetcalf added a commit to pouchdb/mapreduce that referenced this pull request Aug 4, 2014
@nolanlawson
Copy link
Member

+1 when green, see my comments in the express-pouchdb PR.

calvinmetcalf added a commit to pouchdb/express-pouchdb that referenced this pull request Aug 5, 2014
@nolanlawson
Copy link
Member

published express@0.5.3, but NPM is being very eventually consistent today

@nolanlawson
Copy link
Member

@calvinmetcalf Your express-pouchdb fix is in, but it's still failing due to this:

  1) test.bulk_docs.js-http Test errors on invalid doc id:

      correct error status returned
      + expected - actual

      +400
      -500

      at /home/travis/build/pouchdb/pouchdb/tests/test.bulk_docs.js:128:27
      at /home/travis/build/pouchdb/pouchdb/lib/utils.js:400:11
      at process._tickCallback (node.js:419:13)

@calvinmetcalf
Copy link
Member Author

woops typo on my part

@@ -153,7 +159,7 @@ exports.parseDoc = function (doc, newEdits) {
if (doc._rev) {
revInfo = /^(\d+)-(.+)$/.exec(doc._rev);
if (!revInfo) {
throw "invalid value for property '_rev'";
throw new TypeError("invalid value for property '_rev'");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think you need to set status = 400 here

@nolanlawson
Copy link
Member

@calvinmetcalf Still not passing...

@calvinmetcalf
Copy link
Member Author

did we publish the new version of express-pouchdb yet?

@nolanlawson
Copy link
Member

yeah

@calvinmetcalf
Copy link
Member Author

the one with 01183ff9357c99751d1da4733b2b8bdfdd888131?

@calvinmetcalf
Copy link
Member Author

@nolanlawson
Copy link
Member

this is the one that was supposed to fix it, no? pouchdb/express-pouchdb@487d01a

@calvinmetcalf
Copy link
Member Author

didn't fix it completely hence the other one, sometimes a 400 error would
return but it would get turned into a 500 error

On Sun, Aug 10, 2014 at 11:54 AM, Nolan Lawson notifications@github.com
wrote:

this is the one that was supposed to fix it, no? pouchdb/express-pouchdb@
487d01a
pouchdb/express-pouchdb@487d01a


Reply to this email directly or view it on GitHub
#2546 (comment).

-Calvin W. Metcalf

@nolanlawson
Copy link
Member

Those are both merged into master tho.

@nolanlawson
Copy link
Member

Ah crap need to publish. One sec.

@nolanlawson
Copy link
Member

b9d8644

@nolanlawson nolanlawson deleted the error-throws branch August 12, 2014 05:45
calvinmetcalf added a commit to pouchdb/mapreduce that referenced this pull request Aug 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants