From 0410295d15d09c5d2e229a1f1b12568b6c6a2997 Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Mon, 8 Jan 2024 17:38:01 +0100 Subject: [PATCH] feat: improve and test ref utils error handling (#1077) --- .changeset/kind-rats-invite.md | 5 ++ .../__snapshots__/platform.test.snap.js | 2 +- .../utils/reference/getReferences.test.js | 12 ++++- .../utils/reference/resolveReferences.test.js | 30 ++++++++--- __tests__/utils/resolveObject.test.js | 6 +-- lib/buildFile.js | 20 ++++---- .../formatHelpers/createPropertyFormatter.js | 4 +- lib/common/formatHelpers/sortByReference.js | 3 +- lib/utils/index.js | 2 +- lib/utils/references/getReferences.js | 51 +++++++++++++++---- lib/utils/references/resolveReferences.js | 33 +++++++----- package-lock.json | 4 +- types/Config.d.ts | 1 + 13 files changed, 123 insertions(+), 50 deletions(-) create mode 100644 .changeset/kind-rats-invite.md diff --git a/.changeset/kind-rats-invite.md b/.changeset/kind-rats-invite.md new file mode 100644 index 000000000..bd36a20bd --- /dev/null +++ b/.changeset/kind-rats-invite.md @@ -0,0 +1,5 @@ +--- +'style-dictionary': minor +--- + +Improve and test the error handling of standalone usage of reference utilities. diff --git a/__integration__/logging/__snapshots__/platform.test.snap.js b/__integration__/logging/__snapshots__/platform.test.snap.js index 4f0976d5d..7e23fdb0a 100644 --- a/__integration__/logging/__snapshots__/platform.test.snap.js +++ b/__integration__/logging/__snapshots__/platform.test.snap.js @@ -21,7 +21,7 @@ Unknown transformGroup "foo" found in platform "css": snapshots["integration logging platform property reference errors should throw and notify users of unknown references"] = ` Property Reference Errors: -Reference doesn't exist: color.danger.value tries to reference color.red.value, which is not defined +Reference doesn't exist: color.danger.value tries to reference color.red.value, which is not defined. Problems were found when trying to resolve property references`; /* end snapshot integration logging platform property reference errors should throw and notify users of unknown references */ diff --git a/__tests__/utils/reference/getReferences.test.js b/__tests__/utils/reference/getReferences.test.js index 96c63cd88..60127260a 100644 --- a/__tests__/utils/reference/getReferences.test.js +++ b/__tests__/utils/reference/getReferences.test.js @@ -12,7 +12,9 @@ */ import { expect } from 'chai'; -import getReferences from '../../../lib/utils/references/getReferences.js'; +import getReferences, { + getReferences as publicGetReferences, +} from '../../../lib/utils/references/getReferences.js'; const tokens = { color: { @@ -49,6 +51,14 @@ const tokens = { describe('utils', () => { describe('reference', () => { describe('getReferences()', () => { + describe('public API', () => { + it('should not collect errors but rather throw immediately when using public API', () => { + expect(() => publicGetReferences('{foo.bar}', tokens)).to.throw( + `Reference doesn't exist: tries to reference foo.bar, which is not defined.`, + ); + }); + }); + it(`should return an empty array if the value has no references`, () => { expect(getReferences(tokens.color.red.value, tokens)).to.eql([]); }); diff --git a/__tests__/utils/reference/resolveReferences.test.js b/__tests__/utils/reference/resolveReferences.test.js index 71a90460b..1b0664697 100644 --- a/__tests__/utils/reference/resolveReferences.test.js +++ b/__tests__/utils/reference/resolveReferences.test.js @@ -12,7 +12,10 @@ */ import { expect } from 'chai'; import { fileToJSON } from '../../__helpers.js'; -import { resolveReferences } from '../../../lib/utils/references/resolveReferences.js'; +import { + _resolveReferences as resolveReferences, + resolveReferences as publicResolveReferences, +} from '../../../lib/utils/references/resolveReferences.js'; import GroupMessages from '../../../lib/utils/groupMessages.js'; const PROPERTY_REFERENCE_WARNINGS = GroupMessages.GROUP.PropertyReferenceWarnings; @@ -24,6 +27,21 @@ describe('utils', () => { GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS); }); + describe('public API', () => { + it('should not collect errors but rather throw immediately when using public API', () => { + const obj = fileToJSON('__tests__/__json_files/multiple_reference_errors.json'); + expect(() => publicResolveReferences(obj.a.b, obj)).to.throw( + `Reference doesn't exist: tries to reference b.a, which is not defined.`, + ); + expect(() => publicResolveReferences(obj.a.c, obj)).to.throw( + `Reference doesn't exist: tries to reference b.c, which is not defined.`, + ); + expect(() => publicResolveReferences(obj.a.d, obj)).to.throw( + `Reference doesn't exist: tries to reference d, which is not defined.`, + ); + }); + }); + it('should do simple references', () => { const test = resolveReferences('{foo}', fileToJSON('__tests__/__json_files/simple.json')); expect(test).to.equal('bar'); @@ -105,8 +123,8 @@ describe('utils', () => { expect(resolveReferences(obj.foo, obj)).to.be.undefined; expect(resolveReferences(obj.error, obj)).to.be.undefined; expect(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS)).to.eql([ - "Reference doesn't exist: tries to reference bar, which is not defined", - "Reference doesn't exist: tries to reference a.b.d, which is not defined", + "Reference doesn't exist: tries to reference bar, which is not defined.", + "Reference doesn't exist: tries to reference a.b.d, which is not defined.", ]); }); @@ -202,9 +220,9 @@ describe('utils', () => { expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).to.equal(3); expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS))).to.equal( JSON.stringify([ - "Reference doesn't exist: tries to reference b.a, which is not defined", - "Reference doesn't exist: tries to reference b.c, which is not defined", - "Reference doesn't exist: tries to reference d, which is not defined", + "Reference doesn't exist: tries to reference b.a, which is not defined.", + "Reference doesn't exist: tries to reference b.c, which is not defined.", + "Reference doesn't exist: tries to reference d, which is not defined.", ]), ); }); diff --git a/__tests__/utils/resolveObject.test.js b/__tests__/utils/resolveObject.test.js index fe6ce76b8..4364db158 100644 --- a/__tests__/utils/resolveObject.test.js +++ b/__tests__/utils/resolveObject.test.js @@ -321,9 +321,9 @@ describe('utils', () => { expect(GroupMessages.count(PROPERTY_REFERENCE_WARNINGS)).to.equal(3); expect(JSON.stringify(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS))).to.equal( JSON.stringify([ - "Reference doesn't exist: a.b tries to reference b.a, which is not defined", - "Reference doesn't exist: a.c tries to reference b.c, which is not defined", - "Reference doesn't exist: a.d tries to reference d, which is not defined", + "Reference doesn't exist: a.b tries to reference b.a, which is not defined.", + "Reference doesn't exist: a.c tries to reference b.c, which is not defined.", + "Reference doesn't exist: a.d tries to reference d, which is not defined.", ]), ); }); diff --git a/lib/buildFile.js b/lib/buildFile.js index 698477515..67a2aef42 100644 --- a/lib/buildFile.js +++ b/lib/buildFile.js @@ -110,7 +110,7 @@ export default function buildFile(file, platform = {}, dictionary) { } }); - let tokenNamesCollisionCount = GroupMessages.count(PROPERTY_NAME_COLLISION_WARNINGS); + const tokenNamesCollisionCount = GroupMessages.count(PROPERTY_NAME_COLLISION_WARNINGS); fs.writeFileSync( fullDestination, format( @@ -122,7 +122,7 @@ export default function buildFile(file, platform = {}, dictionary) { ), ); - let filteredReferencesCount = GroupMessages.count(GroupMessages.GROUP.FilteredOutputReferences); + const filteredReferencesCount = GroupMessages.count(GroupMessages.GROUP.FilteredOutputReferences); // don't show name collision warnings for nested type formats // because they are not relevant. @@ -131,13 +131,13 @@ export default function buildFile(file, platform = {}, dictionary) { } else { const warnHeader = `⚠️ ${fullDestination}`; if (tokenNamesCollisionCount > 0) { - let tokenNamesCollisionWarnings = GroupMessages.fetchMessages( + const tokenNamesCollisionWarnings = GroupMessages.fetchMessages( PROPERTY_NAME_COLLISION_WARNINGS, ).join('\n '); - let title = `While building ${chalk + const title = `While building ${chalk .rgb(255, 69, 0) .bold(destination)}, token collisions were found; output may be unexpected.`; - let help = chalk.rgb( + const help = chalk.rgb( 255, 165, 0, @@ -149,7 +149,7 @@ export default function buildFile(file, platform = {}, dictionary) { '* overly inclusive file filters', ].join('\n '), ); - let warn = `${warnHeader}\n${title}\n ${tokenNamesCollisionWarnings}\n${help}`; + const warn = `${warnHeader}\n${title}\n ${tokenNamesCollisionWarnings}\n${help}`; if (platform?.log === 'error') { throw new Error(warn); } else { @@ -158,20 +158,20 @@ export default function buildFile(file, platform = {}, dictionary) { } if (filteredReferencesCount > 0) { - let filteredReferencesWarnings = GroupMessages.flush( + const filteredReferencesWarnings = GroupMessages.flush( GroupMessages.GROUP.FilteredOutputReferences, ).join('\n '); - let title = `While building ${chalk + const title = `While building ${chalk .rgb(255, 69, 0) .bold( destination, )}, filtered out token references were found; output may be unexpected. Here are the references that are used but not defined in the file`; - let help = chalk.rgb( + const help = chalk.rgb( 255, 165, 0, )(['This is caused when combining a filter and `outputReferences`.'].join('\n ')); - let warn = `${warnHeader}\n${title}\n ${filteredReferencesWarnings}\n${help}`; + const warn = `${warnHeader}\n${title}\n ${filteredReferencesWarnings}\n${help}`; if (platform?.log === 'error') { throw new Error(warn); } else { diff --git a/lib/common/formatHelpers/createPropertyFormatter.js b/lib/common/formatHelpers/createPropertyFormatter.js index 00e348c1c..af0298c86 100644 --- a/lib/common/formatHelpers/createPropertyFormatter.js +++ b/lib/common/formatHelpers/createPropertyFormatter.js @@ -10,8 +10,8 @@ * CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions * and limitations under the License. */ - -import { usesReferences, getReferences } from '../../utils/index.js'; +import getReferences from '../../utils/references/getReferences.js'; +import usesReferences from '../../utils/references/usesReferences.js'; /** * @typedef {import('../../../types/DesignToken.d.ts').Dictionary} Dictionary diff --git a/lib/common/formatHelpers/sortByReference.js b/lib/common/formatHelpers/sortByReference.js index 53f646897..f8f3277cc 100644 --- a/lib/common/formatHelpers/sortByReference.js +++ b/lib/common/formatHelpers/sortByReference.js @@ -11,7 +11,8 @@ * and limitations under the License. */ -import { usesReferences, getReferences } from 'style-dictionary/utils'; +import usesReferences from '../../utils/references/usesReferences.js'; +import getReferences from '../../utils/references/getReferences.js'; /** * @typedef {import('../../../types/DesignToken.d.ts').TransformedTokens} Tokens diff --git a/lib/utils/index.js b/lib/utils/index.js index 3be3ceafa..030b2296a 100644 --- a/lib/utils/index.js +++ b/lib/utils/index.js @@ -12,7 +12,7 @@ */ import usesReferences from './references/usesReferences.js'; -import getReferences from './references/getReferences.js'; +import { getReferences } from './references/getReferences.js'; import { resolveReferences } from './references/resolveReferences.js'; // Public style-dictionary/utils API diff --git a/lib/utils/references/getReferences.js b/lib/utils/references/getReferences.js index a37b50a67..1fe61b367 100644 --- a/lib/utils/references/getReferences.js +++ b/lib/utils/references/getReferences.js @@ -10,13 +10,33 @@ * CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions * and limitations under the License. */ - import getPathFromName from './getPathFromName.js'; import createReferenceRegex from './createReferenceRegex.js'; import getValueByPath from './getValueByPath.js'; import GroupMessages from '../groupMessages.js'; import defaults from './defaults.js'; +const FILTER_WARNINGS = GroupMessages.GROUP.FilteredOutputReferences; + +/** + * @typedef {import('../../../types/Config.d.ts').RegexOptions} RegexOptions + * @typedef {import('../../StyleDictionary.js').default} Dictionary + * @typedef {import('../../../types/DesignToken.d.ts').TransformedTokens} Tokens + * @typedef {import('../../../types/DesignToken.d.ts').TransformedToken} Token + * + * Public API wrapper around the function below this one + * + * @memberof Dictionary + * @param {string|Object} value the value that contains a reference + * @param {Tokens} tokens the dictionary to search in + * @param {RegexOptions & { unfilteredTokens?: Tokens }} [opts] + * @param {Token[]} [references] array of token's references because a token's value can contain multiple references due to string interpolation + * @returns {Token[]} + */ +export function getReferences(value, tokens, opts = {}, references = []) { + return _getReferences(value, tokens, opts, references, true); +} + /** * This is a helper function that is added to the dictionary object that * is passed to formats and actions. It will resolve a reference giving @@ -26,19 +46,21 @@ import defaults from './defaults.js'; * ```css * --color-background-base: var(--color-core-white); * ``` - * @typedef {import('../../../types/Config.d.ts').RegexOptions} RegexOptions - * @typedef {import('../../StyleDictionary.js').default} Dictionary - * @typedef {import('../../../types/DesignToken.d.ts').TransformedTokens} Tokens - * @typedef {import('../../../types/DesignToken.d.ts').TransformedToken} Token * - * @memberof Dictionary * @param {string|Object} value the value that contains a reference * @param {Tokens} tokens the dictionary to search in * @param {RegexOptions & { unfilteredTokens?: Tokens }} [opts] * @param {Token[]} [references] array of token's references because a token's value can contain multiple references due to string interpolation + * @param {boolean} [throwImmediately] * @returns {Token[]} */ -export default function getReferences(value, tokens, opts = {}, references = []) { +export default function _getReferences( + value, + tokens, + opts = {}, + references = [], + throwImmediately = false, +) { const regex = createReferenceRegex(opts); /** @@ -55,14 +77,21 @@ export default function getReferences(value, tokens, opts = {}, references = []) let ref = getValueByPath(pathName, tokens); - if (!ref && opts.unfilteredTokens) { + if (ref === undefined && opts.unfilteredTokens) { + if (!throwImmediately) { + // warn the user about this + GroupMessages.add(FILTER_WARNINGS, variable); + } // fall back on unfilteredTokens as it is unfiltered ref = getValueByPath(pathName, opts.unfilteredTokens); - // and warn the user about this - GroupMessages.add(GroupMessages.GROUP.FilteredOutputReferences, variable); } + if (ref !== undefined) { references.push(ref); + } else if (throwImmediately) { + throw new Error( + `Reference doesn't exist: tries to reference ${variable}, which is not defined.`, + ); } return ''; } @@ -83,7 +112,7 @@ export default function getReferences(value, tokens, opts = {}, references = []) } // if it is an object, we go further down the rabbit hole if (value.hasOwnProperty(key) && typeof value[key] === 'object') { - getReferences(value[key], tokens, opts, references); + _getReferences(value[key], tokens, opts, references); } } } diff --git a/lib/utils/references/resolveReferences.js b/lib/utils/references/resolveReferences.js index 47e98c3eb..7ad17c5db 100644 --- a/lib/utils/references/resolveReferences.js +++ b/lib/utils/references/resolveReferences.js @@ -28,14 +28,15 @@ const PROPERTY_REFERENCE_WARNINGS = GroupMessages.GROUP.PropertyReferenceWarning * @typedef {import('../../../types/DesignToken.d.ts').DesignToken} DesignToken */ -/** Utility to resolve references inside a string value +/** + * Public API wrapper around the functon below this one * @param {string} value * @param {DesignTokens} tokens * @param {RefOpts} [opts] * @returns {string|number|undefined} */ export function resolveReferences(value, tokens, opts) { - return _resolveReferences(value, tokens, opts); + return _resolveReferences(value, tokens, { ...opts, throwImmediately: true }); } /** @@ -59,6 +60,7 @@ export function _resolveReferences( stack = [], foundCirc = {}, firstIteration = true, + throwImmediately = false, } = {}, ) { const reg = regex ?? createReferenceRegex({ opening_character, closing_character, separator }); @@ -136,10 +138,15 @@ export function _resolveReferences( circStack.push(reference); // Add circ reference info to our list of warning messages - GroupMessages.add( - PROPERTY_REFERENCE_WARNINGS, - 'Circular definition cycle: ' + circStack.join(', '), - ); + const warning = `Circular definition cycle: ${circStack.join(', ')}`; + if (throwImmediately) { + throw new Error(warning); + } else { + GroupMessages.add( + PROPERTY_REFERENCE_WARNINGS, + 'Circular definition cycle: ' + circStack.join(', '), + ); + } } else { to_ret = _resolveReferences(to_ret, tokens, { regex: reg, @@ -164,12 +171,14 @@ export function _resolveReferences( // User might have passed current_context option which is path (arr) pointing to key // that this value is associated with, helpful for debugging const context = getName(current_context, { separator }); - GroupMessages.add( - PROPERTY_REFERENCE_WARNINGS, - `Reference doesn't exist:${ - context ? ` ${context}` : '' - } tries to reference ${variable}, which is not defined`, - ); + const warning = `Reference doesn't exist:${ + context ? ` ${context}` : '' + } tries to reference ${variable}, which is not defined.`; + if (throwImmediately) { + throw new Error(warning); + } else { + GroupMessages.add(PROPERTY_REFERENCE_WARNINGS, warning); + } to_ret = ref; } stack.pop(); diff --git a/package-lock.json b/package-lock.json index b745ea85e..5bf9f8c54 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "style-dictionary", - "version": "4.0.0-prerelease.5", + "version": "4.0.0-prerelease.6", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "style-dictionary", - "version": "4.0.0-prerelease.5", + "version": "4.0.0-prerelease.6", "hasInstallScript": true, "license": "Apache-2.0", "dependencies": { diff --git a/types/Config.d.ts b/types/Config.d.ts index c552b97d7..1175c809a 100644 --- a/types/Config.d.ts +++ b/types/Config.d.ts @@ -43,6 +43,7 @@ export interface ResolveReferenceOptionsInternal extends ResolveReferenceOptions stack?: string[]; foundCirc?: Record; firstIteration?: boolean; + throwImmediately?: boolean; } export interface PlatformConfig extends RegexOptions {