Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

first cut at catching property name collisions during file output #273

Merged
merged 7 commits into from
May 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions __tests__/buildFile.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

var buildFile = require('../lib/buildFile');
var helpers = require('./__helpers');
var GroupMessages = require('../lib/utils/groupMessages');
var chalk = require('chalk');

function format() {
return "hi";
Expand Down Expand Up @@ -52,6 +54,37 @@ 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', () => {
let name = 'someName';
let properties = {
allProperties: [{
name: name,
path: ['some', 'name', 'path1'],
value: 'value1'
}, {
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);
expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_NAME_COLLISION_WARNINGS))).toBe(expectJSON);
});

it('should write to a file properly', () => {
buildFile('test.txt', format, {buildPath: '__tests__/__output/'}, {});
expect(helpers.fileExists('./__tests__/__output/test.txt')).toBeTruthy();
Expand Down
58 changes: 52 additions & 6 deletions lib/buildFile.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -33,17 +34,62 @@ function buildFile(destination, format, platform, dictionary, filter) {
if (!destination || typeof destination !== 'string')
throw new Error('Please enter a valid destination');

// if there is a build path, prepend the destination with it
var fullDestination = destination;

// 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);

fs.writeFileSync(destination, format(filterProperties(dictionary, filter), platform));
console.log(chalk.bold.green('✔︎') + ' ' + destination);
var filteredProperties = filterProperties(dictionary, filter);

// Check for property name Collisions
var nameCollisionObj = {};
filteredProperties.allProperties && filteredProperties.allProperties.forEach((propertyData) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this check? I think when we are constructing allProperties it will always be at least an empty array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is required. The tests do not generate allProperties at all in some cases, causing an error to throw here and the tests to fail.

let propertyName = propertyData.name;
if(!nameCollisionObj[propertyName]) {
nameCollisionObj[propertyName] = [];
}
nameCollisionObj[propertyName].push(propertyData);
});

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) => {
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('orangered').bold(propertyName)} was generated by:\n ${collisions}`
);
}
});

let propertyNamesCollisionCount = GroupMessages.count(PROPERTY_NAME_COLLISION_WARNINGS);

fs.writeFileSync(fullDestination, format(filteredProperties, platform));
console.log((propertyNamesCollisionCount>0 ? '⚠️ ' : chalk.bold.green('✔︎ ')) + ' ' + fullDestination);

if(propertyNamesCollisionCount > 0) {
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:',
'* 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('darkorange').bold(warn));
}
}


Expand Down
1 change: 1 addition & 0 deletions lib/utils/groupMessages.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are getting some real use out of the groupMessages! I never thought we'd have this many warnings and errors, but it follows our tenet to fail loudly! Super cool.

},

flush: function (messageGroup) {
Expand Down