fix: throw better error when failing to parse json (#304)

* test: be more specific about expected exception

* test: cover error cases for JSON.parse in configLoader

* fix: throw better error when failing to parse json

Fixes #266

Author: marionebl

package.json

@@ -77,6 +77,7 @@
     "inquirer": "1.1.2",
     "lodash": "4.14.1",
     "minimist": "1.2.0",
+    "path-exists": "2.1.0",
     "shelljs": "0.5.3",
     "strip-json-comments": "2.0.1"
   },

src/configLoader/getContent.js

@@ -1,47 +1,63 @@
 import fs from 'fs';
 import path from 'path';
+import {sync as existsSync} from 'path-exists';
 import stripJSONComments from 'strip-json-comments';
 
 import {getNormalizedConfig} from '../configLoader';
 
-export default getContent;
+export default getConfigContent;
+
+/**
+ * Read the content of a configuration file
+ * - if not js or json: strip any comments
+ * - if js or json: require it
+ * @param {String} configPath - full path to configuration file
+ * @return {Object}
+ */
+function readConfigContent(configPath) {
+    const parsedPath = path.parse(configPath)
+    const isRcFile = parsedPath.ext !== '.js' && parsedPath.ext !== '.json';
+    const jsonString = fs.readFileSync(configPath, 'utf-8');
+    const parse = isRcFile ?
+      (contents) => JSON.parse(stripJSONComments(contents)) :
+      (contents) => JSON.parse(contents);
+
+    try {
+        const parsed = parse(jsonString);
+
+        Object.defineProperty(parsed, 'configPath', {
+          value: configPath
+        });
+
+        return parsed;
+    } catch (error) {
+        error.message = [
+          `Parsing JSON at ${configPath} for commitizen config failed:`,
+          error.mesasge
+        ].join('\n');
+
+        throw error;
+    }
+}
 
 /**
  * Get content of the configuration file
- * @param {String} config - partial path to configuration file
+ * @param {String} configPath - partial path to configuration file
  * @param {String} directory - directory path which will be joined with config argument
  * @return {Object}
  */
-function getContent(config, directory) {
-    if (!config) {
-        return;
+function getConfigContent(configPath, baseDirectory) {
+    if (!configPath) {
+      return;
     }
 
-    var configPath = path.resolve(directory, config);
-    var ext;
-    var content;
-
-    config = path.basename(config);
-
-    if (fs.existsSync(configPath)) {
-        ext = path.extname(configPath);
-
-        if (ext === '.js' || ext === '.json') {
-            content = JSON.parse(fs.readFileSync(configPath, 'utf8'));
-        } else {
-            content = JSON.parse(
-                stripJSONComments(
-                    fs.readFileSync(configPath, 'utf8')
-                )
-            );
-        }
-
-        // Adding property via Object.defineProperty makes it
-        // non-enumerable and avoids warning for unsupported rules
-        Object.defineProperty(content, 'configPath', {
-            value: configPath
-        });
+    const resolvedPath = path.resolve(baseDirectory, configPath);
+    const configBasename = path.basename(resolvedPath);
+
+    if (!existsSync(resolvedPath)) {
+      return getNormalizedConfig(resolvedPath);
     }
-    return getNormalizedConfig(config, content);
-    
-};
+
+    const content = readConfigContent(resolvedPath);
+    return getNormalizedConfig(configBasename, content);
+};

test/fixtures/invalid-json-rc

@@ -0,0 +1,4 @@
+{
+  "some": "json"
+  "that": "is invalid"
+}

test/fixtures/invalid-json.json

@@ -0,0 +1,4 @@
+{
+  "some": "json"
+  "that": "is invalid"
+}

test/fixtures/valid-json-rc

@@ -0,0 +1,4 @@
+{
+  "some": "json"
+  // with comments
+}

test/tests/configLoader.js

@@ -1,8 +1,23 @@
+import path from 'path';
 import {expect} from 'chai';
-import {getNormalizedConfig} from '../../src/configLoader';
+import {getContent, getNormalizedConfig} from '../../src/configLoader';
+
+const fixturesPath = path.resolve(__dirname, '..', 'fixtures');
 
 describe('configLoader', function() {
 
+  it('errors appropriately for invalid json', function() {
+    expect(() => getContent('invalid-json.json', fixturesPath))
+      .to.throw(/parsing json at/i);
+    expect(() => getContent('invalid-json-rc', fixturesPath))
+      .to.throw(/parsing json at/i);
+  });
+
+  it('parses json files with comments', function() {
+    expect(getContent('valid-json-rc', fixturesPath))
+      .to.deep.equal({'some': 'json'});
+  });
+
   it('normalizes package.json configs', function() {
 
     let config = 'package.json';
@@ -46,4 +61,4 @@ describe('configLoader', function() {
 
   });
 
-});
+});

test/tests/init.js

@@ -35,7 +35,7 @@ describe('init', function() {
     // Install an adapter
     commitizenInit(sh, config.paths.endUserRepo, 'cz-conventional-changelog');
 
-    // TEST 
+    // TEST
 
     // Check resulting json
     let packageJson = util.getParsedPackageJsonFromPath(config.paths.endUserRepo);
@@ -70,12 +70,12 @@ describe('init', function() {
     sh.cd(config.paths.endUserRepo);
     commitizenInit(sh, config.paths.endUserRepo, 'cz-conventional-changelog', { saveDev:true });
 
-    // TEST 
-    sh.cd(config.paths.endUserRepo); 
+    // TEST
+    sh.cd(config.paths.endUserRepo);
     // Adding a second adapter
     expect(function() {
       commitizenInit(sh, config.paths.endUserRepo, 'cz-jira-smart-commit', { saveDev:true });
-    }).to.throw();
+    }).to.throw(/already configured/);
 
     // Check resulting json
     let packageJson = util.getParsedPackageJsonFromPath(config.paths.endUserRepo);
@@ -92,7 +92,7 @@ describe('init', function() {
     // SETUP
 
     // Add a first adapter
-    sh.cd(config.paths.endUserRepo); 
+    sh.cd(config.paths.endUserRepo);
     commitizenInit(sh, config.paths.endUserRepo, 'cz-conventional-changelog', { saveDev:true });
 
     // TEST
@@ -115,7 +115,7 @@ describe('init', function() {
     // SETUP
 
     // Add a first adapter
-    sh.cd(config.paths.endUserRepo); 
+    sh.cd(config.paths.endUserRepo);
     commitizenInit(sh, config.paths.endUserRepo, 'cz-conventional-changelog');
     let packageJson = util.getParsedPackageJsonFromPath(config.paths.endUserRepo);
 
@@ -129,7 +129,7 @@ describe('init', function() {
     // // But you CAN NOT increment a range
     // expect(semver.inc(range, 'major')).to.equal(null);
     // TODO: We need to figure out how to check if the repo has save exact set
-    // in the config before we can re-enable this. The --save-exact setting 
+    // in the config before we can re-enable this. The --save-exact setting
     // in our package.json breaks this test
 
   });
@@ -141,7 +141,7 @@ describe('init', function() {
     // SETUP
 
     // Add a first adapter
-    sh.cd(config.paths.endUserRepo); 
+    sh.cd(config.paths.endUserRepo);
     commitizenInit(sh, config.paths.endUserRepo, 'cz-conventional-changelog', {saveExact: true});
     let packageJson = util.getParsedPackageJsonFromPath(config.paths.endUserRepo);
 
@@ -161,10 +161,10 @@ describe('init', function() {
 afterEach(function() {
   // All this should do is archive the tmp path to the artifacts
   clean.afterEach(sh, config.paths.tmp, config.preserve);
-}); 
+});
 
 after(function() {
-  // Once everything is done, the artifacts should be cleaned up based on 
+  // Once everything is done, the artifacts should be cleaned up based on
   // the preserve setting in the config
   clean.after(sh, config.paths.tmp, config.preserve);
-}); 
+});