From baee56fc152511e4dc96a63f101a8621776d9d5d Mon Sep 17 00:00:00 2001 From: Danny Banks Date: Tue, 2 Feb 2021 13:46:01 -0800 Subject: [PATCH 1/2] fix(export platform): fixing infinite loop when there are reference errors --- __tests__/exportPlatform.test.js | 97 ++++++++++++++++++++++++++------ lib/exportPlatform.js | 12 +++- 2 files changed, 90 insertions(+), 19 deletions(-) diff --git a/__tests__/exportPlatform.test.js b/__tests__/exportPlatform.test.js index 52786bdc8..eec481cc7 100644 --- a/__tests__/exportPlatform.test.js +++ b/__tests__/exportPlatform.test.js @@ -11,17 +11,18 @@ * 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(); }); @@ -29,7 +30,7 @@ describe('exportPlatform', () => { it('should throw if not given a proper platform', () => { expect( function(){ - StyleDictionary.exportPlatform('foo'); + styleDictionary.exportPlatform('foo'); } ).toThrow(); }); @@ -37,33 +38,33 @@ describe('exportPlatform', () => { 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'] @@ -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); + }); + }); + }); diff --git a/lib/exportPlatform.js b/lib/exportPlatform.js index 8d9e3e741..539dca606 100644 --- a/lib/exportPlatform.js +++ b/lib/exportPlatform.js @@ -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); // 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 + ) { finished = true; } } From fd1fd9d862eb3cd83312f12335bf8a4c15b1f1d1 Mon Sep 17 00:00:00 2001 From: Danny Banks Date: Tue, 2 Feb 2021 14:37:40 -0800 Subject: [PATCH 2/2] chore(export platform): changes based on comments --- lib/exportPlatform.js | 12 ++---------- lib/utils/resolveObject.js | 3 +-- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/lib/exportPlatform.js b/lib/exportPlatform.js index 539dca606..8d9e3e741 100644 --- a/lib/exportPlatform.js +++ b/lib/exportPlatform.js @@ -93,22 +93,14 @@ 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); + // nothing left to transform -> ready - 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 - ) { + if (deferredPropValueTransforms.length === 0) { finished = true; } } diff --git a/lib/utils/resolveObject.js b/lib/utils/resolveObject.js index 8e5a1cd20..c81f5d629 100644 --- a/lib/utils/resolveObject.js +++ b/lib/utils/resolveObject.js @@ -143,8 +143,7 @@ function compile_value(value, stack) { PROPERTY_REFERENCE_WARNINGS, "Reference doesn't exist: " + context + " tries to reference " + variable + ", which is not defined" ); - // Leave the circular reference unchanged - to_ret = value; + to_ret = ref; } stack.pop(variable);