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

fix(export platform): fixing infinite loop when there are reference errors #531

Merged
merged 2 commits into from
Feb 2, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
97 changes: 80 additions & 17 deletions __tests__/exportPlatform.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,59 +11,60 @@
* and limitations under the License.
*/

var helpers = require('./__helpers');
var keys = require('lodash/keys');
var config = helpers.fileToJSON(__dirname + '/__configs/test.json');
var StyleDictionary = require('../index').extend(config);
const helpers = require('./__helpers');
const keys = require('lodash/keys');
const config = helpers.fileToJSON(__dirname + '/__configs/test.json');
const StyleDictionary = require('../index');
const styleDictionary = StyleDictionary.extend(config);

describe('exportPlatform', () => {

it('should throw if not given a platform', () => {
expect(
function(){
StyleDictionary.exportPlatform()
styleDictionary.exportPlatform()
}
).toThrow();
});

it('should throw if not given a proper platform', () => {
expect(
function(){
StyleDictionary.exportPlatform('foo');
styleDictionary.exportPlatform('foo');
}
).toThrow();
});

it('should not throw if given a proper platform', () => {
expect(
function(){
StyleDictionary.exportPlatform('web');
styleDictionary.exportPlatform('web');
}
).not.toThrow();
});

it('should return an object', () => {
var dictionary = StyleDictionary.exportPlatform('web');
var dictionary = styleDictionary.exportPlatform('web');
expect(typeof dictionary).toBe('object');
});

it('should have the same structure as the original properties', () => {
var dictionary = StyleDictionary.exportPlatform('web');
expect(keys(dictionary)).toEqual(keys(StyleDictionary.properties));
var dictionary = styleDictionary.exportPlatform('web');
expect(keys(dictionary)).toEqual(keys(styleDictionary.properties));
});

it('should have resolved references', () => {
var dictionary = StyleDictionary.exportPlatform('web');
var dictionary = styleDictionary.exportPlatform('web');
expect(dictionary.color.font.link.value).toEqual(dictionary.color.base.blue['100'].value);
});

it('should have applied transforms', () => {
var dictionary = StyleDictionary.exportPlatform('web');
var dictionary = styleDictionary.exportPlatform('web');
expect(dictionary.size.padding.base.value.indexOf('px')).toBeGreaterThan(0);
});

it('should have applied transforms for props with refs in it', () => {
const StyleDictionaryExtended = StyleDictionary.extend({
const StyleDictionaryExtended = styleDictionary.extend({
platforms: {
test: {
transforms: ['color/css','color/darken']
Expand All @@ -86,17 +87,79 @@ describe('exportPlatform', () => {
});

it('should not have mutated the original properties', () => {
var dictionary = StyleDictionary.exportPlatform('web');
expect(dictionary.color.font.link.value).not.toEqual(StyleDictionary.properties.color.font.link.value);
expect(StyleDictionary.properties.size.padding.base.value.indexOf('px')).toBe(-1);
var dictionary = styleDictionary.exportPlatform('web');
expect(dictionary.color.font.link.value).not.toEqual(styleDictionary.properties.color.font.link.value);
expect(styleDictionary.properties.size.padding.base.value.indexOf('px')).toBe(-1);
});

// Make sure when we perform transforms and resolve references
// we don't mutate the original object added to the property.
it('properties should have original value untouched', () => {
var dictionary = StyleDictionary.exportPlatform('web');
var dictionary = styleDictionary.exportPlatform('web');
var properties = helpers.fileToJSON(__dirname + '/__properties/colors.json');
expect(dictionary.color.font.link.original.value).toEqual(properties.color.font.link.value);
});

describe('reference warnings', () => {
const errorMessage = `Problems were found when trying to resolve property references`;
const platforms = {
css: {
transformGroup: `css`
}
}

it('should throw if there are simple property reference errors', () => {
const properties = {
a: "#ff0000",
b: "{c}"
}
expect(
function() {
StyleDictionary.extend({
properties,
platforms
}).exportPlatform('css')
}
).toThrow(errorMessage);
});

it('should throw if there are circular reference errors', () => {
const properties = {
a: "{b}",
b: "{a}"
}
expect(
function() {
StyleDictionary.extend({
properties,
platforms
}).exportPlatform('css')
}
).toThrow(errorMessage);
});

it('should throw if there are complex property reference errors', () => {
const properties = {
color: {
core: {
red: { valuer: "#ff0000" }, // notice misspelling
blue: { "value:": "#0000ff" }
},
danger: { value: "{color.core.red.value}" },
warning: { value: "{color.base.red.valuer}" },
info: { value: "{color.core.blue.value}" },
error: { value: "{color.danger.value}" }
}
}
expect(
function() {
StyleDictionary.extend({
properties,
platforms
}).exportPlatform('css')
}
).toThrow(errorMessage);
});
});

});
12 changes: 10 additions & 2 deletions lib/exportPlatform.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,22 @@ 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.
const transformationsCountBefore = transformedPropRefs.length;
const transformed = transformObject(exportableResult, platformConfig, transformationContext);

// referenced values, that have not (yet) been transformed should be excluded from resolving
const ignorePathsToResolve = deferredPropValueTransforms.map(p => getName([p, 'value']));
exportableResult = resolveObject(transformed, {ignorePaths: ignorePathsToResolve});

console.log(transformationsCountBefore, transformedPropRefs.length, deferredPropValueTransforms.length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this was an accidental include of some debug code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right you are. This is why I need my code to be reviewed.

// nothing left to transform -> ready
if (deferredPropValueTransforms.length === 0) {
if (
// if there is no deferred value transforms left we are done
deferredPropValueTransforms.length === 0 ||
// or if we didn't transform anything since the last loop.
// This would happen if a token reference is not found or is circular
// because we keep the broken reference
transformedPropRefs.length === transformationsCountBefore
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I trust your logic, and it makes sense... but it seems unintuitive to me that this is a solid "or".

finished = true;
}
}
Expand Down