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
Rename 'include' to 'references' and filter out by default #1170
Comments
I think we should also align this with the DTCG resolver proposal by @SorsOps to align terminology. I agree it can be more clear. More thoughts following soon! |
A bit of history on The one difficulty with calling the 'base' set of tokens 'references' is they might not all be references. All that being said, I am 100% open to changing the name of anything if it is more understandable and v4 is the time to do it. I'm not married to Just my initial thoughts, I'll keep noodling on it |
One option would be to keep I guess the problem with this is that the name There's one problem to solve with this approach that I see: what overrides what for three properties, I guess the order is Anyway both approaches also have one other problem to solve: if a glob pattern has a file in both |
I think there is a fundamental issue with creating properties ('include') that do the same thing as other properties ('source') where the only difference is the layering. What if I need a third layer, then I need to come up with another property name, etc. There's already a way to do extending/layering by using One suggested API that we could use is the following extension on "source": {
"source": [
"foo.json",
["base-1.json", "base-2.json"],
["extend-1.json", "extend-2.json"],
"bar.json"
]
}
This extends the source API without a breaking change, the old API still works as is, and it allows any number of layers. That said, you can also just ignore the collision warnings, they're a LOT less verbose by default in v4, and you can turn off warnings altogether as well. In addition, I changed the warning message a bit so it's more clear that you can ignore the warning if the collision was intended. So I am wondering how important the use case still is that @dbanksdesign described and whether this API redesign is worth it. My suggested API shouldn't be too much work I think, so I'm a bit on the edge about this. Finally, the proposal of "references" tackles the other use case which is creating a split between tokens you intend on outputting and tokens merely used as a reference layer, but I wanted to clarify that I think that the global property |
Personally I see collision warnings important in telling me there probably is a mistake somewhere since output may be completely unexpected. In fact we have our CI setup in a way that the build does not pass if there are collisions detected. It wouldn't be great at least to have to deduce somehow if some collision warning is important or not, so I support the API redesign in that way. Would collisions between |
Files originating from globs that are inside the same nested array (layer) will throw collision warnings if there are any. |
"source"
versus"include"
property has been the source of a lot of confusion for users, including myself as well.Include acts as a base layer of tokens that is generally only used as references for other tokens, and it does 2 things:
isSource
property with valuetrue
is added for any tokens that originate from files in the "source" globs arrayThe main problems here is that "include" is poorly named and that the majority use case for such tokens is to not include them in the output, which means it requires from users to always add a filter that filters tokens by isSource.
It is somewhat tedious to repeat this filter for every platform and each file when this is the majority use case for design systems, as usually the guidelines of a design system do not permit usage of primitives tokens, only semantics and components tokens.
Suggestion (both are breaking changes):
include
toreferences
, so it's clear that you should use this for token sets that are merely used as reference/alias for other tokens.outputReferenceTokens
(new property suggestion, we don't have this yet) is set to true, OR ifoutputReferences
on the format level options is set to true, because in this case we cannot safely filter out refs if we plan on outputting for example CSS custom properties for tokens referencing other tokens.Impact:
true
, since tokens from references array will be filtered out by default nowThe text was updated successfully, but these errors were encountered: