feat: New checkAndEvaluateMath function that prevents unit mixing by floscr · Pull Request #357 · tokens-studio/sd-transforms

floscr · 2025-05-27T07:34:26Z

Draft PR on sd-transform checkAndEvaluateMath to get feedback from token-studio team and outside contributers if wanted.

Changes

Not allowing math on relative units like 10em + 10px this doesn't make sense IMO to resolve as em is relative to the dom context that we don't know about.
This is something that wont be allowed from penpot, if we really want to keep the previous way it could be done with a flag.
Throwing explicit errors to work nicely with the new SD errors API
Removed the post css transform, it wasnt required with the new rules.
Unitless numbers don't get converted to px anymore, this shouldn't be inferred by the transform but defined by the user

Issues

Composite token values error handling

Composite token values are an issue for this for this with the new error handling:

          value: {
            width: '6px / 4',
            style: 'solid',
            color: '#000',
          },

It will throw on style and color, which shouldn't be handled in the first place by the math expression.

I think it would make sense to explicitely filter out properties that can be evaluated.

Discussion Points

New major version / New transform / strict property in config?

Possibly we call this strictCheckAndEvaluateMath which is much more simplified and doesn't result in unexpected outputs as the previous version did in regards to unit mixing.

If users want to mix units as it would work in css (e.g.: calc(10em + 10px)) there should be a new transform that runs after this one to wrap into css calc expressions.

Px Conversion of unitless at the end

When no unit is given I don't think we should append px to it and keep it as an Int.
If users want to have px values they should either define it in a config or add an extra transform.

changeset-bot · 2025-05-27T07:34:30Z

⚠️ No Changeset found

Latest commit: e6d1b21

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

test: update tests to throw error for mixed units refactor: Add custom MixedUnitsExpressionError for better error handling refactor: simplify unit parsing logic and add debug logging fix: handle multiplication with different units in math expressions Detect units Unit parsing Unit tests Last tests

floscr · 2025-05-27T07:44:38Z

test/spec/checkAndEvaluateMath.spec.ts

-    expect(
-      checkAndEvaluateMath({
-        value: 'linear-gradient(90deg, #354752 0%, #0b0d0e 100%)',
-        type: 'dimension',
-      }),
-    ).to.equal('linear-gradient(90deg, #354752 0%, #0b0d0e 100%)');


This should be filtered out before this transform, linear gradient on a dimension token doesnt make any sense...

As the comment says, it's just a smoke-test that ensures we don't handle values that aren't math expressions but do happen to have space seperated substrings. This could be put into its own test.

Math transform applies to all tokens that are strings. It doesn't exclude token types that aren't dimensions. We just need to check that a string value is actually a valid math expression and ignore it if it's not, that's what this expect verifies.

floscr · 2025-05-27T07:44:54Z

test/spec/register.spec.ts

  --spacingSm: 8px;
  --spacingXl: 64px;
-  --spacingMultiValue: 8px 64px; /* You can have multiple values in a single spacing token. Read more on these: https://docs.tokens.studio/available-tokens/spacing-tokens#multi-value-spacing-tokens */
+  --spacingMultiValue: 8px 64px; /** You can have multiple values in a single spacing token. Read more on these: https://docs.tokens.studio/available-tokens/spacing-tokens#multi-value-spacing-tokens */


Bug in new SD version? It's either missing an extra * at the end or the extra * should be removed.

Feature! This makes generated comments, whether multi or single line, compatible with JSDocs, just a small enhancement. This way folks can put for example @deprecated into the comment.

floscr · 2025-05-27T07:56:48Z

src/checkAndEvaluateMath.ts

