-
Notifications
You must be signed in to change notification settings - Fork 62
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
Alias $type inheritance? #236
Comments
I think the It also helps explicitly define the type of the token, since you might want to mix tokens to achieve another (like combining opacity number and RGB color to make a RGBA new color). Or for example, if you multiply I do think there you have a great idea of not including it every time though. It seems verbose and makes it difficult to manually parse JSON without a tool to strip the noise. Not sure how to practically achieve that without an explicit structure or some other system to accommodate for the inference of types. Also this issue emphasizes the need for a section of the docs on parsing, and expectations the spec has - to create some consistency between tooling. |
Well that’s just the thing though—parsers actually have to validate aliases currently. Aliases have to be non-circular. And non-aliases have to be valid tokens. By just accepting an invalid schema at face-value, the user will get errors with no clear action on how to fix it. So parsers actually do have to scan the alias and know it’s referring to a color anyway, and there’s no way for them to avoid having that work (or they’d be bad, hard-to-use-tools). But I was more interested in exploring whether that’s an error to show to the user “fix this!” or if it just silently works
💯! |
The spec lists out the following resolution order:
So, in your examples {
"color": {
"a": { "$type": "color", "$value": "#336699" },
"b": { "$value": "{color.a}" }
} This is valid, and {
"color": {
"a": { "$type": "color", "$value": "#336699" },
"b": { "$type": "dimension", "$value": "{color.a}" }
} This is invalid; the spec says that a token with the type {
"base": {
"$type": "color",
"a": { "$value": "#336699" }
},
"semantic": { "$value": "{base.a}" }
} This is valid, and The spec is pretty unambiguous about these, though it might need a bit more language to acknowledged that a referenced token might inherit its type, too, either from its group or another reference.
Yes, almost any type on a token with a value like |
Oh I hadn’t even thought of this, but you’re right just evaluating a token’s
Agreed—I think clarifying this would help. The resolution definition you provided earlier implies this, but doesn’t outright explicate it, and it could be clearer. Thanks for clarifying! This seems like it’s not really ambiguous, then, how the spec is interpreted. More just a possible documentation TODO of “should alias types get a little more specific regarding this behavior or not”. |
We are experiencing 2 simular problems:
Let's first dive into the aliases, but the second seams related to so I added that in the end. The aliases would have a solution to include a type in xml I beleive as an attribute can hold the class. But json does not have attribute in its spec. In joson I would like to propose a way to include the type. {
"background": {
"gradient": {
"value": {
"path": "gradient.default.1",
"type": "color"
},
"type": "reference"
}
}
} This reads as: "at the path My second remark is about the different allowed types in the Design Tokens spec. Could someone refer me to another related issue?
To conclude might I suggest to preferably add more types to the spec but definatly use the json structure to include the reference type that has a |
I thought a lot about the issues you mention @doozMen, and I landed on the same conclusion: the json tree lacks data. BUT, we can still specify a mapping for allowed aliasing matches among tokens: // Example with Border token
const borderTokenTypeMapping = {
_unionOf: [
{ _tokenType: borderTokenTypeName },
{
_mapOf: {
color: colorTokenTypeMapping,
style: strokeStyleTokenTypeMapping,
width: dimensionTokenTypeMapping,
},
},
],
} satisfies TokenTypesMapping; Beside this off-initial-topic answer, shouldn't we close this issue? |
I’m happy to close the issue, yeah. There might be a TODO on just adding some additional clarification in the documentation, but no spec changes are necessary IMO. |
Thanks for taking my off-topic suggestion into account. Your suggestion to have a mapping slightly slows down the parsing and at least for me is less readable but if this is possible is there an issue I can track to when this mapping description lands in the spec? |
Was digging into alias behavior a bit, and I had a few question about alias
$type
s. What are peoples’ thoughts on whether any of the following are valid or invalid? Also I may have missed a previous conversation or part of the spec that applies to this.Omitting $type
Is this valid?
$type mismatch
Will
color.b
be a color, and be validated as such (sort of an extension of the previous one)?And an even bigger question: is declaring ANY
$type
on an alias invalid?$type inheritance
Will the inherited
$type
also apply to the alias? (and override any possible$type
the alias may have)?In all of these, I’d like to reduce error as much as possible, and I can’t think of a good reason for even declaring
$type
on aliases because there’s at best there’s duplication; at worst there’s the possibility of a conflict. And wondering when it’s helpful to err, when it’s helpful to warn, and when it’s helpful to do nothing (silently work)The text was updated successfully, but these errors were encountered: