Repository: rwjblue/ember-template-lint
Pull request #323 (2017-12-06 21:38:32)

no longer mutate the config
This issue was uncovered in #322. The config was being mutated (even if `require`d). This was causing terrible issues, with the set of recommended rules getting overwritten among others. While changing this, it also uncovered a bunch of incorrect or brittle tests.


Patch:
diff --git a/lib/get-config.js b/lib/get-config.js
index d167e87..0a868c2 100644
--- a/lib/get-config.js
+++ b/lib/get-config.js
@@ -31,23 +31,39 @@ function resolveConfigPath(configPath, pluginPath) {
 
 }
 
-function ensureRootProperties(config) {
-  config.rules = config.rules || {};
-  config.pending = config.pending || [];
-  config.ignore = config.ignore || [];
-  config.plugins = config.plugins || [];
+function ensureRootProperties(config, source, options) {
+  let logger = options.console || console;
+
+  config.rules = assign({}, source.rules || {});
+  config.pending = (source.pending || []).slice();
+  config.ignore = (source.ignore || []).slice();
+
+  let plugins = source.plugins || [];
+  config.plugins = Array.isArray(plugins) ? plugins.slice() : assign({}, plugins);
+
+
+  let extendedList = [];
+  if (source.extends) {
+    if (typeof source.extends === 'string') {
+      extendedList = [source.extends];
+    } else if (Array.isArray(source.extends)) {
+      extendedList = source.extends.slice();
+    } else {
+      logger.log(chalk.yellow('config.extends should be string or array '));
+    }
+  }
+  config.extends = extendedList;
 }
 
-function migrateRulesFromRoot(config, options) {
+function migrateRulesFromRoot(config, source, options) {
   let logger = options.console || console;
 
   let invalidKeysFound = [];
-  for (let key in config) {
+  for (let key in source) {
     if (KNOWN_ROOT_PROPERTIES.indexOf(key) === -1) {
       invalidKeysFound.push(key);
 
-      config.rules[key] = config[key];
-      delete config[key];
+      config.rules[key] = source[key];
     }
   }
 
@@ -156,40 +172,28 @@ function processExtends(config, options) {
 
   let logger = options.console || console;
 
-  if (config.extends) {
+  let extendedList = config.extends;
 
-    let extendedList = [];
-    if (typeof config.extends === 'string') {
-      extendedList = [config.extends];
-    } else if (config.extends instanceof Array) {
-      extendedList = config.extends;
-    } else {
-      logger.log(chalk.yellow('config.extends should be string or array '));
-    }
-
-    for (let i = 0; i < extendedList.length; i++) {
-
-      let extendName = extendedList[i];
-
-      let configuration = config.loadedConfigurations[extendName];
-      if (configuration) {
+  for (let i = 0; i < extendedList.length; i++) {
 
-        if (configuration.rules) {
+    let extendName = extendedList[i];
 
-          config.rules = assign(configuration.rules, config.rules);
+    let configuration = config.loadedConfigurations[extendName];
+    if (configuration) {
 
-        } else {
+      if (configuration.rules) {
 
-          logger.log(chalk.yellow(`Missing rules for extends: ${extendName}`));
-
-        }
+        config.rules = assign({}, configuration.rules, config.rules);
 
       } else {
 
-        logger.log(chalk.yellow(`Cannot find configuration for extends: ${extendName}`));
+        logger.log(chalk.yellow(`Missing rules for extends: ${extendName}`));
 
       }
 
+    } else {
+
+      logger.log(chalk.yellow(`Cannot find configuration for extends: ${extendName}`));
 
     }
 
@@ -218,11 +222,17 @@ function validateRules(config, options) {
 }
 
 module.exports = function(options) {
-  let config = options.config || resolveConfigPath(options.configPath);
+  let source = options.config || resolveConfigPath(options.configPath);
+  let config;
+
+  if (source._processed) {
+    config = source;
+  } else {
+    // don't mutate a `require`d object, you'll have a bad time
+    config = {};
 
-  if (!config._processed) {
-    ensureRootProperties(config);
-    migrateRulesFromRoot(config, options);
+    ensureRootProperties(config, source, options);
+    migrateRulesFromRoot(config, source, options);
 
     processPlugins(config, options);
     processLoadedRules(config);


Test patch:
diff --git a/test/acceptance-test.js b/test/acceptance-test.js
index 9ccdc63..688b5e8 100644
--- a/test/acceptance-test.js
+++ b/test/acceptance-test.js
@@ -31,13 +31,15 @@ describe('public api', function() {
 
   describe('Linter.prototype.loadConfig', function() {
     it('uses provided config', function() {
-      let config = {};
+      let basePath = path.join(fixturePath, 'config-in-root');
+      let expected = require(path.join(basePath, '.template-lintrc'));
+
       let linter = new Linter({
         console: mockConsole,
-        config: config
+        config: expected
       });
 
-      expect(linter.config).to.equal(config);
+      expect(linter.config.rules).to.deep.equal(expected.rules);
     });
 
     it('uses .template-lintrc.js in cwd if present', function() {
@@ -50,7 +52,7 @@ describe('public api', function() {
         console: mockConsole
       });
 
-      expect(linter.config).to.equal(expected);
+      expect(linter.config.rules).to.deep.equal(expected.rules);
     });
 
     it('uses .template-lintrc in provided configPath', function() {
@@ -65,7 +67,7 @@ describe('public api', function() {
         configPath: configPath
       });
 
-      expect(linter.config).to.equal(expected);
+      expect(linter.config.rules).to.deep.equal(expected.rules);
     });
   });
 
diff --git a/test/unit/get-config-test.js b/test/unit/get-config-test.js
index 2fe3179..03cb0a0 100644
--- a/test/unit/get-config-test.js
+++ b/test/unit/get-config-test.js
@@ -2,7 +2,6 @@
 
 const path = require('path');
 const expect = require('chai').expect;
-const clone = require('lodash').clone;
 const getConfig = require('../../lib/get-config');
 const recommendedConfig = require('../../lib/config/recommended');
 
@@ -15,10 +14,12 @@ describe('get-config', function() {
   });
 
   it('if config is provided, it is returned', function() {
-    let expected = { };
+    let basePath = path.join(fixturePath, 'config-in-root');
+    let expected = require(path.join(basePath, '.template-lintrc'));
+
     let actual = getConfig({ config: expected });
 
-    expect(actual).to.equal(expected);
+    expect(actual.rules).to.deep.equal(expected.rules);
   });
 
   it('uses .template-lintrc.js in cwd if present', function() {
@@ -29,7 +30,7 @@ describe('get-config', function() {
 
     let actual = getConfig({});
 
-    expect(actual).to.deep.equal(expected);
+    expect(actual.rules).to.deep.equal(expected.rules);
   });
 
   it('uses .template-lintrc in provided configPath', function() {
@@ -43,7 +44,7 @@ describe('get-config', function() {
       configPath: configPath
     });
 
-    expect(actual).to.deep.equal(expected);
+    expect(actual.rules).to.deep.equal(expected.rules);
   });
 
   it('can specify that it extends a default configuration', function() {
@@ -57,19 +58,19 @@ describe('get-config', function() {
   });
 
   it('can extend and override a default configuration', function() {
-    let expected = clone(recommendedConfig);
-    expected.rules['bare-strings'] = true;
+    let expected = recommendedConfig;
 
     let actual = getConfig({
       config: {
         extends: 'recommended',
         rules: {
-          'bare-strings': false
+          'block-indentation': 4
         }
       }
     });
 
-    expect(actual.rules['bare-strings']).to.be.false;
+    expect(actual.rules['block-indentation']).to.equal(4);
+    expect(actual.rules['block-indentation']).to.not.equal(expected.rules['block-indentation']);
   });
 
   it('migrates rules in the config root into `rules` property', function() {
@@ -240,4 +241,18 @@ describe('get-config', function() {
     expect(secondMessage).to.not.be.ok;
   });
 
+  it('does no mutate the config', function() {
+    let config = {
+      config: {
+        extends: 'recommended'
+      }
+    };
+
+    let cloned = JSON.parse(JSON.stringify(config));
+
+    let actual = getConfig(config);
+
+    expect(Object.keys(actual.rules), 'make sure the operation would have resulted in a mutated object').to.not.equal(0);
+    expect(config, 'assert object matches its original clone').to.deep.equal(cloned);
+  });
 });