Better error handling.

n1k0 · n1k0 · commit 02a3d733fcb0 · 2015-03-13T15:46:45.000+01:00
diff --git a/package.json b/package.json
@@ -27,6 +27,7 @@
   "devDependencies": {
     "chai": "^2.1.*",
     "mocha": "^2.1.*",
+    "object-assign": "^2.0.0",
     "sinon": "^1.12.*",
     "supertest": "^0.15.*"
   }
diff --git a/scrape.js b/scrape.js
@@ -3,6 +3,7 @@ var phantomjs = require("phantomjs");
 var binPath = phantomjs.path;
 var path = require("path");
 var Promise = require("bluebird");
+var objectAssign = require("object-assign");
 
 var readabilityPath = process.env.READABILITY_LIB_PATH ||
                       path.normalize(path.join(__dirname, "vendor", "Readability.js"));
@@ -33,7 +34,7 @@ module.exports = function scrape(url, options) {
         error = response.error;
       }
       if (error) {
-        reject({error: error});
+        reject(objectAssign(new Error(), error));
       } else {
         fulfill(response);
       }
diff --git a/server.js b/server.js
@@ -46,7 +46,7 @@ app.get("/api/get", function(req, res) {
     res.json(sanitize ? sanitizeResult(result) : result);
   }).catch(function(err) {
     console.log(err);
-    res.status(500).json(err);
+    res.status(500).json({error: JSON.parse(JSON.stringify(err))});
   });
 });
 
diff --git a/static/index.html b/static/index.html
@@ -11,6 +11,7 @@
     <div class="page-header">
       <h1>Readability.js <small>test page</small></h1>
     </div>
+    <div class="alert alert-danger hide" id="error"></div>
     <div class="row">
       <div class="col-md-6">
         <form class="form-horizontal" id="form">
diff --git a/static/main.js b/static/main.js
@@ -4,6 +4,7 @@
   var q = document.querySelector.bind(document);
 
   function injectReadableContents(params, target) {
+    q("#error").classList.add("hide");
     var req = new XMLHttpRequest();
     var apiUrl = [
       "/api/get?sanitize=" + (params.sanitize ? "yes" : "no"),
@@ -13,12 +14,24 @@
     req.open("GET", apiUrl, false);
     req.send(null);
     var jsonResponse = JSON.parse(req.responseText);
-    q("#title").textContent = jsonResponse.title;
-    q("#byline").textContent = jsonResponse.byline;
-    q("#length").textContent = jsonResponse.length;
-    q("#dir").textContent = jsonResponse.dir;
-    q("#excerpt").textContent = jsonResponse.excerpt;
-    target.contentDocument.body.innerHTML = jsonResponse.content;
+    if (jsonResponse.error) {
+      q("#error").textContent = jsonResponse.error.message;
+      q("#error").classList.remove("hide");
+      q("#title").textContent = "";
+      q("#byline").textContent = "";
+      q("#length").textContent = "";
+      q("#dir").textContent = "";
+      q("#excerpt").textContent = "";
+      target.contentDocument.body.innerHTML = "";
+    } else {
+      q("#error").textContent = "";
+      q("#title").textContent = jsonResponse.title;
+      q("#byline").textContent = jsonResponse.byline;
+      q("#length").textContent = jsonResponse.length;
+      q("#dir").textContent = jsonResponse.dir;
+      q("#excerpt").textContent = jsonResponse.excerpt;
+      target.contentDocument.body.innerHTML = jsonResponse.content;
+    }
   }
 
   function init() {
diff --git a/test/index.js b/test/index.js
@@ -57,18 +57,31 @@ describe("Tests", function() {
       });
 
       scrape("http://invalid.test/").catch(function(err) {
-        expect(err.error.message).to.match(/Unable to parse JSON proxy response/);
+        expect(err.message).to.match(/Unable to parse JSON proxy response/);
+        done();
+      });
+    });
+
+    it("should reject on data extraction error", function(done) {
+      sandbox.stub(childProcess, "execFile", function(exec, args, cb) {
+        cb(null, JSON.stringify({error: {message: "Foo"}}));
+      });
+
+      scrape("http://invalid.test/").catch(function(err) {
+        expect(err).to.be.an.instanceOf(Error);
+        expect(err.message).eql("Foo");
         done();
       });
     });
 
     it("should fulfill with a valid json result", function(done) {
       sandbox.stub(childProcess, "execFile", function(exec, args, cb) {
-        cb(null, JSON.stringify({title: "plop"}));
+        cb(null, JSON.stringify({title: "plop", content: "plip"}));
       });
 
       scrape("http://invalid.test/").then(function(result) {
         expect(result.title).eql("plop");
+        expect(result.content).eql("plip");
         done();
       });
     });
@@ -131,6 +144,20 @@ describe("Tests", function() {
             .end(done);
         });
 
+        it("should return a server error on call error", function(done) {
+          sandbox.stub(childProcess, "execFile", function(exec, args, cb) {
+            cb(null, JSON.stringify({error: {message: "fail"}}));
+          });
+
+          request(app)
+            .get("/api/get?url=http://invalid.test/")
+            .expect(500)
+            .expect(function(res) {
+              expect(res.body.error.message).eql("fail");
+            })
+            .end(done);
+        });
+
         it("should apply custom user agent when provided", function(done) {
           sandbox.stub(childProcess, "execFile", function(exec, args, cb) {
             cb(null, "{}");

Original file line number	Diff line number	Diff line change
`@@ -27,6 +27,7 @@`
`27`	`27`	`"devDependencies": {`
`28`	`28`	`"chai": "^2.1.*",`
`29`	`29`	`"mocha": "^2.1.*",`
	`30`	`+ "object-assign": "^2.0.0",`
`30`	`31`	`"sinon": "^1.12.*",`
`31`	`32`	`"supertest": "^0.15.*"`
`32`	`33`	`}`