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

Conflicts between ts/resolveMath and outputReferences #13

Closed
Tracked by #62
germain-gg opened this issue Feb 13, 2023 · 8 comments
Closed
Tracked by #62

Conflicts between ts/resolveMath and outputReferences #13

germain-gg opened this issue Feb 13, 2023 · 8 comments
Labels
documentation Improvements or additions to documentation external Related to another repository, e.g. Style-Dictionary wontfix This will not be worked on

Comments

@germain-gg
Copy link
Contributor

germain-gg commented Feb 13, 2023

Hi,

We've started using tokens.studio fairly recently and are transforming the tokens to be consumed by web/android/ios. I have encountered a weird issue that I think I've narrowed down to ts/resolveMath.

This issue seems to happen only for a handful of values across the entire spacing scale we use. It gives a buggy output for the multipliers 6, 10, 11, 12, 16, 36, 56.

Below are the repro steps for this issue as trimmed out as I could.
Could try and have a look at the issue if you can think of an area that could be the culprit

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 the following style-dictionary config:

{
  "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);
}
@jorenbroekema
Copy link
Member

Yes I have also noticed a few problematic scenarios myself when doing some testing with this.. thanks for reporting it, I'll try to dig into these soon!

@germain-gg
Copy link
Contributor Author

@jorenbroekema much appreciated! Do you keep a list of those problematic scenarios somewhere? That'd be quite useful to anticipate and try to navigate around those issues as we're building our tokens codebase

@jorenbroekema
Copy link
Member

From the top of my head, but not 100% sure anymore:

  • 4px * 8px would not compute, 4 * 8px or 4px * 8 would. I think I also had an issue where if I used a scale of 4 but I put "type": "dimension" on that token, it would also fail
  • combining math expressions as well as multi-value tokens in one, also would make the math fail, e.g. a button padding could be: "8px 4px" with a space in between, but if either of those were a math expression, the resolving of that math would fail, because the resolver doesn't understand the concept of space separated multi-value (yet)

germain-gg added a commit to element-hq/compound-design-tokens that referenced this issue Feb 13, 2023
As a temporary workaround for tokens-studio/sd-transforms#13

This commit needs to be reverted once the issue above has been resolved
@jorenbroekema
Copy link
Member

@gsouquet I believe your issue is related to combining transformGroup with transforms, see also amzn/style-dictionary#813

This was very counter-intuitive to me as well, so I hope they improve it or document it more clearly, but I'd try to see if you can solve your issue by only using a single transformGroup, OR array of standalone transforms. I've had similar weirdness with my tokens when I was combining transformGroup and transforms together.

@germain-gg
Copy link
Contributor Author

germain-gg commented Feb 14, 2023

@jorenbroekema I have removed the transformGroup property from my sd config and still obtain the same unexpected output

@jorenbroekema
Copy link
Member

I actually also don't really understand why your expected output is this way, because outputReferences for css/variables format would introduce CSS custom properties https://amzn.github.io/style-dictionary/#/formats?id=references-in-output-files , pointing to the reference value. Since outputReferences would replace the resolved value with the original ref value as CSS Custom prop, even if you resolve the math expression correctly, I wouldn't expect it to be in there at all.

So yeah, my expected output would be:

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

or perhaps at best:

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

which is also not very helpful, because a raw math expression like that doesn't work in CSS without wrapping it inside calc()

I doubt StyleDictionary understands what to do when restoring reference value (for example the CSS format), if it's combined with a math expression.
Generally speaking I think StyleDictionary will only understand and support "value": "{some.reference}".

If you want --space-6x: 24px it would make more sense for me to remove outputReferences: true, because you don't want a reference value, you want the resolved one. If you really need outputReferences: true, you could write a custom CSS format that mimicks outputReferences: true and throws a calc() around it if there are math operators involved, https://amzn.github.io/style-dictionary/#/formats?id=custom-format-with-output-references <-- this resource might help with that

The math expressions as introduced by Tokens Studio is not something that's seen as a standard, StyleDictionary or its options/features (like outputReferences) don't know about it, perhaps if this feature makes the Design Tokens Specification, it would also land in StyleDictionary, in the future, but for now it's probably best to work around this limitation

@jorenbroekema
Copy link
Member

jorenbroekema commented Mar 28, 2023

I've jumped into this again today and I figured out what exactly is causing this conflict, and I produced a minimal reproduction here that doesn't involve our resolve math transform, but rather shows the problem is more generic to style-dictionary and transform of type "value" causing referencing tokens and its reference token values to diverge: https://configurator.tokens.studio/#project=jVPBbpwwEP0Vy5fNbllQD71QbS+NcmtVNb2FVWVg2LgBG9njVVeIf4/HBBaIEoUDsmfevJk3nul4oVUlT/E/qxVPeZcpxjJutTMFZDxlD2Qg026X7GLUT6BsAGecPMdoCGhrgZU2jaWYbowp7PxOFjRC2Qn4MDrIpUQDSYEyeYJc5BmP5s7m8t1Z1M2fMX7lR5sYsLo+ww+Bj0Nt9B2ja+7WQCX/U14vsJwRZDx3si5/hch0vCa++mSBqmQN67q765EgJViUSqCkBhHV37MwUuQ+MA7diJZ4kiJwgFK+Cf0KqVsiXfZz8jlsHf6GCgyoYqgRjYM5rr9epuNxOIS7//U84gZO0iKY+9s7p4qQ0j+3nwzZtNogu8dLDbcyeIS5sMrohm0sWfflZN58zVSmVth45J4e8SYowUsLpP8sal/yoJuGIX3n3cMcSZRnGJQGo+9k8QgmZTdhTrfs8I2FU0x07HA4+ErLvW1FAftcmM2ViqhXkT9dk4OJW2Es3NVa4OCLQ5lb9ol99uH91iv1bbPoSqnn+zFbJsq32ItK69VevGgnyV8WM0fNeRlZTyPVaZztPhrZvJK32bqQPfYZ+w/yTsPQPwM=

As you can see, outputReferences is not working. This is because we've involved a custom transform that changes the token value of the foo token, which is referenced by bar. This happens on a transform level which happens before formats happen. Output references option works on a format level, and what it does under the hood (in style-dictionary lib) is use String.replace() to replace the token value of bar (1 in my example, because it refers to foo which equals 1), with var(--sd-space-foo), but it does so by use of regex, trying to match the 1. However, we applied a custom transform that transforms the foo token to add 1 -> which results in 2. So foo value has been transformed to 2, but bar is 1 , so even though bar is referencing foo, style-dictionary can't replace that 1 with var(--sd-space-foo) anymore, because the value that's being referenced is not 1 anymore.

Conclusion

Style-Dictionary outputReferences feature does not work if the referenced token value has been transformed by a transform, but the token that is doing the referencing hasn't, or vice versa. If the referencing token and its respective reference token have "diverged" in value, the outputReferences option in your format will not work as expected.

Most transforms in this repository that transform the value happen after references have been resolved (transitive: true), so they will not cause this divergence because both the referencing token and reference token will be transformed in the same way. However, a value that is 1 part reference and 1 part something else, e.g. {space.foo} * 10 math expression, can definitely cause this divergence, e.g. through our resolveMath transform which only matches math expression values, so that transforms the token without its reference being transformed in the same way.

I have marked it as "documentation" because it would be good for us to document this quirk/limitation, but the limitation is mostly on style-dictionary's side, hence why I marked it external. Might be worth creating an issue on their repository.

For what it's worth, the reason you got your output:

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

is because normally it would be 24px, and if you do 24px.replace('4px', 'var(--space-scale)'` ('4px' being the reference value) then that's the output you get.. Took me a while to understand 😅

@jorenbroekema jorenbroekema added wontfix This will not be worked on external Related to another repository, e.g. Style-Dictionary labels Mar 28, 2023
@germain-gg
Copy link
Contributor Author

Thank you for the investigation and the very detailed answer here!

I'll open an issue on style-dictionary and see whether we can figure out an approach to work around this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation external Related to another repository, e.g. Style-Dictionary wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants