tokens-studio / sd-transforms

Custom transforms for Style-Dictionary, to work with Design Tokens that are exported from Tokens Studio
MIT License
185 stars 28 forks source link

[Bug]: References across files when expanding composite tokens / addFontStyle #217

Closed aleksrs closed 7 months ago

aleksrs commented 10 months ago

What happened?

I have the following file structure:

We're using typography tokens to more easily work with typography, but for the code built tokens i want to expand the typography tokens for the component sets, but somehow I'm having trouble resolving some references:

Warning: could not resolve reference {core.font-weight.normal}
Warning: could not resolve reference {base.typography.body}

scss                                                       
- dist/scss/tokens.scss                                    

scss                                                       
✔︎ dist/scss/tokens.scss     

It manages to expand the typography token in the base layer, but not in the component layer:

$base-typography-body-font-family: 'Open Sans';
$base-typography-body-font-weight: Regular;
$base-typography-body-line-height: 1.5;
$base-typography-body-font-size: 14px;
$base-typography-body-text-decoration: none;
$body: [object Object];

If i toggle the expand option to false in the build config, it builds the tokens correctly:

$base-typography-body: 400 14px/1.5 'Open Sans';
$comp-announcement-typography-body: 400 14px/1.5 'Open Sans';

Reproduction

A reproduction of the issue can be found here

Expected output

I want the typography tokens in the component sets to expand properly aswell

Version

0.11.9

jorenbroekema commented 10 months ago

Smallest repro

Seems related to references across multiple files, since in a single file it does work. I suppose the expand typography parser is trying to resolve references inside only the current file, but not across the whole dictionary of combined JSONs.

Affected use cases, when having cross-file references in:

jorenbroekema commented 10 months ago

Unfortunately, I cannot fix this bug right now without first introducing a new feature to Style-Dictionary.

I have created a PR to style-dictionary for Preprocessors which will allow us to change our Tokens Studio -> Style-Dictionary custom parsers (works on a per file basis) to preprocessors (works on the merged dictionary as a whole) to ensure that this bug gets resolved in sd-transforms. https://github.com/amzn/style-dictionary/pull/1043/

I'll try to give this feature high prio, as this bug is pretty annoying to get and hard to work around. Only way to work around it for now is to not have cross-file references in composite tokens that get expanded.

stringsduck commented 9 months ago

👋🏻 Hi @jorenbroekema,

Sorry for the longer message. I'm not sure if this will interest you but I was hitting a similar issue and have had an idea to resolve it.

Background My font weight tokens couldn't have the font style extracted from them due to the core font weight tokens and the typography tokens that used them coming from different source JSON files. This meant the value resolution of the font weight couldn't occur in the addFontStyle method, and consequently the matching/extraction couldn't take place.

How do solve it? So ideally this should just work... but you highlighted the difficulties of token resolution across different source files within parsers. This sounds difficult to achieve and it feels to me like it's against the fundamentals of how SD parsers are intended to work?

I had an alternative idea that might be useful (or might not be!). Could this be split into a 2 part process?

Firstly the parser could simply copy the unresolved fontWeight token value as a new fontStyle token. So for example:

// Before Parsing
value: {
  fontSize: {fontSizeToken001},
  fontWeight: {fontWeightToken002},
}

// Before Parsing
value: {
  fontSize: {fontSizeToken001},
  fontWeight: {fontWeightToken002},
  fontStyle: {fontWeightToken002},
}

After expansion and resolution you would be at a stage where you now have fontWeight and fontStyle tokens, both with the same value.

Could you then not create a couple of simple SD transforms to handle the last part of it?

One that matched fontWeights and transformed their value to remove any mention of the style from them (eg. value.replace(/italic|oblique|normal/i, ""))

One that matched fontStyles and removed any mention of the weight from them (eg. value.replace(/Regular|Bold|400|700|.../i, ""))

I understand this might not be an ideal solution but maybe the hybrid of a parser and a transformer could handle most situations without any need to hack things or create extremely complex parsing logic 🙂

jorenbroekema commented 8 months ago

Yeah I get that idea but parsers are also there to parse token files from entirely different languages/formats, like yaml, php, etc.

And our addFontWeight function is a javascript function, so it would be hard to execute that in the context of tokens that aren't JSON/JS. Parser just isn't the right method to do these kinds of processing steps, which is why I added a new lifecycle hook that happens after parsing and combining the files together into 1 dictionary: https://github.com/amzn/style-dictionary/blob/v4/docs/preprocessors.md , this is added in v4.0.0-prerelease.2 .

