Skip to content
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

$type should be a required property #139

Closed
reblim opened this issue Jun 19, 2022 · 25 comments
Closed

$type should be a required property #139

reblim opened this issue Jun 19, 2022 · 25 comments
Assignees

Comments

@reblim
Copy link

reblim commented Jun 19, 2022

3.2 (Design) Token Properties

A concern I have with the current specification is that we are not making the "$type" property required. I believe that both the "$type" and "$value" properties should always be required to avoid tools from having to guess a type based on the group type, or even higher scopes.

Nevertheless, in order for this to work well, we must define our core types more precisely and create a complete list of all possible "primitive" token types. In addition, we could have a "default" type that acts like "any" in TypeScript, to be used if a defined type does not provide enough context for a token.

@reblim reblim changed the title Type and Value shpould be required token properties Type should be a required property Jun 19, 2022
@reblim reblim changed the title Type should be a required property $type should be a required property Jun 19, 2022
@romainmenke
Copy link
Contributor

A concern I have with the current specification is that we are not making the "$type" property required. I believe that both the "$type" and "$value" properties should always be required to avoid tools from having to guess a type based on the group type, or even higher scopes.

I fully agree with this.
Even for string and number the $type field should be set in my opinion.
see : #120

Composite types should be typed groups.
see : #126


Nevertheless, in order for this to work well, we must define our core types more precisely and create a complete list of all possible "primitive" token types. In addition, we could have a "default" type that acts like "any" in TypeScript, to be used if a defined type does not provide enough context for a token.

This I do not see the purpose of, but I might be missing the point :)

Tools would be unable to use this token as they do not know its type.

So a default or any type would create "write only" tokens.
You could store them, but you would be unable to read them.

@reblim
Copy link
Author

reblim commented Jun 19, 2022

This I do not see the purpose of, but I might be missing the point :)

I agree with you. I'm always thinking as "when things go wrong". But in this case, ony if "type" becomes required, maybe it makes no sense to have also a defult type.

@TravisSpomer
Copy link

I agree with this; I don't see the point to having $type be optional. It seems like it saves a couple of seconds of typing if you're manually creating a token file at the expense of making it harder for future humans and code to deal with.

@c1rrus
Copy link
Member

c1rrus commented Jun 26, 2022

Perhaps the spec can be worded more clearly but: Every token has an unambiguous type and tools are expressly forbidden from trying to guess the token's type (e.g. from the value or name).

However, the $type property on a token is only one of several ways of specifying what the type is. The algorithm for determining a token's type is described in the Design Token - Type chapter.

As it stands, the order of precedence is (from highest to lowest):

  1. The $type property on the token object, if present. Otherwise...
  2. If the token is a reference, the type of the token it is referencing. Otherwise...
  3. The inherited type from the nearest parent group that has a $type property. Otherwise...
  4. Whichever of the basic JSON types the token's value is

If we mandated that every token object must have $type property, then that would change things quite substantially. For instance, we'd need to remove the ability to have a $type property on a group since it would become completely useless.