+ * Numbers without units will be represented in the units set with an empty string "".
+ */
+export function parseUnits(expr: string): { units: Set<string>; unitlessExpr: string } {
+  const unitRegex = /(\d+\.?\d*)(?<unit>([a-zA-Z]|%)+)?/g;


Additionally I would throw on any unsupported units which should be definable by the user in the platform config.
For penpot we would allow unitless|px|rem for example

DarioSoller · 2025-05-27T08:46:40Z

@floscr do you mind to sum up a brief PR description? An example on what this PR is changing, would be nice. THX!
Also I would be happy to test this against my token set.

floscr · 2025-05-27T09:01:18Z

@floscr do you mind to sum up a brief PR description? An example on what this PR is changing, would be nice. THX! Also I would be happy to test this against my token set.

Sure! I've moved internal discussion description and points here, but please concider that this is still a draft PR.

fix: pixel expression check not needed anymore chore: Cleanup

…teMath

jorenbroekema · 2025-05-29T11:59:50Z

src/checkAndEvaluateMath.ts

-      mathChars.includes(right) && mathChars.includes(left), // left/right are both math chars
-      left === '' && mathChars.includes(right), // tail of expr, right is math char
-      right === '' && mathChars.includes(left), // head of expr, left is math char
+      mathChars.has(tok), // current token is a math char


Nice optimization 👍🏻

jorenbroekema · 2025-05-29T12:02:00Z

test/integration/expand-composition.test.ts

      end: '--sdDeepRef: italic 800 26px/1.25 Arial;',
    });
-    const expectedOutput = `--sdCompositionSize: 24px;
+    // TODO: Why did this change?


SD v5 refactored quite a few things about composite token expansion, optimizing it a lot. I suspect this is why the order of some tokens has changed because in the old SD we expanded tokens and then flattened, in the new SD we first flatten and then expand.

jorenbroekema · 2025-05-29T12:03:42Z

test/spec/checkAndEvaluateMath.spec.ts

    expect(checkAndEvaluateMath({ value: '4em + 7em', type: 'dimension' })).to.equal('11em');
-    expect(checkAndEvaluateMath({ value: '4 + 7rem', type: 'dimension' })).to.equal('4 + 7rem');
-    expect(checkAndEvaluateMath({ value: '4em + 7rem', type: 'dimension' })).to.equal('4em + 7rem');
+    expect(() => checkAndEvaluateMath({ value: '4 + 7rem', type: 'dimension' })).to.throw(


This seems fine by me but we do need to ensure we have an integration test somewhere that verifies that the token value remains unchanged if the math throws an error. So the behavior of the token value is actually the same (which is important for backwards compatibility), except this function throws now.

jorenbroekema · 2025-05-29T12:08:01Z

test/spec/checkAndEvaluateMath.spec.ts

-    expect(checkAndEvaluateMath({ value: '4px * 7em', type: 'dimension' })).to.equal('28em');
-    // 50em would be incorrect when dividing, as em grows, result should shrink, but doesn't
-    expect(checkAndEvaluateMath({ value: '1000 / 20em', type: 'dimension' })).to.equal(
-      '1000 / 20em',
+    expect(() => checkAndEvaluateMath({ value: '4px * 7em', type: 'dimension' })).to.throw(
+      MixedUnitsExpressionError,


Technically you can mix units with pixels, assuming the unit that you mix with resolves to pixels (e.g. is a multiple of). Same for division.

What I think we should do, also for backwards compatibility, is make sure that we add a platform option "ts/resolveMath:strict" which is set to false by default and allows value mixing where possible. But if strict is set to true, it will throw errors even for unit mixing that technically "can work". We will document this option and recommend it be set to true for everyone, and probably even warn users that in the future we'll probably break this and set it to true by default, because unit-mixing in design token math expressions is just not a good practice. Or we can go a step further and deprecate the option altogether to be forced to strict in the future.

The reason I wanna call it "ts/resolveMath:strict" and essentially namespace the option by the transform name, is because at this time SD only allows passing options to transforms via the platform options. And due to that scope of platform options, I think it's smart that options meant to be used by transforms are namespaced by the transform name for now.

In SD v6 we'll probably redesign that a bit and make it easier to pass options to transforms, rather than via upper scope in a hacky way.

The unit + unitless mixing is one of the issues that can't quite be solved easily with the current eval-expr, but would be solved by using the custom parser.

jorenbroekema

Looks good for the most part at least on the tests etc.

I think we need to reach a consensus on how we will provide backwards compatibility for right now:

2 separate transforms
1 transform but with an option (strict) to configure (I prefer this)

and make a plan for how we will do it in the future, e.g.:

force it to strict and don't allow to configure it (I prefer this, but we have to document clearly and give users quite a long time to move and make their tokens math expr consistent with units)
set it to strict default but allow not strict as opt-in, etc.

I might have missed it in the tests but I think that combining unitless with unit should always be allowed, given that the unit is consistent everywhere. Then we can just assume that 5rem * 5 = 25rem, and 5px * 5 = 5px etc.

I still have to more thoroughly review the actual implementation changes but that's something more easily done in a call together after this is out of draft state. So far I like what I see though.

floscr · 2025-06-02T07:38:34Z

Thanks for the feedback 🙌
I've moved the transform behind a config for now, but will also work on adding the changes from our slack channel (which I will then outline in the issue description once completed)
https://github.com/tokens-studio/sd-transforms/pull/359/files

floscr force-pushed the errors-sd-transform branch from a0e9378 to fd1a798 Compare May 27, 2025 07:35

floscr commented May 27, 2025

View reviewed changes

floscr force-pushed the errors-sd-transform branch from 08daf20 to fef81ee Compare May 27, 2025 09:29

floscr added 5 commits May 27, 2025 11:29

fix: Postcss transform not needed anymore

d2bc2c1

fix: pixel expression check not needed anymore chore: Cleanup

chore: Add todo

1d7f09f

fix: add type annotations to MixedUnitsExpressionError class

06e4938

refactor: extract and type Units as a reusable type in checkAndEvalua…

1ffe794

…teMath

chore: Use set for mathChars, move mathCharsRegexp

10033b6

floscr force-pushed the errors-sd-transform branch from fef81ee to 5ba7cdf Compare May 27, 2025 09:29

chore: Depend on StyleDictionary v5

f2eeb9a

floscr force-pushed the errors-sd-transform branch from 5ba7cdf to f2eeb9a Compare May 27, 2025 09:30

floscr added 3 commits May 27, 2025 11:43

chore: fix comment syntax integration tests failing

e305740

chore: fix new order

7abe34e

chore: remove composite rules in integration test?

e6d1b21

jorenbroekema reviewed May 29, 2025

View reviewed changes

floscr mentioned this pull request Jun 2, 2025

Strict checkAndEvaluateMath #359

Open

floscr closed this Jun 2, 2025

Conversation

floscr commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Issues

Composite token values error handling

Discussion Points

New major version / New transform / strict property in config?

Px Conversion of unitless at the end

Uh oh!

changeset-bot bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

floscr May 27, 2025

Choose a reason for hiding this comment

Uh oh!

jorenbroekema May 29, 2025

Choose a reason for hiding this comment

Uh oh!

floscr May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorenbroekema May 29, 2025

Choose a reason for hiding this comment

Uh oh!

floscr May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarioSoller commented May 27, 2025

Uh oh!

floscr commented May 27, 2025

Uh oh!

jorenbroekema May 29, 2025

Choose a reason for hiding this comment

Uh oh!

jorenbroekema May 29, 2025

Choose a reason for hiding this comment

Uh oh!

jorenbroekema May 29, 2025

Choose a reason for hiding this comment

Uh oh!

jorenbroekema May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floscr Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

jorenbroekema left a comment

Choose a reason for hiding this comment

Uh oh!

floscr commented Jun 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

floscr commented May 27, 2025 •

edited

Loading

changeset-bot bot commented May 27, 2025 •

edited

Loading

floscr May 27, 2025 •

edited

Loading

floscr May 27, 2025 •

edited

Loading

jorenbroekema May 29, 2025 •

edited

Loading