I have yet to upgrade sd-transforms to allow this capability for v4 users of style-dictionary. I tried to do it this week and then ran into some bottlenecks with regards to style-dictionary's type structure, so I'm first cleaning up the types and applying strict type safety throughout the style-dictionary repo, and then I'll make this upgrade :) so keep following this issue, I'll let you guys know when you can solve this issue in the latest versions of style-dictionary/sd-transforms.

stringsduck commented 8 months ago

Great response, thank you 🙂. I've not looked into SD v4 yet so it'll be interesting to see how that changes things.

We also found the need to start looking into pre-processing and post-processing scripts for a variety of reasons so exciting to see this is going to be a core part of SD.

jorenbroekema commented 8 months ago
import { expandComposites } from '@tokens-studio/sd-transforms';
import StyleDictionary from 'style-dictionary';

const sd = StyleDictionary.extend({ source: ['**/*.json'] })

sd.tokens = expandComposites(sd.tokens, { expand: { typography: true } });
sd.properties = sd.tokens;

this workaround should do it for v3 btw, just to unblock people. It runs the typography expand processing step on the full dictionary after the parsing step.

You can replace expandComposites for addFontStyles instead if you want to run the font styles step on it.

Once you're on style-dictionary v4.0.0-prerelease.2 minimum, and also the upcoming minor release of sd-transforms (after which I will be able to close this issue), you should be able to delete the workaround.

DavithkbY commented 8 months ago
import { expandComposites } from '@tokens-studio/sd-transforms';
import StyleDictionary from 'style-dictionary';

const sd = StyleDictionary.extend({ source: ['**/*.json'] })

sd.tokens = expandComposites(sd.tokens, { expand: { typography: true } });
sd.properties = sd.tokens;

this workaround should do it for v3 btw, just to unblock people. It runs the typography expand processing step on the full dictionary after the parsing step.

You can replace expandComposites for addFontStyles instead if you want to run the font styles step on it.

Once you're on style-dictionary v4.0.0-prerelease.2 minimum, and also the upcoming minor release of sd-transforms (after which I will be able to close this issue), you should be able to delete the workaround.

After applying the workaround I'm getting all these errors:

image

jorenbroekema commented 8 months ago

@DavithkbY You probably will have to set typography: false for the registerTransforms call, so it doesn't expand typography in the parsing process

jorenbroekema commented 8 months ago

As long as the StyleDictionary instance that you pass to registerTransforms() function has version v4.0.0-prerelease.2 or higher, this issue is now fixed in sd-transforms v0.13.0 :)

bengie commented 7 months ago

I'm still getting output like --top-banner-description-font-weight: [object Object]; or --top-banner-description-font-family: '[object Object]';with v4-prerelease.9 and expand.typography: true I removed the v3 workaround and changed the build script to the v4 way of working. Any idea on what I could be doing wrong?

jorenbroekema commented 7 months ago

I'm still getting output like --top-banner-description-font-weight: [object Object]; or --top-banner-description-font-family: '[object Object]';with v4-prerelease.9 and expand.typography: true I removed the v3 workaround and changed the build script to the v4 way of working. Any idea on what I could be doing wrong?

Nothing ;) it's a bug on my end. I didn't test this thoroughly enough, fixing it now and adding a more rigorous test https://github.com/tokens-studio/sd-transforms/pull/254

mikeriley131 commented 2 months ago

I'm switching from InVision's DSM to Figma Tokens Studio for my tokens authoring. I'm running into the above mentioned issues (Warning: could not resolve reference {font.weight.semibold}). I'm on style-dictionary version 3.8.0. Based on what I'm reading here there is no way to resolve the reference issues without upgrading to a pre-release version of style-dictionary v4. Is that right?

jorenbroekema commented 2 months ago

That's correct, style-dictionary v3 only had the parser lifecycle hook and no preprocessor hook, and this bug happened due to running our mutations during the parsing process which happens before the different token files are merged together as 1 dictionary. You can use the latest prerelease and the latest sd-transforms versions, and that should fix it (among many other bugs).

mikeriley131 commented 2 months ago

OK, thanks for the info. I'll try with the latest pre-release version.

mikeriley131 commented 2 months ago

@jorenbroekema Now that v1 of sd-transforms is released and v4 of Style Dictionary is released, is the following still relevant?

sd.tokens = expandComposites(sd.tokens, { expand: { typography: true } });
sd.properties = sd.tokens;
jorenbroekema commented 2 months ago

No that's no longer relevant.

See https://github.com/tokens-studio/sd-transforms/?tab=readme-ov-file#using-expand and https://v4.styledictionary.com/reference/config/#expand and how to use Expand feature in sd v4 / sd-transforms v1

mikeriley131 commented 2 months ago

Great, thanks!