Personally, I prefer to retain the ability to set a default type at the group level (yes, I'm a lazy typist! 😛) but that only makes sense if the $type property on tokens remains optional.

Btw, the last fallback case (4.) is a bit like the "default" type idea proposed in @reblim's OP. Admittedly, most of those basic types are unlikely to be useful in the context of a design system (though number could be useful for things like unitless line heights). However, I do believe they are worth keeping in the spec for the following reasons:

  • I think having some kind of fallback or "default" type is useful to guarantee that every token always has a type - even when the author forgot to set one via the $type property. However, just picking one of the spec's "normal" types (color dimension, fontFamily, etc.) feels arbitrary. Saying a string is a string, a number is a number etc. might not be useful but at least it's not wrong.
  • For teams and tools that have exotic requirements which are not directly covered by the spec, these fallback types in conjunction with $extensions could provide a neat way to define unofficial types. E.g. Let's say you want to create "roughness" tokens for 3D graphics which define how glossy or matt a texture is. Perhaps you'd represent the value of such tokens as a number, but since there's no "roughness" type in the spec, you could define an extension to differentiate your roughness tokens from ones that are "just" numbers1 (in tools that understand that extension - to any other tool it would still just be a number). If we did not have these fallback types, then you'd necessarily need to pick one of the spec's actual types and misuse use it for your intended purpose. To me that feels more hacky.
  • The 1st editor's draft had a concept of user-defined composite types. While this was removed in the 2nd draft in favour of a set of pre-defined composite types, I for one would like to revisit this idea in future versions of the spec. It could become an "official" way to define your own token types (as opposed to the unofficial route I outlined in the previous point) and my hunch is that having those basic JSON types available could be useful for that. I can imagine we might one day provide a way to extend or compose them into whatever you need.

Note that I think it's perfectly acceptable for tools to ignore tokens that have a type that's not relevant to them. For example, imagine I wrote a tokens file that contained a color token and a boolean token. If I loaded that file into a design tool like Figma, I'd probably expect it to make the color token available to me wherever I can set the color for something (essentially like the "color styles" Figma has today). However, the boolean token probably has no use in the context of that tool, so I think it would be fine for Figma to just ignore it.

In my view, what every tool MUST be capable of is to parse the entire file and implement that type resolution algorithm in order to determine what kinds of tokens there are. Whether it subsequently does stuff with all or just a sub-set of the tokens is entirely up to the tool though.

Footnotes

  1. Example of how a hypothetical "roughness" token could look (please excuse the crappy formatting, but it seems I can't do code blocks in footnotes): { "shiny": { "$value": 0.8, "$extensions": { "com.example.custom-type": "roughness" } } }.

@WanaByte
Copy link

In the case of parsing or linting, I think it is much simpler for each token to carry a $type.

Using $type for every token also adds security that an alias matches the expected type for the token.

For example, I would want an error if I tried to do this:

{ 
  "black": {
    "$value": "#000000",
    "$type": "color"
  },
  "height": {
    "$value": "{black}",
    "$type": "dimension".   <----- I want this to fail. It would succeed if "$type" can be omitted.
  }
}

With "$type" present, I can get an error when I author this mistake. Without "$type", this error may not surface until I try to build CSS from the tokens.

I think we're also seeing a trend in developer tooling toward stronger type safety (e.g. Python type annotations, Typescript, etc). For all those reasons, I would like to see "$type" be required.

@romainmenke
Copy link
Contributor

Using $type for every token also adds security that an alias matches the expected type for the token.

Hehe, this is the one exception where I think $type can not be set.

A background color and a background image will have different types as tokens but might be able to be assigned to the same property by consumers.

If tokens with references as values have a type, it will no longer be possible to change the type at all for the source token. Each attempt will just be an error.


It might be interesting to have a specialised diff and validation tool for token consumers. You could feed this 2 token files and it could tell you if any types have changed.

@c1rrus
Copy link
Member

c1rrus commented Jul 13, 2022

@KyleWpppd Perhaps the spec's wording doesn't make this clear, but what you've described is how things should work.

As per my earlier comment, a $type property on a token has the highest precedence when resolving that token's type.

Therefore, if that token happens to reference another token which has a different type, then that is an error since the value will be incompatible.

By having $type optional, we afford authors to express their intent in a more nuanced way. For example:

{
  "token-a": {
     "$type": "dimension",
    "$value": "1rem"
  },

  // token-b's resolved type is "dimension"
  // If the author's intent is: token-b is just another name for token-a, regardless of what
  // token-a's type is, then they can express that by not setting $type and thus having the
  // the resolved type be determined by the other token they are referencing.
  "token-b": {
    "$value": "{token-a}"
  },    

  // This token's resolved type is color. However, it is invalid
  // as its value references a non-color token. Tools are therefore
  // required to reject this token and should show an appropriate
  // warning or error message to the user.
  // If the author's intent is: token-c must be a color, so I want tools to warn
  // me if I accidentally reference a non-color token, then this is the way to
  // achieve that.
  "token-c": {
    "$type": "color",
    "$value": "{token-a}"
  }   
}

@romainmenke
Copy link
Contributor

Therefore, if that token happens to reference another token which has a different type, then that is an error since the value will be incompatible.

If the author's intent is: token-c must be a color, so I want tools to warn
me if I accidentally reference a non-color token, then this is the way to
achieve that.

This goes both ways right?
You will not be able to reference a non-color token and you will also not be able to update the referenced token to something that isn't a color.

If you do need to change the type you would first need to remove the type constraint everywhere the token is referenced or remove all references and add them back later.

correct?

@c1rrus
Copy link
Member

c1rrus commented Jul 15, 2022

@romainmenke Yes, I think you're right. I had so far mainly been focussing on what the expectations are for tools when they read or write tokens files, not so much what they do with the parsed data inbetween. My assumption is that different tools will have different internal representations and usages of the token data.

That being said, tools must only write out valid token files. So, one way or another, they'll need to do something along the lines of what you described in order to meet that requirement.

Perhaps the requirements could be specified something like this:

  • When a token's type is changed, tools must find all other tokens that reference the changed token and...
    • If those tokens have their own $type attribute (which no longer matches the changed token's new type), either:
      • EITHER, Remove it (so that the token's type is now determined by the changed token it references). In this case, the process will need to be repeated for any tokens that reference that token.
      • OR, "detach the reference" by replacing the token's $value with the previous value of the changed token.

In both cases, one can argue that the intent of the referencing tokens is being altered in some way, so tools should warn the user or, better yet, give them a choice of what to do.

@romainmenke
Copy link
Contributor

But I do agree that $type is useful on alias tokens and that I think tools can provide flows to manage the different state and data changes.

@WanaByte
Copy link

Over the weekend I did some thinking. One place I got stuck is how to process $value in Java when there is no $type to decide how to parse a value?

@TravisSpomer
Copy link

Why do you see it being different in Java? Per c1rrus's comments above, every token already has an unambiguous type in the current format. $type on a token is only optional when the type can be deterministically calculated from $type elsewhere in the file.

@WanaByte
Copy link

@TravisSpomer apologies I did not understand that part of the spec. I was worried about parsing the $value where $type was ambiguous.

@TravisSpomer
Copy link

No need to apologize. 🙂 And I think I misunderstood things a bit myself: it looks like it's in fact always optional, but if you don't specify $type anywhere you get a primitive string or number token rather than an error. It's still unambiguous so it shouldn't be a problem from any language, but it also feels like rather weird behavior to me.

@kevinmpowell
Copy link
Contributor

Related to #149

@laat
Copy link
Contributor

laat commented Aug 30, 2022

As it stands, the order of precedence is (from highest to lowest):

  1. The $type property on the token object, if present. Otherwise...
  2. If the token is a reference, the type of the token it is referencing. Otherwise...
  3. The inherited type from the nearest parent group that has a $type property. Otherwise...
  4. Whichever of the basic JSON types the token's value is

This algorithm makes it much harder to describe the shape of the documents with TypeScript, JSON Schema or other tools for describing JSON. With a simpler model, for example making "$type" required with every "$value", we can create simple schemas for the document. With a simple JSON Schema we can get autocomplete and linting in Visual Studio Code almost for free.

@kaelig kaelig added Needs Feedback/Review Open for feedback from the community, and may require a review from editors. dtcg-format All issues related to the format specification. labels Sep 7, 2022
@kevinmpowell kevinmpowell added this to the Next Draft Priority milestone Oct 3, 2022
@kevinmpowell kevinmpowell removed Needs Feedback/Review Open for feedback from the community, and may require a review from editors. dtcg-format All issues related to the format specification. labels Oct 3, 2022
@nclsndr
Copy link

nclsndr commented Oct 28, 2022

@KyleWpppd & @romainmenke, I used to think that manipulating the tree would get difficult without a mandatory $type.
But we need to recall the format aims for data transportation and "easy" human editing.

To put my head around this issue I'm building a parser that reveal how $type is not mandatory for data further data transformation nor validation by traversing the tree.
Source: https://github.com/nclsndr/design-tokens-format-module/blob/main/src/parseDesignTokens.ts#L130

@romainmenke
Copy link
Contributor

But we need to recall the format aims for data transportation and "easy" human editing.

That is not true currently :)
The spec only mentions that using JSON has the side effect of being somewhat human-readable. But it doesn't even state that this is a goal or a requirement.

It however has been mentioned many times in comments.
Which is why I asked for clarifications on this conflict between comments and the spec here : #149

Even when a parser for the current spec can be implemented, doesn't mean that it will age gracefully.

My concern is the long term.
Starting out with a specification that has many implicit and vague parts means that it will be much harder to expand later. Starting with a simpler and explicit format is easier.

I would like to see a transfer format that can easily be used in older and newer versions of tools and where older and newer files can easily be opened in all tools.

I also want new additions and expansions to be simple and I never want them to be breaking changes for any and all users.

These properties reduce friction between spec editors, tools and users, in turn these drive adoption.

@nclsndr
Copy link

nclsndr commented Oct 31, 2022

The spec only mentions that using JSON has the side effect of being somewhat human-readable. But it doesn't even state that this is a goal or a requirement.
You're right @romainmenke - I did a shortcut here.

I do share your claims around making the format future proof being careful on architectural choices.

Yet, I do see the $type resolution as a quite simple macro that could not drive regressions moving forward, at least not with its current definition.

IMO, what is actually at stake here is performance. Any loose schema with implicit resolutions like $type will cost a couple of extra spins to make sure of its integrity / validity. But in the case of DTCG, aliasing is already way more greedy compared to $type resolution and seems to be a valuable feature for the format.

These are definitely design decisions that will impact any future tools, UX and performance.

@romainmenke
Copy link
Contributor

romainmenke commented Oct 31, 2022

{
  "$value": "foo"
}

This token has the JSON type string and no $type property.
What is this?

  1. a keyword foo
  2. a string literal "foo"
  3. neither, it is undefined and should be ignored by all tools

1 is problematic because there is no context and this can be ambiguous.
Multiple types can share the same keywords (auto, initial, center, ...)
This makes it impossible to derive the true type from the value.

2 is the only option that makes sense. But then why not "$type": "string"?

3 If it is undefined and should be ignored, then why even include it in the specification?

It needs to be defined now what the JSON type string is and what it will be used for.
Otherwise tools will make a guess. Some might use it for keywords, others might ignore it.
When it needs to be defined, why not make it explicit with "$type": "string"?

"$type": "string" separates the "carrier" format (which needs to be a string for many types) from the token value type.

{
  "$value": "foo",
  "$type": "string"
}

vs.

{
  "$value": "foo",
  "$type": "keyword"
}

It is highly unusual for specification to have these large gaps in their definitions. Especially for fundamental things like basic value types.

@romainmenke
Copy link
Contributor

romainmenke commented Oct 31, 2022

Another good reason to make it required is because it doesn't make sense to omit $type during serialisation.

After first going through the algorithm to assign types to every token it doesn't make sense to remove them again when writing out back to json.

This is just more consistent

@c1rrus
Copy link
Member

c1rrus commented Nov 29, 2022

Thanks all for your comments and feedback. The spec editors reviewed this and have decided to update the spec to make $type mandatory on either the token or one of its parent groups. To put it another way, when all else fails while resolving a token's type instead of falling back on one of the JSON types, it should now be considered an error:

The resolved type of a design token is:

  1. The $type property on the token object, if present. Otherwise...
  2. If the token is a reference, the resolved type of the token it is referencing. Otherwise...
  3. The inherited type from the nearest parent group that has a $type property. Otherwise...
  4. Whichever of the basic JSON types the token's value is the token is invalid and should be ignored

The intent is to preserve the (human) authoring convenience that inheriting $type from parent groups provides, while guarding against accidental omission of any kind of type info. In those situations, rather than assuming the author intended the token to be one of the basic JSON types, we're now assuming that was an accident.

I'll leave this issue open until we've created and merged a PR that updates the spec to reflect this change.

@reblim
Copy link
Author

reblim commented Dec 20, 2022

@c1rrus That's great news! Do you have a link to the PR? Thank you!

Also, what about the $value property?

@c1rrus
Copy link
Member

c1rrus commented Jan 3, 2023

I've just opened a PR to make the required changes: #201 :-)

It'll need to be approved by other editors before it can be merged. But you're all very welcome to take a look and add review comments. Thanks!

@c1rrus
Copy link
Member

c1rrus commented May 17, 2023

Just realised this issue never got closed even though PR #201 has been merged for a while now. Therefore go ahead and closing this.

@c1rrus c1rrus closed this as completed May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants