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

Add new entries on property object #356

Merged
merged 4 commits into from
Jul 20, 2020
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
24 changes: 24 additions & 0 deletions __tests__/__properties/paddings.1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"size": {
"padding": {
"tiny": {
"value": "3"
},
"small": {
"value": "5"
},
"base": {
"value": "10"
},
"large": {
"value": "15"
},
"xl": {
"value": "20"
},
"xxl": {
"value": "30"
}
}
}
}
67 changes: 54 additions & 13 deletions __tests__/extend.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ var helpers = require('./__helpers');
var StyleDictionary = require('../index');
var _ = require('lodash');

function traverseObj(obj, fn) {
for (let key in obj) {
fn.apply(this, [obj, key, obj[key]]);
if (obj[key] && typeof obj[key] === 'object') {
traverseObj(obj[key], fn);
}
}
}

var test_props = {
size: {
padding: {
Expand All @@ -24,7 +33,6 @@ var test_props = {
};

describe('extend', () => {

describe('method signature', () => {
it('should accept a string as a path to a JSON file', () => {
var StyleDictionaryExtended = StyleDictionary.extend(__dirname + '/__configs/test.json');
Expand Down Expand Up @@ -77,17 +85,31 @@ describe('extend', () => {

it('should build the properties object if an include is given', () => {
var StyleDictionaryExtended = StyleDictionary.extend({
"include": [__dirname + "/__properties/paddings.json"]
include: [__dirname + "/__properties/paddings.json"]
});
expect(StyleDictionaryExtended.properties).toEqual(helpers.fileToJSON(__dirname + "/__properties/paddings.json"));
var output = helpers.fileToJSON(__dirname + "/__properties/paddings.json");
traverseObj(output, (obj) => {
if (obj.value && !obj.filePath) {
obj.filePath = __dirname + "/__properties/paddings.json";
obj.isSource = false;
}
});
expect(StyleDictionaryExtended.properties).toEqual(output);
});

it('should override existing properties if include is given', () => {
var StyleDictionaryExtended = StyleDictionary.extend({
properties: test_props,
include: [__dirname + "/__properties/paddings.json"]
"include": [__dirname + "/__properties/paddings.json"]
});
expect(StyleDictionaryExtended.properties).toEqual(helpers.fileToJSON(__dirname + "/__properties/paddings.json"));
var output = helpers.fileToJSON(__dirname + "/__properties/paddings.json");
traverseObj(output, (obj) => {
if (obj.value && !obj.filePath) {
obj.filePath = __dirname + "/__properties/paddings.json";
obj.isSource = false;
}
});
expect(StyleDictionaryExtended.properties).toEqual(output);
});

it('should update properties if there are includes', () => {
Expand All @@ -106,7 +128,6 @@ describe('extend', () => {
});
});


describe('source', () => {
it('should throw if source isnt an array', () => {
expect(
Expand All @@ -125,34 +146,54 @@ describe('extend', () => {
var StyleDictionaryExtended = StyleDictionary.extend({
"source": [__dirname + "/__properties/paddings.json"]
});
expect(StyleDictionaryExtended.properties).toEqual(helpers.fileToJSON(__dirname + "/__properties/paddings.json"));
var output = helpers.fileToJSON(__dirname + "/__properties/paddings.json");
traverseObj(output, (obj) => {
if (obj.value && !obj.filePath) {
obj.filePath = __dirname + "/__properties/paddings.json";
obj.isSource = true;
}
});
expect(StyleDictionaryExtended.properties).toEqual(output);
});

it('should override existing properties source is given', () => {
var StyleDictionaryExtended = StyleDictionary.extend({
properties: test_props,
source: [__dirname + "/__properties/paddings.json"]
});
expect(StyleDictionaryExtended.properties).toEqual(helpers.fileToJSON(__dirname + "/__properties/paddings.json"));
var output = helpers.fileToJSON(__dirname + "/__properties/paddings.json");
traverseObj(output, (obj) => {
if (obj.value && !obj.filePath) {
obj.filePath = __dirname + "/__properties/paddings.json";
obj.isSource = true;
}
});
expect(StyleDictionaryExtended.properties).toEqual(output);
});
});


// This is to allow style dictionaries to depend on other style dictionaries and
// override properties. Useful for skinning
it('should not throw a collision error if a source file collides with an include', () => {
var StyleDictionaryExtended = StyleDictionary.extend({
include: [__dirname + "/__properties/paddings.json"],
include: [__dirname + "/__properties/paddings.1.json"],
source: [__dirname + "/__properties/paddings.json"],
log: 'error'
});
expect(StyleDictionaryExtended.properties).toEqual(helpers.fileToJSON(__dirname + "/__properties/paddings.json"));
var output = helpers.fileToJSON(__dirname + "/__properties/paddings.json");
traverseObj(output, (obj) => {
if (obj.value && !obj.filePath) {
obj.filePath = __dirname + "/__properties/paddings.json";
obj.isSource = true;
}
});
expect(StyleDictionaryExtended.properties).toEqual(output);
});

it('should throw a error if the collision is in source files and log is set to error', () => {
expect(
StyleDictionary.extend.bind(null, {
source: [__dirname + "/__properties/paddings.json", __dirname + "/__properties/paddings.json"],
source: [__dirname + "/__properties/paddings.1.json", __dirname + "/__properties/paddings.json"],
log: 'error'
})
).toThrow('Collisions detected');
Expand All @@ -171,7 +212,7 @@ describe('extend', () => {
var StyleDictionaryExtended = StyleDictionary.extend(__dirname + '/__configs/test.json5');
expect(StyleDictionaryExtended).toHaveProperty('platforms.web');
});

it('should allow for chained extends and not mutate the original', function() {
var StyleDictionary1 = StyleDictionary.extend({
foo: 'bar'
Expand Down
2 changes: 1 addition & 1 deletion lib/extend.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function extend(opts) {
if (!_.isArray(options.include))
throw new Error('include must be an array');

to_ret.properties = combineJSON(options.include, true);
to_ret.properties = combineJSON(options.include, true, null, false);

to_ret.include = null; // We don't want to carry over include references
}
Expand Down
9 changes: 7 additions & 2 deletions lib/transform/propertySetup.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,13 @@ function propertySetup(property, name, path) {
// Only do this once
if (!property.original) {
// Initial property setup
// Keep the original object properties; we will key off of them in the transforms
to_ret.original = _.clone(property);
// Keep the original object properties like it was in file (whitout additional data)
// so we can key off them in the transforms
var to_ret_original = _.clone(property);
delete to_ret_original.filePath;
delete to_ret_original.isSource;

to_ret.original = to_ret_original;
// Copy the name - it will be our base name to transform
to_ret.name = to_ret.name || name || '';
// Create an empty attributes object that we can transform on it later
Expand Down
25 changes: 22 additions & 3 deletions lib/utils/combineJSON.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,26 @@ var glob = require('glob'),
path = require('path'),
resolveCwd = require('resolve-cwd');

function traverseObj(obj, fn) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would make sense to move this to its own file so we don't have to duplicate it here?

for (let key in obj) {
fn.apply(null, [obj, key, obj[key]]);
if (obj[key] && typeof obj[key] === 'object') {
traverseObj(obj[key], fn);
}
}
}

/**
* Takes an array of json files and merges
* them together. Optionally does a deep extend.
* @private
* @param {String[]} arr - Array of paths to json (or node modules that export objects) files
* @param {Boolean} [deep=false] - If it should perform a deep merge
* @param {Function} collision - A function to be called when a name collision happens that isn't a normal deep merge of objects
* @param {Boolean} [source=true] - If json files are "sources", tag properties
* @returns {Object}
*/
function combineJSON(arr, deep, collision) {
function combineJSON(arr, deep, collision, source) {
var i, files = [],
to_ret = {};

Expand All @@ -39,17 +49,26 @@ function combineJSON(arr, deep, collision) {

for (i = 0; i < files.length; i++) {
var resolvedPath = resolveCwd(path.isAbsolute(files[i]) ? files[i] : './' + files[i]);
var file_content;
var file_content = {};

try {
// This delete force require(resolvedPath) to take the latest version of the file. It's handfull when using the node package along chokidar.
delete require.cache[resolvedPath]
file_content = require(resolvedPath);
file_content = deepExtend([file_content, require(resolvedPath)]);
} catch (e) {
e.message = 'Failed to load or parse JSON or JS Object: ' + e.message;
throw e;
}

// Add some side data on each property to make filtering easier
traverseObj(file_content, (obj) => {
if (obj.value && !obj.filePath) {
Copy link
Member

Choose a reason for hiding this comment

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

I can't comment on lines you don't change, but we will need to change the line file_content = require(resolvedPath);. Because we are requiring the file rather than loading it with fs.readFile, any mutations we make to the object stay in memory, which is why some tests are failing and showing isSource as true when it should be false. We can fix this a few ways, first we could remove the && !obj.filePath so that it will always override tokens. Although this could cause weird issues in the future because we are mutating a module. Or we could change a line above from:

    var file_content;
    try {
      file_content = require(resolvedPath);

to:

    var file_content = {};
    try {
      deepExtend([file_content, require(resolvedPath)]);

Now mutating file_content would not result in mutating the original module. Or there might be some more elegant solution to create a deep copy of a module.

obj.filePath = resolvedPath;

obj.isSource = source || source === undefined ? true : false;
}
});

if (deep) {
deepExtend([to_ret, file_content], collision);
} else {
Expand Down