-
Notifications
You must be signed in to change notification settings - Fork 566
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
include
the same token does not do a "deep merge"?
#1425
Comments
👋 In my case, I have a Is that something that seems achievable with the current state of SD library ? |
@hadrien-xbto -- In the current state of the logging configuration, probably not... I think there's been some discussion about making logging configuration more granular, which could help your scenario a little bit maybe. For example, eventually, maybe you'd be able to silence Collision warnings, but not Broken Reference warnings... kind of like "per-rule" configuration in ESlint? However, I think it'd end up being overly-complex to enable "ignore THIS collision warning, but not this OTHER collision warning", so, not sure the SD team would put that much time into reworking the logging architecture. The best I've been able to do in current state is configure logging on a per-Platform basis... so, if there is a particular scenario that I wanted to ignore Collision Warnings, I'd just put that scenario in its own "platform" and turn off the logging for that platform... but currently, it's still an all-or-none scenario... you cannot distinguish one collision from another collision. |
@chris-dura Have you explored custom parsers ? I was wondering if it could be used to import the JSON file and add namespaces for each imported file, then manage the tokens on my own afterwards with a combination of transforms and formats. But although I managed to have my own parser, as soon as I add a namespace, it breaks all the references like that :
|
Alright so there's quite a couple of things here, and perhaps there should be a deep-dive section in the docs explaining this in more detail.
As it turns out, both 😅 . The bug is that the "include" vs "source" do not currently behave the exact same way, also when combining the two. This is because sets within include OR source are merged through the "combineJSON" util which uses the deepExtend util. However, when combining the include & source objects together, the deepExtend util is used directly with a slightly different configuration, these need to be aligned. So I'm labelling this issue as a bug for this. The misunderstanding lies in how token level collisions are handled, your "actual" output is "correct". As for the "original" property, this is constructed to keep track of the token's original state after being parsed and preprocessed. It's there as a reference to a token's pre-transformed state, but merging the separate token files into a single dictionary is considered its original/initial state. We don't keep track currently of whether a token has collided after the fact, in the parsing lifecycle.
This should be a single collision warning, since it's only 1 token. It's throwing a warning for every property of the token 😓 this is definitely a bug of which I'm not really sure there's an open issue yet. Should be relatively easy to fix though.
There's isn't something at the moment to hook into the collision behavior, at least not in the public API of Style Dictionary. Internally we have a collision callback that we call to throw those collision warnings you see in the console, and there's an overrideKeys option we use internally to ensure that token "value" properties don't get merged together, e.g. if Token A and Token B are both typography values (object type), you don't want deep-merging behavior of the typography object values, you just want Token B to take precedence fully. There's an open issue discussing how tokensets can be layered/composed in a way which lets you silence collision warnigns if the collisions are intended. E.g. a collision in 1 layer with a token in another would be "intended", but if such a collision happens within the same layer, it would be a warning. #1170 I would love to get more feedback on this one.
https://styledictionary.com/reference/logging/ It's pretty granular now, warnings can be turned off, unfortunately not so granular yet that you can pick and choose which warnings exactly are turned off (or warn, or throw). That should be easy to add though, feel free to raise a feature request. We started going in that direction already by adding granular control for the broken references errors to be thrown non-fatally, and we can follow it for warnings too, and perhaps also some of the other errors that don't have to be fatal (because we can think of fallback behavior that makes sense).
Yeah, on a per-collision basis is probably overly granular and complex, but I can imagine with correct layering as proposed here #1170 that this would be a way to achieve that, marking certain sets relations as being "okay to collide" but same-layer sets "not okay to collide". |
@jorenbroekema -- thanks for your responses!
When you align these, will there be a mechanism to choose between "merge" (keep metadata) vs "clobber" (default, overwrite everything)? You mention...
Naively, I'm inferring that if a collision is "intended", that it might be preferred that collision does a "merge" instead of a "clobber"? |
Unfortunately, I think I completely missed this point, and thought I could get away with adding everything to
@jorenbroekema -- so, just to confirm, does this mean it is expected that if I want to write a /* config.json */
{
"source": [
"base.tokens.json",
"override1.tokens.json",
"override2.tokens.json",
"dark.tokens.json"
],
} /* base.tokens.json */
{
"primary": {
"$value": "#ff0000",
"$description": "The primary color."
}
}
/* override1.tokens.json */
{
"primary": {
"$value": "#00ff00",
"$description": "The primary color." /* WET */
}
}
/* override2.tokens.json */
{
"primary": {
"$value": "#0000ff",
"$description": "The primary color." /* WET */
}
}
/* dark.tokens.json */
{
"primary": {
"$value": "#ffffff",
"$description": "The primary color." /* WET */
}
} |
Yes, correct, from a design philosophy perspective, tokens are generally "isolated" (although can reference other tokens), so even if a token is targeting to override another, that coupling is not something the tokens should rely on individually.
No, it will be "clobber" always for the token level, and "merge" for token groups. I'd have to see a strong use case in order to change my mind and make this configurable. |
@jorenbroekema Here is our usecase :
For now, I have found a workaround (that creates its own set of problems though) : when loading the JSON file, I namespace the contents of .desktop and .mobile files, then do the merging part manually to be able to export values like Is it a weird usecase and we could architecture things differently, or it seems legitimate to you ? |
From Docs:
I do not know if what I'm seeing is a bug, or a misunderstanding of what "deep merge" means to StyleDictionary...
I assumed "deep merge" meant that metadata fields, like
$extensions
or$description
, that were NOT overridden would still be in the output... IOW, "inherited" by the files later in the precedence order.However, this does not seem to be the case if the same token, stored in two separate files, is listed in the
include
config...However, if I move the
overrides.tokens.json
tosource
, instead ofinclude
, the output is what I would expect.This seems perhaps to be the intended usage of
source
; however, the problem arises when you have multiple overrides for the same token insource
, for example when trying to generate "derivative" themes, or dark modes, because a glut of "red herring" Collision warnings are generated.Before I go silencing ALL collision warnings (some of which might actually be wanted☹️ ), I wanted to make sure that the above is expected behavior, and in order to inherit metadata, then ALL my "override" files need to be in the
source
config?getReferences()
level? #1287The text was updated successfully, but these errors were encountered: