style-dictionary icon indicating copy to clipboard operation
style-dictionary copied to clipboard

Crash when mixing refs and complex values in transitive transform

Open michaelurban opened this issue 2 years ago • 11 comments

I'm trying to do something that I haven't seen documented, but, should(?) work. It almost works, at any rate.

Example project here: box-shadow-example

I'm specifying box-shadows as a list:

{
  "box-shadow": {
    "level1": {
      "value": [
        {
          "blurRadius": "2px",
          "color": "{color.shadow.key.value}",
          "offsetX": "0",
          "offsetY": "1px",
          "spreadRadius": "0"
        },
        {
          "blurRadius": "3px",
          "color": "{color.shadow.ambient.value}",
          "offsetX": "0",
          "offsetY": "1px",
          "spreadRadius": "1px"
        }
      ]
    }
}

tokens/box-shadows.refs.json

And, I'm using a transform to convert that into a valid CSS box-shadow property value.

Expected Output (Ideal):

:root {
  --bse-color-shadow-ambient: rgba(0, 0, 0, 0.15);
  --bse-color-shadow-key: rgba(0, 0, 0, 0.3);
  --bse-box-shadow-level2: 0 1px 2px var(--bse-color-shadow-key),
    0 2px 6px var(--bse-color-shadow-ambient);
  --bse-box-shadow-level1: 0 1px 2px var(--bse-color-shadow-key),
    0 1px 3px var(--bse-color-shadow-ambient);
  --bse-component-rect-height: 90px;
  --bse-component-rect-width: 160px;
  --bse-component-rect-box-shadow: var(--bse-box-shadow-level1);
}

Alternatively, if it's not possible to output the token references, this would work as the expected output too:

Expected Output (Alternative):

:root {
  --bse-color-shadow-ambient: rgba(0, 0, 0, 0.15);
  --bse-color-shadow-key: rgba(0, 0, 0, 0.3);
  --bse-box-shadow-level2: 0 1px 2px rgba(0, 0, 0, 0.3),
    0 2px 6px rgba(0, 0, 0, 0.15);
  --bse-box-shadow-level1: 0 1px 2px rgba(0, 0, 0, 0.3),
    0 1px 3px rgba(0, 0, 0, 0.15);
  --bse-component-rect-height: 90px;
  --bse-component-rect-width: 160px;
  --bse-component-rect-box-shadow: var(--bse-box-shadow-level1);
}

Actual Output:

if (a.original && dictionary.usesReference(a.original.value)) {
          ^

TypeError: Cannot read properties of undefined (reading 'original')
    at sorter (/<project path>/box-shadow-example/node_modules/style-dictionary/lib/common/formatHelpers/sortByReference.js:35:11)
    at sorter (/<project path>/box-shadow-example/node_modules/style-dictionary/lib/common/formatHelpers/sortByReference.js:57:16)
    at Array.sort (<anonymous>)
    at formattedVariables (/<project path>/box-shadow-example/node_modules/style-dictionary/lib/common/formatHelpers/formattedVariables.js:56:32)
    at Object.css/variables [as format] (/<project path>/box-shadow-example/node_modules/style-dictionary/lib/common/formats.js:49:7)
    at buildFile (/<project path>/box-shadow-example/node_modules/style-dictionary/lib/buildFile.js:103:37)
    at /<project path>/box-shadow-example/node_modules/style-dictionary/lib/buildFiles.js:33:7
    at Array.forEach (<anonymous>)
    at buildFiles (/<project path>/box-shadow-example/node_modules/style-dictionary/lib/buildFiles.js:31:18)
    at Object.buildPlatform (/<project path>/box-shadow-example/node_modules/style-dictionary/lib/buildPlatform.js:62:3)

When I log the token values post-transform, I can see that the references are being substituted correctly:

0 1px 2px rgba(0, 0, 0, 0.3), 0 1px 3px rgba(0, 0, 0, 0.15)
0 1px 2px rgba(0, 0, 0, 0.3), 0 2px 6px rgba(0, 0, 0, 0.15)

The weird thing is, if I omit token references and hard-code the color values, my transform works as expected:

{
  "box-shadow": {
    "level1": {
      "value": [
        {
          "blurRadius": "2px",
          "color": "black",
          "offsetX": "0",
          "offsetY": "1px",
          "spreadRadius": "0"
        },
        {
          "blurRadius": "3px",
          "color": "black",
          "offsetX": "0",
          "offsetY": "1px",
          "spreadRadius": "1px"
        }
      ]
    }
}

tokens/box-shadows.no-refs.json

Expected and actual output:

:root {
  --bse-box-shadow-level2: 0 1px 2px black, 0 2px 6px black;
  --bse-box-shadow-level1: 0 1px 2px black, 0 1px 3px black;
  --bse-component-rect-height: 90px;
  --bse-component-rect-width: 160px;
  --bse-color-shadow-ambient: rgba(0, 0, 0, 0.15);
  --bse-color-shadow-key: rgba(0, 0, 0, 0.3);
  --bse-component-rect-box-shadow: var(--bse-box-shadow-level1);
}

Bug? My fault? I won't be able to dig into the internals of SD for a bit. If anyone can tell me if this is a bug or why what I'm trying to do is impossible, I'd appreciate it.

michaelurban avatar Mar 17 '22 11:03 michaelurban

Is there a mailing list or a more active forum for Style Dictionary?

michaelurban avatar Mar 21 '22 13:03 michaelurban

Is anyone monitoring the bug tracker?

michaelurban avatar Apr 03 '22 18:04 michaelurban

My apologies, this project is community driven and supported by a few individuals. I recently had a 2nd baby and am dealing with a lot of work and personal things at the moment. We do monitor the issues and PRs and try to get them resolved in a timely manner, but some weeks/months things get slow. Thank you for your patience. Now onto your question.

It appears our sorting and reference logic in our built-in formats does not handle arrays of objects as values well. It can handle an object as a value or a simple array, but not an array of objects.

I see the issue is our internal usesReference function returns true because it searches the entire value object all levels deep. But our getReferences function only looks at the first level of the object. I came up with a fix for it: #812

dbanksdesign avatar Apr 19 '22 22:04 dbanksdesign

Congrats on the baby! Or, sorry about the second kid. I don't know your situation.

Thanks for the fix, will check it out tomorrow. I ended up implementing everything using regexs, but I'm keen to use objects instead of my CSS derived DSL.

While I have you here. Style Dictionary has been great to work with. The docs are complete and absolutely everything just works as you would expect it to.

michaelurban avatar Apr 20 '22 00:04 michaelurban

@dbanksdesign I've been following your commits related to this, thanks for taking it on!

michaelurban avatar May 18 '22 20:05 michaelurban

I realized this is not released to npm yet, I was going to try to release today, but found a bug in our releasing scripts. Once I get that fixed and merged I can do a release to get this out!

dbanksdesign avatar May 24 '22 21:05 dbanksdesign

@dbanksdesign boom

michaelurban avatar May 25 '22 15:05 michaelurban

@dbanksdesign @michaelurban i am trying to transform complex box shadow tokens like as described in the beginning of this thread but cannot iterate over the array of objects to extract values. is there a fix in place? Thank you in advance guys!

  "box-shadow": {
    "level1": {
      "value": [
        {
          "blurRadius": "2px",
          "color": "{color.shadow.key.value}",
          "offsetX": "0",
          "offsetY": "1px",
          "spreadRadius": "0"
        },
        {
          "blurRadius": "3px",
          "color": "{color.shadow.ambient.value}",
          "offsetX": "0",
          "offsetY": "1px",
          "spreadRadius": "1px"
        }
      ]
    }```

lexguru1 avatar May 09 '23 12:05 lexguru1

Hello! I am curious if there have been any updates to this, as I am bumping into the same issue when dealing with shadows.

Also, @michaelurban - curious if you would be willing/able to share a more fully functional version of the transform code you were working with for shadows, and what (if anything) got things working for you?

Thanks! 🙏 🤜 🤛

tklives avatar Nov 09 '23 21:11 tklives

Chiming in on boxShadow. Everything generated seems well and good, except for some boxShadow transforms which looked like this in my scss and css files:

$boxShadowSensorDanger: {boxShadow.sensor.danger.dim};
$boxShadowSensorError: {boxShadow.sensor.error.dim};
$boxShadowSensorOk: {boxShadow.sensor.ok.dim};
$boxShadowNone: 0 0 0 0 $colorOtherPaletteNone;

They point to these tokens:

"boxShadow": {
  "sensor": {
      "ok": {
        "dim": {
          "value": {
            "x": "{sizing.custom.shadows.sensor.on.x}",
            "y": "{sizing.custom.shadows.sensor.on.y}",
            "blur": "{sizing.custom.shadows.sensor.on.blur}",
            "spread": "{sizing.custom.shadows.sensor.on.spread}",
            "color": "{color.feedback.foreground.ok}",
            "type": "dropShadow"
          },
          "type": "boxShadow"
        },
}

which again points to another file with these tokens:

"sizing": {
    "custom": {
      "shadows": {
        "sensor": {
          "on": {
            "x": {
              "value": "{dimension.none}",
              "type": "sizing"
            },
            "y": {
              "value": "{dimension.none}",
              "type": "sizing"
            },
            "blur": {
              "value": "{dimension.0004x}",
              "type": "sizing"
            },
            "spread": {
              "value": "{dimension.none}",
              "type": "sizing"
            }
          },
...

My initial guess was that the nested references might cause the issue. Either way, I'm supplying an additional case to the mix. In case it is of relevance, I'm using tokens-studio for my transformGroup, so this might be an issue to take with them instead.

MathiasGronseth avatar Apr 25 '24 10:04 MathiasGronseth

I'm new to this issue in particular but this week I'm adding some stuff to style-dictionary v4 to deal with object-value tokens (aka composite tokens) and I'm planning to bring the CSS shorthand transforms from sd-transforms into style-dictionary, since these token types are now part of the DTCG spec and not Tokens Studio specific.

I'll keep this issue open in a browser tab and ensure I add some tests for the use cases mentioned in this issue. Just for transparency, the original post highlights an ideal output and alternative output, I'm aiming for the alternative output here and the reason for that is described in this issue: https://github.com/amzn/style-dictionary/issues/1124, you'll have to use the outputReferencesTransformed utility to tell it to not output a reference when the value was transitively transformed, which is the case for a transform that turns the object value into a CSS shadow shorthand value. This transform action must not be undone by outputting a ref, and this utility takes care of this by doing a diff between the original value (references resolved) and the transformed value, and identifying that outputting a ref when there is a diff, could cause issues. The issue I linked is a bit of a long read because outputting references and transitive transforms is a very tricky combination, but hopefully it makes sense

jorenbroekema avatar Apr 25 '24 10:04 jorenbroekema