Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash when mixing refs and complex values in transitive transform #795

Open
michaelurban opened this issue Mar 17, 2022 · 11 comments
Open

Comments

@michaelurban
Copy link

michaelurban commented Mar 17, 2022

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
Copy link
Author

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

@michaelurban
Copy link
Author

Is anyone monitoring the bug tracker?

@dbanksdesign
Copy link
Member

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

@michaelurban
Copy link
Author

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
Copy link
Author

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

@dbanksdesign
Copy link
Member

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!

@michaelurban
Copy link
Author

@dbanksdesign boom

@lexguru1
Copy link

lexguru1 commented May 9, 2023

@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"
        }
      ]
    }```

@tklives
Copy link

tklives commented Nov 9, 2023

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! 🙏 🤜 🤛

@MathiasGronseth
Copy link

MathiasGronseth commented Apr 25, 2024

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.

@jorenbroekema
Copy link
Collaborator

jorenbroekema commented Apr 25, 2024

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: #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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants