Fix errors and improve error messaging (#158)

* updated error messaging. Fixes for issues with references. * adding in didoo's test from #118 * cleanup of terminology * fixed resolveObject to correctly replace multiple references. modified testing suite to reflect new test. * updates per comments by didoo and dbanksdesign * case sensitive, oops. * case sensitive, oops. * minor updates based on PR feedback * merging with develop to ensure we stay synched * removing cli error handling and moving to module * removing per dannys comments * making constants for group errors per Dannys comments * switch to error grouping mindset and naming * switch to error grouping mindset and naming * per danny's comment * fix flush to execute across all groups if called with no group; remove flush on uncaught exceptions to prevent confusion * simplify, simplify, simplify * changed out error naming to message mindset, cleaned out console.log, fixed issues with simplified GroupMessages * sepearate circular reference tests into separate expects * avoid using string so we dont get it confused with String
amzn · Oct 24, 2018 · f5c0486 · f5c0486
1 parent 83fc888
commit f5c0486
Show file tree

Hide file tree

Showing 13 changed files with 234 additions and 88 deletions.
diff --git a/__tests__/__json_files/circular.json b/__tests__/__json_files/circular.json
@@ -1,7 +1,6 @@
 {
-
   "a": "{b}",
   "b": "{c}",
   "c": "{d}",
   "d": "{a}"
-}
+}
diff --git a/__tests__/__json_files/circular_2.json b/__tests__/__json_files/circular_2.json
@@ -1,8 +1,8 @@
 {
   "a": {
     "b": {
-      "c": "{d}"
+      "c": "{j}"
     }
   },
-  "d": "{a.b.c}"
-}
+  "j": "{a.b.c}"
+}
diff --git a/__tests__/__json_files/circular_3.json b/__tests__/__json_files/circular_3.json
@@ -1,12 +1,10 @@
 {
   "a": {
-    "b": {
-      "c": "{d.e.f}"
-    }
+    "b": "{c.d.e}"
   },
-  "d": {
-    "e": {
-      "f": "{a.b.c}"
+  "c": {
+    "d": {
+      "e": "{a.b}"
     }
   }
-}
+}
diff --git a/__tests__/__json_files/circular_4.json b/__tests__/__json_files/circular_4.json
@@ -1,15 +1,17 @@
 {
   "a": {
     "b": {
-      "c": "{d.e.f}"
+      "c": {
+        "d": "{e.f.g}"
+      }
     }
   },
-  "d": {
-    "e": {
-      "f": "{g.h}"
+  "e": {
+    "f": {
+      "g": "{h.i}"
     }
   },
-  "g": {
-    "h": "{a.b.c}"
+  "h": {
+    "i": "{a.b.c.d}"
   }
-}
+}
diff --git a/__tests__/__json_files/circular_5.json b/__tests__/__json_files/circular_5.json
@@ -0,0 +1,6 @@
+{
+  "k": "{l}",
+  "l": "{m}",
+  "m": "{l}",
+  "n": "{k}"
+}
diff --git a/__tests__/__json_files/not_circular.json b/__tests__/__json_files/not_circular.json
@@ -0,0 +1,8 @@
+{
+  "prop1" : { "value": "test1 value" },
+  "prop2" : { "value": "test2 value" },
+  "prop3" : { "value": "{prop1.value}" },
+  "prop4" : { "value": "{prop3.value}" },
+  "prop12" : { "value": "{prop1.value}, {prop2.value} and some extra stuff" },
+  "prop124" : { "value": "{prop1.value}, {prop2.value} and {prop4.value}" }
+}
diff --git a/__tests__/extend.test.js b/__tests__/extend.test.js
@@ -131,7 +131,7 @@ describe('extend', () => {
         source: [__dirname + "/__properties/paddings.json", __dirname + "/__properties/paddings.json"],
         log: 'error'
       })
-    ).toThrow('Collision detected at:');
+    ).toThrow('Collisions detected');
   });
 
   it('should throw a warning if the collision is in source files and log is set to warn', () => {

diff --git a/__tests__/utils/resolveObject.test.js b/__tests__/utils/resolveObject.test.js
@@ -13,6 +13,9 @@
 
 var resolveObject = require('../../lib/utils/resolveObject');
 var helpers = require('../__helpers');
+var GroupMessages = require('../../lib/utils/groupMessages');
+
+var PROPERTY_REFERENCE_WARNINGS = GroupMessages.GROUP.PropertyReferenceWarnings;
 
 describe('utils', () => {
   describe('resolveObject', () => {
@@ -113,27 +116,69 @@ describe('utils', () => {
       ).toThrow();
     });
 
-    it('should gracefully handle circular references', () => {
-      expect(
-        resolveObject.bind(null,
-          helpers.fileToJSON(__dirname + '/../__json_files/circular.json')
-        )
-      ).toThrow('Circular definition: a | d');
-      expect(
-        resolveObject.bind(null,
-          helpers.fileToJSON(__dirname + '/../__json_files/circular_2.json')
-        )
-      ).toThrow('Circular definition: a.b.c | d');
-      expect(
-        resolveObject.bind(null,
-          helpers.fileToJSON(__dirname + '/../__json_files/circular_3.json')
-        )
-      ).toThrow('Circular definition: a.b.c | d.e.f');
-      expect(
-        resolveObject.bind(null,
-          helpers.fileToJSON(__dirname + '/../__json_files/circular_4.json')
-        )
-      ).toThrow('Circular definition: a.b.c | g.h');
+    it('should gracefully handle basic circular references', () => {
+      GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS);
+
+      resolveObject(helpers.fileToJSON(__dirname + '/../__json_files/circular.json'));
+      expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).toBe(1);
+      expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS))).toBe(JSON.stringify([
+         'Circular definition cycle:  a, b, c, d, a'
+      ]));
+    });
+
+    it('should gracefully handle basic and nested circular references', () => {
+      GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS);
+
+      resolveObject(helpers.fileToJSON(__dirname + '/../__json_files/circular_2.json'));
+      expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).toBe(1);
+      expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS))).toBe(JSON.stringify([
+        'Circular definition cycle:  a.b.c, j, a.b.c'
+      ]));
+    });
+
+    it('should gracefully handle nested circular references', () => {
+      GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS);
+
+      resolveObject(helpers.fileToJSON(__dirname + '/../__json_files/circular_3.json'));
+      expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).toBe(1);
+      expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS))).toBe(JSON.stringify([
+        'Circular definition cycle:  a.b, c.d.e, a.b'
+      ]));
+    });
+
+    it('should gracefully handle multiple nested circular references', () => {
+      GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS);
+
+      resolveObject(helpers.fileToJSON(__dirname + '/../__json_files/circular_4.json'));
+      expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).toBe(1);
+      expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS))).toBe(JSON.stringify([
+        'Circular definition cycle:  a.b.c.d, e.f.g, h.i, a.b.c.d',
+      ]));
+    });
+
+    it('should gracefully handle down-chain circular references', () => {
+      GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS);
+
+      resolveObject(helpers.fileToJSON(__dirname + '/../__json_files/circular_5.json'));
+      expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).toBe(1);
+      expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS))).toBe(JSON.stringify([
+        'Circular definition cycle:  l, m, l',
+      ]));
+    });
+
+    it('should correctly replace multiple references without reference errors', function() {
+      GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS);
+
+      var obj = resolveObject(helpers.fileToJSON(__dirname + '/../__json_files/not_circular.json'));
+      expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).toBe(0);
+      expect(JSON.stringify(obj)).toBe(JSON.stringify({
+        prop1: { value: 'test1 value' },
+        prop2: { value: 'test2 value' },
+        prop3: { value: 'test1 value' },
+        prop4: { value: 'test1 value' },
+        prop12: { value: 'test1 value, test2 value and some extra stuff' },
+        prop124: { value: 'test1 value, test2 value and test1 value' }
+      }));
     });
 
     describe('ignoreKeys', () => {
@@ -238,11 +283,15 @@ describe('utils', () => {
     });
 
     it('should collect multiple reference errors', () => {
-      expect(
-        resolveObject.bind(null,
-          helpers.fileToJSON(__dirname + '/../__json_files/multiple_reference_errors.json')
-        )
-      ).toThrow('Failed due to 3 errors:');
+      GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS);
+
+      resolveObject(helpers.fileToJSON(__dirname + '/../__json_files/multiple_reference_errors.json'));
+      expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).toBe(3);
+      expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS))).toBe(JSON.stringify([
+         "Reference doesn't exist: a.b tries to reference b.a, which is not defined",
+         "Reference doesn't exist: a.c tries to reference b.c, which is not defined",
+         "Reference doesn't exist: a.d tries to reference d, which is not defined"
+      ]));
     });
 
   });

diff --git a/lib/exportPlatform.js b/lib/exportPlatform.js
@@ -13,7 +13,10 @@
 
 var resolveObject   = require('./utils/resolveObject'),
     transformObject = require('./transform/object'),
-    transformConfig = require('./transform/config');
+    transformConfig = require('./transform/config'),
+    GroupMessages = require('./utils/groupMessages');
+
+var PROPERTY_REFERENCE_WARNINGS = GroupMessages.GROUP.PropertyReferenceWarnings;
 
 /**
  * Exports a properties object with applied
@@ -41,9 +44,17 @@ function exportPlatform(platform) {
   // values like "1px solid {color.border.base}" we want to
   // transform the original value (color.border.base) before
   // replacing that value in the string.
-  return resolveObject(
+  var to_ret = resolveObject(
     transformObject(this.properties, platformConfig)
   );
+
+  if(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS) > 0) {
+    var warnings = GroupMessages.flush(PROPERTY_REFERENCE_WARNINGS).join('\n');
+    console.log(`\n${PROPERTY_REFERENCE_WARNINGS}:\n${warnings}\n\n`);
+    throw new Error('Problems were found when trying to resolve property references');
+  }
+
+  return to_ret;
 }
 
 

diff --git a/lib/extend.js b/lib/extend.js
@@ -17,7 +17,10 @@ var combineJSON = require('./utils/combineJSON'),
     deepExtend = require('./utils/deepExtend'),
     resolveCwd = require('resolve-cwd'),
     _ = require('lodash'),
-    chalk = require('chalk');
+    chalk = require('chalk'),
+    GroupMessages = require('./utils/groupMessages');
+
+var PROPERTY_VALUE_COLLISIONS = GroupMessages.GROUP.PropertyValueCollisions;
 
 /**
  * Either a string to a JSON file that contains configuration for the style dictionary or a plain Javascript object
@@ -113,18 +116,23 @@ function extend(opts) {
     if (!_.isArray(options.source))
       throw new Error('source must be an array');
 
-    var props = combineJSON(options.source, true, function Collision(opts) {
-      if (options.log) {
-        var str = `Collision detected at: ${opts.path.join('.')}! Original value: ${opts.target[opts.key]}, New value: ${opts.copy[opts.key]}`;
-        if (options.log === 'warn') {
-          console.warn(chalk.keyword('orange')(str));
-        } else if (options.log === 'error') {
-          throw new Error(str);
-        }
+    var props = combineJSON(options.source, true, function Collision(prop) {
+      GroupMessages.add(
+        PROPERTY_VALUE_COLLISIONS,
+        `Collision detected at: ${prop.path.join('.')}! Original value: ${prop.target[prop.key]}, New value: ${prop.copy[prop.key]}`
+      );
+    });
+
+    if(GroupMessages.count(PROPERTY_VALUE_COLLISIONS) > 0) {
+      var collisions = GroupMessages.flush(PROPERTY_VALUE_COLLISIONS).join('\n');
+      console.log(`\n${PROPERTY_VALUE_COLLISIONS}:\n${collisions}\n\n`);
+      if (options.log === 'error') {
+        throw new Error('Collisions detected');
       }
+    }
 
-    });
     to_ret.properties = deepExtend([{}, to_ret.properties, props]);
+
     to_ret.source = null; // We don't want to carry over the source references
   }
 

diff --git a/lib/utils/deepExtend.js b/lib/utils/deepExtend.js
@@ -62,10 +62,11 @@ function deepExtend(objects, collision, path) {
             clone = src && _.isPlainObject(src) ? src : {};
           }
 
-          path.push(name);
+          var nextPath = path.slice(0);
+          nextPath.push(name);
 
           // Never move original objects, clone them
-          target[ name ] = deepExtend( [clone, copy], collision, path );
+          target[ name ] = deepExtend( [clone, copy], collision, nextPath );
 
           // Don't bring in undefined values
         } else if ( copy !== undefined ) {
@@ -74,7 +75,6 @@ function deepExtend(objects, collision, path) {
           }
           target[ name ] = copy;
         }
-        path.pop();
       }
     }
   }

diff --git a/lib/utils/groupMessages.js b/lib/utils/groupMessages.js
@@ -0,0 +1,49 @@
+/*
+ * Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance with
+ * the License. A copy of the License is located at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
+ * CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+var groupedMessages = {};
+
+var GroupMessages = {
+  GROUP: {
+    PropertyReferenceWarnings: 'Property Reference Errors',
+    PropertyValueCollisions: 'Property Value Collisions',
+  },
+
+  flush: function (messageGroup) {
+    var messages = GroupMessages.fetchMessages(messageGroup);
+    GroupMessages.clear(messageGroup);
+    return messages;
+  },
+
+  add: function (messageGroup, message) {
+    if(messageGroup) {
+      if(!groupedMessages[messageGroup]) {
+        groupedMessages[messageGroup] = [];
+      }
+      groupedMessages[messageGroup].push(message);
+    }
+  },
+
+  count: function (messageGroup) {
+    return groupedMessages[messageGroup] ? groupedMessages[messageGroup].length : 0;
+  },
+
+  fetchMessages: function (messageGroup) {
+    return (messageGroup && groupedMessages[messageGroup]) || [];
+  },
+
+  clear: function (messageGroup) {
+    messageGroup && groupedMessages[messageGroup] && delete groupedMessages[messageGroup];
+  }
+};
+
+module.exports = GroupMessages;