[Bug]: ts/resolveMath unexpected results with outputReferences

[Aries0d0f]

What happened?

Referenced value with math calculation returns unexpected results when outputReferences is set to true. It might be a bug in ts/resolveMath.

I think there's a bug in the part of regex replacement.

sd-transforms/src/checkAndEvaluateMath.ts

Lines 68 to 103 in db489d4

	function parseAndReduce(expr: string): string {
	// We check for px unit, then remove it
	const hasPx = expr.match('px');
	let unitlessExpr = expr.replace(/px/g, '');
	// Remove it here so we can evaluate expressions like 16px + 24px
	const calcParsed = parse(unitlessExpr, {});
	// Attempt to reduce the math expression
	const reduced = reduceExpression(calcParsed);
	let unit;
	// E.g. if type is Length, like 4 * 7rem would be 28rem
	if (reduced && reduced.type !== 'Number') {
	unitlessExpr = `${reduced.value}`.replace(new RegExp(reduced.unit, 'ig'), '');
	unit = reduced.unit;
	}
	// Try to evaluate expression (minus unit) with expr-eval
	let evaluated;
	try {
	evaluated = parser.evaluate(unitlessExpr);
	} catch (ex) {
	return expr;
	}
	// Put back the px unit if needed and if reduced doesn't come with one
	return `${Number.parseFloat(evaluated.toFixed(3))}${unit ?? (hasPx ? 'px' : '')}`;
	}
	export function checkAndEvaluateMath(expr: string | number | undefined): string | undefined {
	if (expr === undefined) {
	return expr;
	}
	const exprs = splitMultiIntoSingleValues(`${expr}`);
	const reducedExprs = exprs.map(_expr => parseAndReduce(_expr));
	return reducedExprs.join(' ');
	}

Reproduction

The following can be reproduced on the Online Token Studio Configurator with the prepared environment.

1. Tokens.json with some value references to a base value, then do some math calculations
2. Set option outputReferences to true
3. Build and lookup the result in build/css/variables.css
4. See the issue

/**
 * Do not edit directly
 * Generated on Fri, 12 May 2023 07:41:37 GMT
 */

:root {
  --refSize100: 8px;
  --refSizeBase: var(--refSize100);
  --refSize75: 4px;
  --refSize200: 12px;
  --refSize300: 16px;
  --refSize500: 24px;
  --refSize600: 32px;
  --refSize700: 40px;
  --refSize800: 4var(--refSizeBase); /* This should be 48px */
  --refSize900: 56px;
  --refSize1000: 64px;
  --refSize50: 2px;
  --refSize400: 20px;
}

When the option outputReferences is unset, the result works as expected.

Expected output

/**
 * Do not edit directly
 * Generated on Fri, 12 May 2023 07:41:37 GMT
 */

:root {
  --refSize100: 8px;
  --refSizeBase: var(--refSize100);
  --refSize75: 4px;
  --refSize200: 12px;
  --refSize300: 16px;
  --refSize500: 24px;
  --refSize600: 32px;
  --refSize700: 40px;
  --refSize800: 48px;
  --refSize900: 56px;
  --refSize1000: 64px;
  --refSize50: 2px;
  --refSize400: 20px;
}

Version

0.8.6

[jorenbroekema]

It's actually a problem with the regex replacement in outputReferences in style-dictionary: https://github.com/amzn/style-dictionary/blob/main/lib/common/formatHelpers/createPropertyFormatter.js#L115

Basically, the way it works is, if you have a token:

{
  "foo": {
    "value": "{bar}"
  },
  "bar": {
    "value": "4px"
  }
}

"foo" will be resolved to "4px" but we keep track of the fact that the original value was a reference value, referencing "bar".

Then, when we get to formatting it to CSS variables, if outputReferences is true, it will look at "foo"'s original value (which is the value of "bar" -> "4px"), and do a replace on the current value value = value.replace(bar, 'var(--bar)') where bar = "4px".

What that means is, if your "value" has been changed after the resolving process, e.g. with a transitive transform (like resolveMath transformer, which must be transitive for it to work at all), the replace will not work, or in some cases it will cause weird behavior like you see with --refSize800: 4var(--refSizeBase); /* This should be 48px */ , the reason this is partially replaced is because refSizeBase is "8px", so "48px" is then actually matched by "8px" and replaced, meaning you get 4var(--refSizeBase).

It might work better if the replace done by style-dictionary was done on the original value, like value = original.value.replace(ref, 'var(--bar)') where ref = "{bar}", although that would mean for a value like "{bar} + {qux}" you would get the outcome of var(--bar) + var(--qux), which you would need to wrap in a calc() statement for it to work in CSS, but hey that's kind of doable because we can scan the outputted value and determine if it's a math expression, and wrap it in a calc() statement if so..

This issue is a duplicate of #13 which I closed because it cannot be fixed in this repo, it would require a fix in style-dictionary, which I think is reasonable to ask because basically, outputReferences doesn't work in general if a transitive transform changes the value, which is also demonstrated in that issue quite well, so it shows it's not just a problem caused by our ts/resolveMath transform.

@gsouquet did you end up creating an issue on style-dictionary for this? Because if we can get their view on this and they agree with my idea to fix it, I am willing to create a PR.

[germain-gg]

@jorenbroekema I have not, apologies for this. But could this afternoon if that helps!

[jorenbroekema]

Yeah would be great, I'll tag along and share my 2 cents if anything is missing

[germain-gg]

@jorenbroekema I've opened amzn/style-dictionary#974

[lukethacoder]

amzn/style-dictionary#820 (comment) mentions a somewhat "hacky" solution that looks to preserve the css variables. See comments on the linked issue as to why this is hacky, pretty much flags that strings could contain * which would you would not want calc() applied.

const StyleDictionary = require('style-dictionary');

StyleDictionary.registerTransform({
  type: `value`,
  transitive: true,
  name: `figma/calc`,
  matcher: ({ value }) => typeof value === 'string' && value?.includes('*'),
  transformer: ({ value }) => `calc(${value})`,
});

This would give you an output of:

--refSize800: calc(4 * var(--refSizeBase));

[jorenbroekema]

@lukethacoder this quite easily runs into issues though when you have chained references:

• Transitive
• Avoids the duplicates

In both cases the output is not what you want.

The reason your workaround was working is:

• no chained references
• outputReferences utility as it used to work is that it would do a regex replace on the value e.g. 4 * 8px where refSizeBase is resolved (assuming 8px for this example), but nowadays it does a regex replace on the original.value 4 * {ref.size.base} to play nicer with transitive transforms, which this issue outlines.

What I think we have here is another use case for deferred transforms, where you control the fact that it runs after all other transforms, including transitive ones. see: amzn/style-dictionary#1063

[jorenbroekema]

I am closing this issue for the following reasons:

• Conflicts with outputReferences when custom transform changes token value amzn/style-dictionary#974 has been fixed & closed
• Transforms that run after all references have been resolved amzn/style-dictionary#1063 has been opened to allow more control for deferring format-specific transforms so that we can run evaluate/compute type transforms first, and format at the end (e.g. putting calc() around a token that contains references and is also a math operation)
• Generally speaking there is a fundamental contradiction when we talk about having a token value be a reference in the output, versus a token value being transformed in its own scope. One cannot both be an alias for another, and have its own unique value at the same time, in the final output.

outputReferences now works better (since 3.9.0) with transitive transforms in that it just undo's the work because it puts back the original.value in the output as a CSS variable, so that's an improvement over how it worked before. Deferred transforms should allow us in the future to go further with transforming stuff after all references are resolved.