diff --git a/__tests__/__json_files/circular.json b/__tests__/__json_files/circular.json index 83ffb53d5..540f97c6b 100644 --- a/__tests__/__json_files/circular.json +++ b/__tests__/__json_files/circular.json @@ -1,7 +1,6 @@ { - "a": "{b}", "b": "{c}", "c": "{d}", "d": "{a}" -} \ No newline at end of file +} diff --git a/__tests__/__json_files/circular_2.json b/__tests__/__json_files/circular_2.json index 1e784fcc9..7bae1c8f9 100644 --- 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}" -} \ No newline at end of file + "j": "{a.b.c}" +} diff --git a/__tests__/__json_files/circular_3.json b/__tests__/__json_files/circular_3.json index 207408880..3f7c529cd 100644 --- 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}" } } -} \ No newline at end of file +} diff --git a/__tests__/__json_files/circular_4.json b/__tests__/__json_files/circular_4.json index a6aa01e11..ada52a853 100644 --- 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}" } -} \ No newline at end of file +} diff --git a/__tests__/__json_files/circular_5.json b/__tests__/__json_files/circular_5.json new file mode 100644 index 000000000..f1788ea9c --- /dev/null +++ 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 new file mode 100644 index 000000000..2d7950e6c --- /dev/null +++ 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 index 5f3c92cdf..8c5765f65 100644 --- 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 index c07d28323..de2197786 100644 --- 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 index 65b5b4346..f2d347de7 100644 --- 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 index d46231916..aa271e8a1 100644 --- 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 index 3c8dc7194..6f7555227 100644 --- 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 new file mode 100644 index 000000000..cd24bc82a --- /dev/null +++ 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; diff --git a/lib/utils/resolveObject.js b/lib/utils/resolveObject.js index 23c6f6ab6..a653f874a 100644 --- a/lib/utils/resolveObject.js +++ b/lib/utils/resolveObject.js @@ -11,7 +11,10 @@ * and limitations under the License. */ -var _ = require('lodash'); +var _ = require('lodash'), + GroupMessages = require('./groupMessages'); + +var PROPERTY_REFERENCE_WARNINGS = GroupMessages.GROUP.PropertyReferenceWarnings; var current_context = []; // To maintain the context so we can test for circular definitions var defaults = { @@ -50,7 +53,6 @@ function resolveObject(object, opts) { function traverseObj(obj) { var key; - var errors = []; for (key in obj) { // We want to check for ignoredKeys, this is so @@ -62,34 +64,27 @@ function traverseObj(obj) { traverseObj( obj[key] ); } else { if (typeof obj[key] === 'string' && obj[key].indexOf('{') > -1) { - try { - obj[key] = compile_value( obj[key], [key] ); - } - catch (e) { - errors.push(e); - } + obj[key] = compile_value( obj[key], [current_context.join('.')] ); } } current_context.pop(); } } - if(errors.length) { - throw new Error(errors.length===1 ? errors[0] : 'Failed due to '+errors.length+' errors:\n' + errors.join('\n').replace(/Error: /g,'\t')); - } - return obj; } - +var foundCirc = {}; function compile_value(value, stack) { + var to_ret, ref; - stack.push(value.slice(1, -1)); // Replace the reference inline, but don't replace the whole string because // references can be part of the value such as "1px solid {color.border.light}" value.replace(regex, function(match, variable) { + stack.push(variable); + // Find what the value is referencing ref = selfRef(variable.trim(), updated_object); @@ -100,9 +95,32 @@ function compile_value(value, stack) { // Recursive so we can compute multi-layer variables like a = b, b = c, so a = c if ( to_ret.indexOf('{') > -1 ) { var reference = to_ret.slice(1, -1); - if(stack.indexOf(reference)!==-1) { - stack.push(reference); - throw new Error('Variable reference for \'' + stack[0] + '\' resolves to circular definition cycle of \'' + reference + '\': ' + stack.join(', ') ); + + // Compare to found circular references + if (foundCirc.hasOwnProperty(reference)) { + // If the current reference is a member of a circular reference, do nothing + } + else if(stack.indexOf(reference)!==-1) { + // If the current stack already contains the current reference, we found a new circular reference + // chop down to just the circular part, save it to our circular reference info, and spit out an error + + // Get the position of the existing reference in the stack + var stackIndexReference = stack.indexOf(reference); + + // Get the portion of the stack that starts at the circular reference and brings you through until the end + var circStack = stack.slice(stackIndexReference); + + // For all the references in this list, add them to the list of references that end up in a circular reference + circStack.forEach(function(key) { foundCirc[key] = true; }); + + // Add our found circular reference to the end of the cycle + circStack.push(reference); + + // Add circ reference info to our list of warning messages + GroupMessages.add( + PROPERTY_REFERENCE_WARNINGS, + "Circular definition cycle: " + circStack.join(', ') + ); } else { to_ret = compile_value( to_ret, stack ); @@ -116,6 +134,7 @@ function compile_value(value, stack) { // Leave the circular reference unchanged to_ret = value; } + stack.pop(variable); return to_ret; }); @@ -124,10 +143,10 @@ function compile_value(value, stack) { } -function selfRef(string, obj) { +function selfRef(str, obj) { var i, ref=obj, - array = string.split(options.separator), + array = str.split(options.separator), context = current_context.join(options.separator); for (i = 0; i < array.length; i++) { @@ -142,16 +161,13 @@ function selfRef(string, obj) { // If the reference doesn't change then it means // we didn't find it in the object if (ref === obj) { - throw new Error('Reference doesn\'t exist: ' + context + ' tries to reference ' + string + ', which is not defined'); + GroupMessages.add( + PROPERTY_REFERENCE_WARNINGS, + "Reference doesn't exist: " + context + " tries to reference " + str + ", which is not defined" + ); } - var test = options.opening_character + context + options.closing_character; - - if (typeof ref === 'string' && ref.indexOf(test) > -1) { - throw new Error('Circular definition: ' + context + ' | ' + string); - } else { - return ref; - } + return ref; }