amzn / style-dictionary

A build system for creating cross-platform styles.
https://styledictionary.com
Apache License 2.0
3.93k stars 557 forks source link

Conflicts with outputReferences when custom transform changes token value #974

Closed germain-gg closed 1 year ago

germain-gg commented 1 year ago

I am using tokens.studio and their set of style-dictionary transforms https://github.com/tokens-studio/sd-transforms

Specifically a transform called ts/resolveMath. It appears that the transform is applied before the formatting phase. And one part of the system gets confused and the output is invalid CSS. See a more in-depth investigation by the package maintainer

 Reproduction steps

{
  "space": {
    "scale": {
      "value": "4",
      "type": "spacing",
      "description": "Base unit for spacing scale."
    },
    "6x": {
      "value": "{space.scale} * 6",
      "type": "spacing"
    },
    "7x": {
      "value": "{space.scale} * 7",
      "type": "spacing"
    }
  }
}

And a style-dictionary config of

{
  "transformGroup": "css-tokens",
  "transforms": [
    "ts/resolveMath",
    "ts/size/px",
    "name/cti/kebab"
  ],
  "files": {
    "destination": "style.css",
    "format": "css/variables",
    "options": {
      "outputReferences": true
    }
  }
}

Expected output

:root {
  --space-scale: 4px;
  --space-7x: 28px;
  --space-6x: 24px;
}

Actual output

:root {
  --space-scale: 4px;
  --space-7x: 28px;
  --space-6x: 2var(--space-scale);
}
chazzmoney commented 1 year ago

Thanks for your note.

After looking into it, there is indeed a conflict between ts/resolveMath and "outputReferences": true. Using both of these will cause problems because they both work functionally on strings.

For the output you wish to see, I would suggest removing the "outputReferences": true. This should create the output that you want. If, however, you wanted something more like:

:root {
  --space-scale: 4px;
  --space-7x: var(--space-scale) * 7;
  --space-6x: var(--space-scale) * 6;
}

Then you can leave in "outputReferences": true and remove the ts/resolveMath transform.

In general these two systems both intend to process the strings (and have specific expectations of what those strings look like) to create different outputs, so should not be used together.

chazzmoney commented 1 year ago

@jorenbroekema if you have additional thoughts, happy to hear them

jorenbroekema commented 1 year ago

Another related issue here https://github.com/tokens-studio/sd-transforms/issues/121 where I share my thoughts.

Basically, the regex replacement that outputReferences does on the current value doesn't work if your value has been transformed, e.g. by means of a transitive transform like a math resolver or something simpler like demonstrated here

At first, my suggestion was also that outputReferences and transforms that alter the value of a token, cannot be combined, however, I came up with an idea that I'd like to explore:

It might work better if the replace done by style-dictionary was done on the original value, like:

value = original.value.replace(`{${ref.name}}`, `var(--${ref.name})`);

where ref.name = "foo" and original.value = "{foo}". See also, https://github.com/amzn/style-dictionary/blob/main/lib/common/formatHelpers/createPropertyFormatter.js#L115

The only problem I see with that, is that this solution actually means that transforms that adjust the value of the token would just be ignored, unless that same transform is done on the referenced value as well, in that case it would be fine..

Also, it would mean for a math expression value like "{foo} + {bar}" you would get the outcome of var(--foo) + var(--bar), 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, using a custom CSS format? Anyway, with regards to math expressions, this is something we are still actively discussing, whether it belongs inside the design token format, whether as a string, an AST-like object, or just as something you do in a step before you get your tokens JSON. So I wouldn't worry too much about the math edge case... rather focus on the "simpler demo" I linked :)

jorenbroekema commented 1 year ago

I'm happy to explore this suggestion btw and create a PR, if it seems like an ok suggestion at first glance.

jorenbroekema commented 1 year ago

Will be part of Style-Dictionary v4