feat: Allow errors bubble up to the caller (#83)

* fix: Allow errors bubble up to the caller

This patch prevents unhandled Promise rejections from crashing the process by allowing the caller to handle them instead.

To properly call `Builder#analyze()`, you **SHOULD** change your code from:

```js
builder.analyze(function(results) {
  // Do something with results
});
```

To:

```js
builder.analyze(function(err, results) {
  if (err) {
    // handle error
  }

  // Do something with results
});
```

However, if you do not update your code, `axe-webdriverjs` will continue to function the same way (for the time being), but will print a deprecation warning to `stderr`. ⚠️ This functionality will be removed in the next major release.⚠️

Additionally, when using the Promise returned by `Builder#analyze()`, you **SHOULD** add a `.catch()` block to handle the possible rejection. For example:

```js
builder
  .analyze()
  .then(results => {
    // Do something with results
  })
  .catch(err => {
    // handle error
  });
```

If neither a `.catch()` block or a callback are provided, an unhandled rejection will occur and the Node process will crash. ☠️

NOTE: when using a Promise _and_ a callback, the Promise will never be rejected. Either the callback will be provided the error, or the error will be thrown instead. This ensures we don't require users to handle the same error twice.

New tests have been added ensuring this behavior works as expected. Additionally, all old tests have been migrated to the new API, which should help encourage users to do the same.

Fixes #56.

* chore: update deprecation message

README.md

@@ -21,7 +21,7 @@ Install axe-webdriverjs and its dependencies: `npm install axe-webdriverjs`
 
 ## Usage
 
-This module uses a chainable API to assist in injecting, configuring and analyzing using aXe with Selenium WebDriverJS.  As such, it is required to pass an instance of WebDriver.
+This module uses a chainable API to assist in injecting, configuring and analyzing using aXe with Selenium WebDriverJS. As such, it is required to pass an instance of WebDriver.
 
 Here is an example of a script that will drive Selenium to this repository, perform analysis and then log results to the console.
 
@@ -35,17 +35,19 @@ var driver = new WebDriver.Builder()
 
 driver
   .get('https://dequeuniversity.com/demo/mars/')
-  .then(function () {
-    AxeBuilder(driver)
-      .analyze(function (results) {
-        console.log(results);
-      });
+  .then(function() {
+    AxeBuilder(driver).analyze(function(err, results) {
+      if (err) {
+        // Handle error somehow
+      }
+      console.log(results);
+    });
   });
 ```
 
 ### AxeBuilder(driver:WebDriver[, axeSource:string])
 
-Constructor for the AxeBuilder helper. You must pass an instance of selenium-webdriver as the first and only argument.  Can be called with or without the `new` keyword.
+Constructor for the AxeBuilder helper. You must pass an instance of selenium-webdriver as the first and only argument. Can be called with or without the `new` keyword.
 
 ```javascript
 var builder = AxeBuilder(driver);
@@ -79,16 +81,16 @@ AxeBuilder(driver)
 
 ### AxeBuilder#options(options:Object)
 
-Specifies options to be used by `axe.a11yCheck`.  **Will override any other configured options, including calls to `withRules` and `withTags`.** See [axe-core API documentation](https://github.com/dequelabs/axe-core/blob/master/doc/API.md) for information on its structure.
+Specifies options to be used by `axe.a11yCheck`. **Will override any other configured options, including calls to `withRules` and `withTags`.** See [axe-core API documentation](https://github.com/dequelabs/axe-core/blob/master/doc/API.md) for information on its structure.
 
 ```javascript
 AxeBuilder(driver)
-  .options({ checks: { "valid-lang": ["orcish"] }});
+  .options({ checks: { 'valid-lang': ['orcish'] } });
 ```
 
 ### AxeBuilder#withRules(rules:Mixed)
 
-Limits analysis to only those with the specified rule IDs.  Accepts a String of a single rule ID or an Array of multiple rule IDs. **Subsequent calls to `AxeBuilder#options`, `AxeBuilder#withRules` or `AxeBuilder#withRules` will override specified options.**
+Limits analysis to only those with the specified rule IDs. Accepts a String of a single rule ID or an Array of multiple rule IDs. **Subsequent calls to `AxeBuilder#options`, `AxeBuilder#withRules` or `AxeBuilder#withRules` will override specified options.**
 
 ```javascript
 AxeBuilder(driver)
@@ -102,7 +104,7 @@ AxeBuilder(driver)
 
 ### AxeBuilder#withTags(tags:Mixed)
 
-Limits analysis to only those with the specified rule IDs.  Accepts a String of a single tag or an Array of multiple tags.  **Subsequent calls to `AxeBuilder#options`, `AxeBuilder#withRules` or `AxeBuilder#withRules` will override specified options.**
+Limits analysis to only those with the specified rule IDs. Accepts a String of a single tag or an Array of multiple tags. **Subsequent calls to `AxeBuilder#options`, `AxeBuilder#withRules` or `AxeBuilder#withRules` will override specified options.**
 
 ```javascript
 AxeBuilder(driver)
@@ -116,7 +118,7 @@ AxeBuilder(driver)
 
 ### AxeBuilder#disableRules(rules:Mixed)
 
-Skips verification of the rules provided.  Accepts a String of a single rule ID or an Array of multiple rule IDs. **Subsequent calls to `AxeBuilder#options`, `AxeBuilder#disableRules` will override specified options.**
+Skips verification of the rules provided. Accepts a String of a single rule ID or an Array of multiple rule IDs. **Subsequent calls to `AxeBuilder#options`, `AxeBuilder#disableRules` will override specified options.**
 
 ```javascript
 AxeBuilder(driver)
@@ -131,7 +133,6 @@ AxeBuilder(driver)
   .disableRules('color-contrast');
 ```
 
-
 ### AxeBuilder#configure(config:Object)
 
 Inject an aXe configuration object to modify the ruleset before running Analyze. Subsequent calls to this
@@ -146,18 +147,24 @@ var config = {
 };
 AxeBuilder(driver)
   .configure(config)
-  .analyze(function (results) {
+  .analyze(function(err, results) {
+    if (err) {
+      // Handle error somehow
+    }
     console.log(results);
   });
 ```
 
 ### AxeBuilder#analyze(callback:Function)
 
-Performs analysis and passes the result object to the provided callback function or promise function.  **Does not chain as the operation is asynchronous**
+Performs analysis and passes any encountered error and/or the result object to the provided callback function or promise function. **Does not chain as the operation is asynchronous**
 
 ```javascript
 AxeBuilder(driver)
-  .analyze(function (results) {
+  .analyze(function(err, results) {
+    if (err) {
+      // Handle error somehow
+    }
     console.log(results);
   });
 ```
@@ -167,11 +174,16 @@ Using the returned promise (optional):
 ```javascript
 AxeBuilder(driver)
   .analyze()
-  .then(function (results) {
+  .then(function(results) {
     console.log(results);
+  })
+  .catch(err => {
+    // Handle error somehow
   });
 ```
 
+_NOTE: to maintain backwards compatibility, the `analyze` function will also accept a callback which takes a single `results` argument. However, if an error is encountered during analysis, the error will be raised which will cause the **process to crash**. ⚠️ This functionality will be removed in the next major release.⚠️_
+
 ## Examples
 
 This project has a couple integrations that demonstrate the ability and use of this module:
@@ -180,7 +192,6 @@ This project has a couple integrations that demonstrate the ability and use of t
 1. [Running against a page with frames](test/integration/frames.js)
 1. [SauceLabs example](test/sauce/sauce.js)
 
-
 ## Contributing
 
 Read the [documentation on contributing](CONTRIBUTING.md)

lib/index.js

@@ -1,3 +1,4 @@
+var deprecate = require('depd')('axe-webdriverjs');
 var AxeInjector = require('./axe-injector');
 var normalizeContext = require('./normalize-context');
 
@@ -118,7 +119,10 @@ AxeBuilder.prototype.configure = function(config) {
 
 /**
  * Perform analysis and retrieve results. *Does not chain.*
- * @param  {Function} callback Function to execute when analysis completes; receives one argument, the results object of analysis
+ *
+ * If a `callback` is provided, it is strongly recommended that it accepts two arguments: `error, results`. If only a single argument is accepted, a deprecation warning will be printed to `stderr` and any errors encoutered during analysis will crash the Node process.
+ *
+ * @param  {Function} [callback] Function to execute when analysis completes
  * @return {Promise}
  */
 AxeBuilder.prototype.analyze = function(callback) {
@@ -128,7 +132,13 @@ AxeBuilder.prototype.analyze = function(callback) {
     config = this._config,
     source = this._source;
 
-  return new Promise(function(resolve) {
+  // Check if the provided `callback` uses the old argument signature (an arity of 1). If it does, provide a helpful deprecation warning.
+  var isOldAPI = callback && callback.length === 1;
+  if (isOldAPI) {
+    deprecate('Error must be handled as the first argument of axe.analyze. See: #83');
+  }
+
+  return new Promise((resolve, reject) => {
     var injector = new AxeInjector({ driver, axeSource: source, config });
     injector.inject(() => {
       driver
@@ -148,9 +158,27 @@ AxeBuilder.prototype.analyze = function(callback) {
         )
         .then(function(results) {
           if (callback) {
-            callback(results);
+            // If using the old API, provide the `results` as the first and only argument. Otherwise, provide `null` indicating no errors were encountered.
+            if (isOldAPI) {
+              callback(results);
+            } else {
+              callback(null, results);
+            }
           }
           resolve(results);
+        })
+        .catch(err => {
+          // When using a callback, do *not* reject the wrapping Promise. This prevents having to handle the same error twice.
+          if (callback) {
+            // If using the old API, throw this error (and unfortunately crash the process), since there is no way to handle it. Otherwise, provide the error as the first argument ("error-back" style).
+            if (isOldAPI) {
+              throw err;
+            } else {
+              callback(err, null);
+            }
+          } else {
+            reject(err);
+          }
         });
     });
   });

package.json

@@ -90,7 +90,8 @@
   },
   "dependencies": {
     "axe-core": "^3.0.0",
-    "babel-runtime": "^6.26.0"
+    "babel-runtime": "^6.26.0",
+    "depd": "^2.0.0"
   },
   "prettier": {
     "semi": true,

test/integration/configure-frames.js

@@ -56,7 +56,8 @@ describe('outer-configure-frame.html', function() {
         }
       })
       .configure(json)
-      .analyze(function(results) {
+      .analyze()
+      .then(function(results) {
         assert.equal(results.violations[0].id, 'dylang');
         // the second violation is in a frame
         assert.equal(results.violations[0].nodes.length, 2);

test/integration/doc-dylang.js

@@ -53,7 +53,8 @@ describe('doc-dylang.html', function() {
     AxeBuilder(driver, src)
       .configure(json)
       .withRules(['dylang'])
-      .analyze(function(results) {
+      .analyze()
+      .then(function(results) {
         assert.lengthOf(results.violations, 1);
         assert.equal(results.violations[0].id, 'dylang');
         assert.notEqual(

test/integration/doc-lang.js

@@ -65,7 +65,8 @@ describe('doc-lang.html', function() {
   it('should not find violations when the rule is disabled', function(done) {
     AxeBuilder(driver)
       .options({ rules: { 'html-has-lang': { enabled: false } } })
-      .analyze(function(results) {
+      .analyze()
+      .then(function(results) {
         results.violations.forEach(function(violation) {
           assert.notEqual(violation.id, 'html-has-lang');
         });

test/integration/frames.js

@@ -45,7 +45,8 @@ describe('outer-frame.html', function() {
   it('should find violations', function(done) {
     AxeBuilder(driver)
       .withRules('html-lang-valid')
-      .analyze(function(results) {
+      .analyze()
+      .then(function(results) {
         assert.lengthOf(results.violations, 1, 'violations');
         assert.equal(results.violations[0].id, 'html-lang-valid');
         assert.lengthOf(
@@ -71,7 +72,8 @@ describe('outer-frame.html', function() {
       .include('body')
       .options({ checks: { 'valid-lang': { options: ['bobbert'] } } })
       .withRules('html-lang-valid')
-      .analyze(function(results) {
+      .analyze()
+      .then(function(results) {
         assert.lengthOf(results.violations, 0);
         assert.lengthOf(results.passes, 1);
         done();
@@ -81,7 +83,8 @@ describe('outer-frame.html', function() {
   it('should not find violations when the rule is disabled', function(done) {
     AxeBuilder(driver)
       .options({ rules: { 'html-lang-valid': { enabled: false } } })
-      .analyze(function(results) {
+      .analyze()
+      .then(function(results) {
         results.violations.forEach(function(violation) {
           assert.notEqual(violation.id, 'html-lang-valid');
         });

test/integration/shadow-dom.js

@@ -72,7 +72,8 @@ describe('shadow-dom.html', function() {
             region: { enabled: false }
           }
         })
-        .analyze(function(results) {
+        .analyze()
+        .then(function(results) {
           assert.lengthOf(results.violations, 2);
           assert.equal(results.violations[0].id, 'aria-roles');
           assert.equal(results.violations[1].id, 'aria-valid-attr');