-
Notifications
You must be signed in to change notification settings - Fork 28
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
[Bug]: Modifier Value References are not resolved #211
Comments
Just noticed that as soon as the value on which the modifier is applied to, also is a token reference, it works in the TokenStudio Configurator, but unfortunately not in my code. Would be cool to somewhere see the sd-transform and maybe the style-dictionary versions that the TokenStudio Configurator currently uses at that moment. |
Hi, acknowledged as bug, I'll try to find some time to look into this one this week. Bit of a hidden feature (more like a bug, but actually really useful 😅) but you can adjust the version of sd-transforms However, it does not work for the style-dictionary import, that one is unfortunately hardcoded at the moment and not really possible to dynamically fetch from the CDN for the Configurator's internal usage, although I will consider making that a feature in the future and change this from a bug (unintended feature) to something officially supported. Will also need to adjust the "Eject" functionality to keep into account these versions that the user may specify when creating the eject output. Currently the version of style-dictionary in the configurator is v4.0.0-prerelease.1 |
@jorenbroekema Thanks a lot for your answer. For me right now, having the possibility to change Style-Dictionary and SD-Transform versions, would be a very welcomed feature of the configurator. Really cool feature idea IMO. As a quick improvement, just displaying the versions somewhere in the configurator, would also already help. Besides that, I just stumbled over another case regarding this bug, where it is not working correctly. As I mentioned in my comment before, as soon as you have a token reference as the color value the alpha is applied to, it works in the configurator. I also wrote, that it does not work for me in my project. The reason for this is, that the token definitions are spread across multiple JSON files. This then is also a reproducible bug in the configurator again. If you have test cases, then these would probably be interesting cases to automatically test against, what do you think? |
@jorenbroekema @DarioSoller I think there is a bug in modifier. I did short investigation: Here is the link to the configurator Let's take a closer look at the code: {
"semantic": {
"alpha": {
"default": {
"value": "0.3",
"type": "opacity"
},
"disabled": {
"value": "{semantic.alpha.default}",
"type": "opacity"
}
},
"color": {
"surface": {
"default": {
"disabled": {
"value": "{core.color.blue.100}",
"type": "color",
"$extensions": {
"studio.tokens": {
"modify": {
"type": "alpha",
"value": "{semantic.alpha.disabled}",
"space": "srgb",
"format": "hex"
}
}
}
}
}
}
}
}
} As you can see from the code, disabled uses the reference to the default If we use "value": "{semantic.alpha.default}" in modifier, then it works: |
@DarioSoller I actually identified this bug yesterday as well, see #217. It is a separate bug though as @yanguly mentions, the references not working on color modifiers is its own bug. Looking into it -> #219 , got a failing test to confirm the bug |
Update, I had to dive pretty deep into style-dictionary to figure out what's going on with transitive transforms and the resolving of references. https://github.com/amzn/style-dictionary/blob/v4/lib/exportPlatform.js#L59 the "offending" code is somewhere in this while loop. Some observations:
So, this is quite a rabbit hole for me to fix in style-dictionary in its current architecture. I might try... but I have also raised an issue to discuss the style-dictionary architecture with regards to references and transforms on a more broad level. In my opinion this use case / issue is more easily tackled in the architecture changes that I propose here amzn/style-dictionary#1032 I'll update this issue if I manage to find an acceptable workaround fix in Style-Dictionary's current structure, but I think I'll then have to look at this again with a fresher mind. |
@jorenbroekema thanks for going down the rabbit hole and your detailed updates on it! Another use case I ran into, has been, that you might want to introduce extra tokens, if a certain condition is met, something like i.e. when a token has a specific value. That's also something which is really hard to do or probably not even possible with Style-Dictionary's current architecture!? As this "modifier value references" issue seems not to be solved easily and quick, I will most likely add another step to my preprocessing, which resolves the references under If there is anything I can help with, do not hesitate to ping me! PS: Thanks @yanguly for your example and contribution to this discussion! |
@DarioSoller since you're doing a preprocessing step outside the scope of Style-Dictionary to work around the current issue of "references inside sibling properties to the value property are not being resolved", it might be interesting for you to test Preprocessors, which is a new lifecycle hook that you can use to do such processing after token files have been parsed and merged, but before any transforming/resolving happens: https://github.com/amzn/style-dictionary/blob/v4/docs/preprocessors.md (as of style-dictionary@4.0.0-prerelease.2) |
@jorenbroekema Happy to try it out this week. I'll try to provide you feedback on this feature as soon as I can, for sure! I am already really curious on the possibilities with the custom preprocessors ;) |
Created a draft PR where I fix this problem amzn/style-dictionary#1069 by allowing a transformer to return undefined, which would defer the transformation until after references are resolved. This is very similar to when the value prop contains references, except now it also would work if metadata that the transformer relies on contains references. Needs a bit more discussion/tests but there's progress on this issue at least :) |
What happened?
Using token references for example for the alpha modifier gets not resolved, resulting in the alpha value just being missing without any warning/error. This might be related more to issue #198, rather than #164.
Consider following token data:
Reproduction
TokenStudio Configurator Reproduction
Expected output
Instead of
I would exect
Version
0.11.6
The text was updated successfully, but these errors were encountered: