From 3cf41fa6d0d04ae07da05fe505b02d03e976b7c7 Mon Sep 17 00:00:00 2001 From: Charles Dorner Date: Fri, 17 May 2019 12:44:48 -0700 Subject: [PATCH 1/7] first cut at catching property name collisions during file output --- examples/basic/properties/color/base.json | 2 + lib/buildFile.js | 50 +++++++++++++++++++++-- lib/transform/config.js | 1 + lib/utils/groupMessages.js | 1 + 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/examples/basic/properties/color/base.json b/examples/basic/properties/color/base.json index 02a99d700..dec0abbc8 100644 --- a/examples/basic/properties/color/base.json +++ b/examples/basic/properties/color/base.json @@ -3,7 +3,9 @@ "base": { "gray": { "light" : { "value": "#CCCCCC" }, + "Light" : { "value": "#DDD" }, "medium": { "value": "#999999" }, + "Medium": { "value": "#aaaaaa" }, "dark" : { "value": "#111111" } }, "red": { "value": "#FF0000" }, diff --git a/lib/buildFile.js b/lib/buildFile.js index 10b4c6d22..d408d9cac 100644 --- a/lib/buildFile.js +++ b/lib/buildFile.js @@ -14,7 +14,8 @@ var path = require('path'), fs = require('fs-extra'), chalk = require('chalk'), - filterProperties = require('./filterProperties'); + filterProperties = require('./filterProperties'), + GroupMessages = require('./utils/groupMessages'); /** * Takes the style property object and a format and returns a @@ -33,6 +34,8 @@ function buildFile(destination, format, platform, dictionary, filter) { if (!destination || typeof destination !== 'string') throw new Error('Please enter a valid destination'); + var dest = destination; + // if there is a build path, prepend the destination with it if (platform.buildPath) { destination = platform.buildPath + destination; @@ -42,8 +45,49 @@ function buildFile(destination, format, platform, dictionary, filter) { if (!fs.existsSync(dirname)) fs.mkdirsSync(dirname); - fs.writeFileSync(destination, format(filterProperties(dictionary, filter), platform)); - console.log(chalk.bold.green('✔︎') + ' ' + destination); + let filteredProperties = filterProperties(dictionary, filter); + + // Check for property name Collisions + var nameCollisionObj = {}; + filteredProperties.allProperties && filteredProperties.allProperties.forEach((propertyData) => { + let propertyName = propertyData.name; + if(!nameCollisionObj[propertyName]) { + nameCollisionObj[propertyName] = []; + } + nameCollisionObj[propertyName].push(propertyData); + }); + + var PROPERTY_NAME_COLLISION_WARNINGS = GroupMessages.GROUP.PropertyNameCollisionWarnings; + var WARNING_COLOR = 'darkorange'; + var HIGHLIGHT_COLOR = 'orangered'; + var HELP_COLOR = 'orange'; + Object.keys(nameCollisionObj).map((propertyName) => { + if(nameCollisionObj[propertyName].length > 1) { + let collisions = nameCollisionObj[propertyName].map(properties => chalk.keyword(HIGHLIGHT_COLOR)(properties.path.join('.')) + ' ' + chalk.keyword(WARNING_COLOR)(properties.value)).join('\n '); + GroupMessages.add( + PROPERTY_NAME_COLLISION_WARNINGS, + `Output name ${chalk.keyword(HIGHLIGHT_COLOR).bold(propertyName)} was generated by:\n ${collisions}` + ); + } + }); + + let propertyNamesCollisionCount = GroupMessages.count(PROPERTY_NAME_COLLISION_WARNINGS); + + fs.writeFileSync(destination, format(filteredProperties, platform)); + console.log(chalk.bold.green((propertyNamesCollisionCount > 0) ? '⚠️ ' : '✔︎ ') + ' ' + destination); + + if(propertyNamesCollisionCount > 0) { + var propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n '); + let title = `While building ${chalk.keyword(HIGHLIGHT_COLOR).bold(dest)}, token collisions were found; output may be unexpected.`; + let help = chalk.keyword(HELP_COLOR)([ + 'This many-to-one issue is usually caused by some combination of:', + '* conflicting or similar paths/names in property definitions', + '* platform transforms/transformGroups affecting names, especially when removing specificity', + '* overly inclusive file filters', + ].join('\n ')); + let warn = `${title}\n ${propertyNamesCollisionWarnings}\n${help}`; + console.log(chalk.keyword(WARNING_COLOR).bold(warn)); + } } diff --git a/lib/transform/config.js b/lib/transform/config.js index 9cd89d252..e36b14456 100644 --- a/lib/transform/config.js +++ b/lib/transform/config.js @@ -29,6 +29,7 @@ var MISSING_TRANSFORM_ERRORS = GroupMessages.GROUP.MissingRegisterTransformError */ function transformConfig(config, dictionary, platformName) { var to_ret = _.clone(config); + to_ret.platformName = platformName; // The platform can define either a transformGroup or an array // of transforms. If given a transformGroup that doesn't exist, diff --git a/lib/utils/groupMessages.js b/lib/utils/groupMessages.js index 4c51ebb44..78f2e5b35 100644 --- a/lib/utils/groupMessages.js +++ b/lib/utils/groupMessages.js @@ -20,6 +20,7 @@ var GroupMessages = { RegisterTemplateDeprecationWarnings: 'Register Template Deprecation Warnings', SassMapFormatDeprecationWarnings: 'Sass Map Format Deprecation Warnings', MissingRegisterTransformErrors: 'Missing Register Transform Errors', + PropertyNameCollisionWarnings: 'Property Name Collision Warnings', }, flush: function (messageGroup) { From 382783779756e004e7dc2eae1736626b85fb0c6c Mon Sep 17 00:00:00 2001 From: Charles Dorner Date: Mon, 20 May 2019 10:47:26 -0700 Subject: [PATCH 2/7] multiple revisions based on PR comments --- lib/buildFile.js | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/buildFile.js b/lib/buildFile.js index d408d9cac..26ef370e6 100644 --- a/lib/buildFile.js +++ b/lib/buildFile.js @@ -34,18 +34,18 @@ function buildFile(destination, format, platform, dictionary, filter) { if (!destination || typeof destination !== 'string') throw new Error('Please enter a valid destination'); - var dest = destination; + var fullDestination = destination; - // if there is a build path, prepend the destination with it + // if there is a build path, prepend the full destination with it if (platform.buildPath) { - destination = platform.buildPath + destination; + fullDestination = platform.buildPath + fullDestination; } - var dirname = path.dirname(destination); + var dirname = path.dirname(fullDestination); if (!fs.existsSync(dirname)) fs.mkdirsSync(dirname); - let filteredProperties = filterProperties(dictionary, filter); + var filteredProperties = filterProperties(dictionary, filter); // Check for property name Collisions var nameCollisionObj = {}; @@ -61,9 +61,13 @@ function buildFile(destination, format, platform, dictionary, filter) { var WARNING_COLOR = 'darkorange'; var HIGHLIGHT_COLOR = 'orangered'; var HELP_COLOR = 'orange'; - Object.keys(nameCollisionObj).map((propertyName) => { + Object.keys(nameCollisionObj).forEach((propertyName) => { if(nameCollisionObj[propertyName].length > 1) { - let collisions = nameCollisionObj[propertyName].map(properties => chalk.keyword(HIGHLIGHT_COLOR)(properties.path.join('.')) + ' ' + chalk.keyword(WARNING_COLOR)(properties.value)).join('\n '); + let collisions = nameCollisionObj[propertyName].map((properties) => { + let propertyPathText = chalk.keyword(HIGHLIGHT_COLOR)(properties.path.join('.')); + let valueText = chalk.keyword(WARNING_COLOR)(properties.value); + return propertyPathText + ' ' + valueText; + }).join('\n '); GroupMessages.add( PROPERTY_NAME_COLLISION_WARNINGS, `Output name ${chalk.keyword(HIGHLIGHT_COLOR).bold(propertyName)} was generated by:\n ${collisions}` @@ -73,12 +77,12 @@ function buildFile(destination, format, platform, dictionary, filter) { let propertyNamesCollisionCount = GroupMessages.count(PROPERTY_NAME_COLLISION_WARNINGS); - fs.writeFileSync(destination, format(filteredProperties, platform)); - console.log(chalk.bold.green((propertyNamesCollisionCount > 0) ? '⚠️ ' : '✔︎ ') + ' ' + destination); + fs.writeFileSync(fullDestination, format(filteredProperties, platform)); + console.log((propertyNamesCollisionCount>0 ? '⚠️ ' : chalk.bold.green('✔︎ ')) + ' ' + fullDestination); if(propertyNamesCollisionCount > 0) { - var propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n '); - let title = `While building ${chalk.keyword(HIGHLIGHT_COLOR).bold(dest)}, token collisions were found; output may be unexpected.`; + let propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n '); + let title = `While building ${chalk.keyword(HIGHLIGHT_COLOR).bold(destination)}, token collisions were found; output may be unexpected.`; let help = chalk.keyword(HELP_COLOR)([ 'This many-to-one issue is usually caused by some combination of:', '* conflicting or similar paths/names in property definitions', From 15b98804143459ab071657862470f5e39806c5a6 Mon Sep 17 00:00:00 2001 From: Charles Dorner Date: Tue, 21 May 2019 12:35:19 -0700 Subject: [PATCH 3/7] removing platformName change --- lib/transform/config.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/transform/config.js b/lib/transform/config.js index e36b14456..9cd89d252 100644 --- a/lib/transform/config.js +++ b/lib/transform/config.js @@ -29,7 +29,6 @@ var MISSING_TRANSFORM_ERRORS = GroupMessages.GROUP.MissingRegisterTransformError */ function transformConfig(config, dictionary, platformName) { var to_ret = _.clone(config); - to_ret.platformName = platformName; // The platform can define either a transformGroup or an array // of transforms. If given a transformGroup that doesn't exist, From 54c5ada05679a18151902720d89a412d1183315a Mon Sep 17 00:00:00 2001 From: Charles Dorner Date: Tue, 21 May 2019 12:38:12 -0700 Subject: [PATCH 4/7] switching to in-line color names --- lib/buildFile.js | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/buildFile.js b/lib/buildFile.js index 26ef370e6..d3ffc9fc8 100644 --- a/lib/buildFile.js +++ b/lib/buildFile.js @@ -58,19 +58,16 @@ function buildFile(destination, format, platform, dictionary, filter) { }); var PROPERTY_NAME_COLLISION_WARNINGS = GroupMessages.GROUP.PropertyNameCollisionWarnings; - var WARNING_COLOR = 'darkorange'; - var HIGHLIGHT_COLOR = 'orangered'; - var HELP_COLOR = 'orange'; Object.keys(nameCollisionObj).forEach((propertyName) => { if(nameCollisionObj[propertyName].length > 1) { let collisions = nameCollisionObj[propertyName].map((properties) => { - let propertyPathText = chalk.keyword(HIGHLIGHT_COLOR)(properties.path.join('.')); - let valueText = chalk.keyword(WARNING_COLOR)(properties.value); + let propertyPathText = chalk.keyword('orangered')(properties.path.join('.')); + let valueText = chalk.keyword('darkorange')(properties.value); return propertyPathText + ' ' + valueText; }).join('\n '); GroupMessages.add( PROPERTY_NAME_COLLISION_WARNINGS, - `Output name ${chalk.keyword(HIGHLIGHT_COLOR).bold(propertyName)} was generated by:\n ${collisions}` + `Output name ${chalk.keyword('orangered').bold(propertyName)} was generated by:\n ${collisions}` ); } }); @@ -82,15 +79,15 @@ function buildFile(destination, format, platform, dictionary, filter) { if(propertyNamesCollisionCount > 0) { let propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n '); - let title = `While building ${chalk.keyword(HIGHLIGHT_COLOR).bold(destination)}, token collisions were found; output may be unexpected.`; - let help = chalk.keyword(HELP_COLOR)([ + let title = `While building ${chalk.keyword('orangered').bold(destination)}, token collisions were found; output may be unexpected.`; + let help = chalk.keyword('orange')([ 'This many-to-one issue is usually caused by some combination of:', '* conflicting or similar paths/names in property definitions', '* platform transforms/transformGroups affecting names, especially when removing specificity', '* overly inclusive file filters', ].join('\n ')); let warn = `${title}\n ${propertyNamesCollisionWarnings}\n${help}`; - console.log(chalk.keyword(WARNING_COLOR).bold(warn)); + console.log(chalk.keyword('darkorange').bold(warn)); } } From 66eb7e72025b1dd41b35ff92a82005b800d3ae7b Mon Sep 17 00:00:00 2001 From: Charles Dorner Date: Tue, 21 May 2019 12:41:24 -0700 Subject: [PATCH 5/7] reverting basic example back --- examples/basic/properties/color/base.json | 2 -- 1 file changed, 2 deletions(-) diff --git a/examples/basic/properties/color/base.json b/examples/basic/properties/color/base.json index dec0abbc8..02a99d700 100644 --- a/examples/basic/properties/color/base.json +++ b/examples/basic/properties/color/base.json @@ -3,9 +3,7 @@ "base": { "gray": { "light" : { "value": "#CCCCCC" }, - "Light" : { "value": "#DDD" }, "medium": { "value": "#999999" }, - "Medium": { "value": "#aaaaaa" }, "dark" : { "value": "#111111" } }, "red": { "value": "#FF0000" }, From 8f3b412b04d6eebf66a555611f3feec0b309ec7c Mon Sep 17 00:00:00 2001 From: Charles Dorner Date: Tue, 21 May 2019 13:22:39 -0700 Subject: [PATCH 6/7] adding tests --- __tests__/buildFile.test.js | 24 ++++++++++++++++++++++++ lib/buildFile.js | 5 +++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/__tests__/buildFile.test.js b/__tests__/buildFile.test.js index 8e742d222..e683b8049 100644 --- a/__tests__/buildFile.test.js +++ b/__tests__/buildFile.test.js @@ -13,6 +13,7 @@ var buildFile = require('../lib/buildFile'); var helpers = require('./__helpers'); +var GroupMessages = require('../lib/utils/groupMessages'); function format() { return "hi"; @@ -52,6 +53,29 @@ describe('buildFile', () => { ).toThrow('Please enter a valid destination'); }); + let dest = 'test.collisions'; + var PROPERTY_NAME_COLLISION_WARNINGS = GroupMessages.GROUP.PropertyNameCollisionWarnings + ":" + dest; + it('should generate warning messages for output name collisions', () => { + GroupMessages.clear(PROPERTY_NAME_COLLISION_WARNINGS); + + buildFile(dest, format, {}, { + allProperties: [{ + name: 'someName', + path: ['some', 'name', 'path1'], + value: 'value1' + }, { + name: 'someName', + path: ['some', 'name', 'path2'], + value: 'value2' + }] + }); + expect(GroupMessages.count(PROPERTY_NAME_COLLISION_WARNINGS)).toBe(1); + console.log(GroupMessages.fetchMessages(PROPERTY_NAME_COLLISION_WARNINGS)) + expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_NAME_COLLISION_WARNINGS))).toBe(JSON.stringify([ + 'Output name \u001b[38;5;202m\u001b[1msomeName\u001b[22m\u001b[39m was generated by:\n \u001b[38;5;202msome.name.path1\u001b[39m \u001b[38;5;214mvalue1\u001b[39m\n \u001b[38;5;202msome.name.path2\u001b[39m \u001b[38;5;214mvalue2\u001b[39m' + ])); + }); + it('should write to a file properly', () => { buildFile('test.txt', format, {buildPath: '__tests__/__output/'}, {}); expect(helpers.fileExists('./__tests__/__output/test.txt')).toBeTruthy(); diff --git a/lib/buildFile.js b/lib/buildFile.js index d3ffc9fc8..fd1fa4a3f 100644 --- a/lib/buildFile.js +++ b/lib/buildFile.js @@ -57,7 +57,8 @@ function buildFile(destination, format, platform, dictionary, filter) { nameCollisionObj[propertyName].push(propertyData); }); - var PROPERTY_NAME_COLLISION_WARNINGS = GroupMessages.GROUP.PropertyNameCollisionWarnings; + var PROPERTY_NAME_COLLISION_WARNINGS = GroupMessages.GROUP.PropertyNameCollisionWarnings + ":" + destination; + GroupMessages.clear(PROPERTY_NAME_COLLISION_WARNINGS); Object.keys(nameCollisionObj).forEach((propertyName) => { if(nameCollisionObj[propertyName].length > 1) { let collisions = nameCollisionObj[propertyName].map((properties) => { @@ -78,7 +79,7 @@ function buildFile(destination, format, platform, dictionary, filter) { console.log((propertyNamesCollisionCount>0 ? '⚠️ ' : chalk.bold.green('✔︎ ')) + ' ' + fullDestination); if(propertyNamesCollisionCount > 0) { - let propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n '); + let propertyNamesCollisionWarnings = GroupMessages.fetchMessages(PROPERTY_NAME_COLLISION_WARNINGS).join('\n '); let title = `While building ${chalk.keyword('orangered').bold(destination)}, token collisions were found; output may be unexpected.`; let help = chalk.keyword('orange')([ 'This many-to-one issue is usually caused by some combination of:', From 4a2b8fa094b5ea4ff9445c51faf9019f53a5acea Mon Sep 17 00:00:00 2001 From: Charles Dorner Date: Wed, 22 May 2019 22:17:01 -0700 Subject: [PATCH 7/7] fixed tests to check for output using chalk instead of hardcoded characters --- __tests__/buildFile.test.js | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/__tests__/buildFile.test.js b/__tests__/buildFile.test.js index e683b8049..39da9b32a 100644 --- a/__tests__/buildFile.test.js +++ b/__tests__/buildFile.test.js @@ -14,6 +14,7 @@ var buildFile = require('../lib/buildFile'); var helpers = require('./__helpers'); var GroupMessages = require('../lib/utils/groupMessages'); +var chalk = require('chalk'); function format() { return "hi"; @@ -56,24 +57,32 @@ describe('buildFile', () => { let dest = 'test.collisions'; var PROPERTY_NAME_COLLISION_WARNINGS = GroupMessages.GROUP.PropertyNameCollisionWarnings + ":" + dest; it('should generate warning messages for output name collisions', () => { - GroupMessages.clear(PROPERTY_NAME_COLLISION_WARNINGS); - - buildFile(dest, format, {}, { + let name = 'someName'; + let properties = { allProperties: [{ - name: 'someName', + name: name, path: ['some', 'name', 'path1'], value: 'value1' }, { - name: 'someName', + name: name, path: ['some', 'name', 'path2'], value: 'value2' }] - }); + }; + + GroupMessages.clear(PROPERTY_NAME_COLLISION_WARNINGS); + buildFile(dest, format, {}, properties); + + let collisions = properties.allProperties.map((properties) => { + let propertyPathText = chalk.keyword('orangered')(properties.path.join('.')); + let valueText = chalk.keyword('darkorange')(properties.value); + return propertyPathText + ' ' + valueText; + }).join('\n '); + let output = `Output name ${chalk.keyword('orangered').bold(name)} was generated by:\n ${collisions}`; + let expectJSON = JSON.stringify([output]); + expect(GroupMessages.count(PROPERTY_NAME_COLLISION_WARNINGS)).toBe(1); - console.log(GroupMessages.fetchMessages(PROPERTY_NAME_COLLISION_WARNINGS)) - expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_NAME_COLLISION_WARNINGS))).toBe(JSON.stringify([ - 'Output name \u001b[38;5;202m\u001b[1msomeName\u001b[22m\u001b[39m was generated by:\n \u001b[38;5;202msome.name.path1\u001b[39m \u001b[38;5;214mvalue1\u001b[39m\n \u001b[38;5;202msome.name.path2\u001b[39m \u001b[38;5;214mvalue2\u001b[39m' - ])); + expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_NAME_COLLISION_WARNINGS))).toBe(expectJSON); }); it('should write to a file properly', () => {