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

Implicit vs. explicit typing in composite tokens #199

Open
ilikescience opened this issue Dec 12, 2022 · 6 comments
Open

Implicit vs. explicit typing in composite tokens #199

ilikescience opened this issue Dec 12, 2022 · 6 comments

Comments

@ilikescience
Copy link

ilikescience commented Dec 12, 2022

The current format defines composite tokens in this way:

{
  "shadow-token": {
    "$type": "shadow",
    "$value": {
      "color": "#00000088",
      "offsetX": "0.5rem",
      "offsetY": "0.5rem",
      "blur": "1.5rem",
      "spread": "0rem"
    }
  }
}

Unlike in a non-composite token, the values aren't explicitly typed. Take, for instance, in the composite token you have this:

...
    "offsetX": "0.5rem",
...

In another token, you'd write that as:

{
    "horizontal offset": {
        "$type": "dimension",
        "$value": "0.5rem"
    }
}

Currently, the spec is written in such a way that the shadow composite token ALWAYS has a offsetX key, and that the offsetX key ALWAYS has a value of type dimension, so a parser should be able to correctly "guess" the type even though it's not explicitly stated.

However, I see a future where this fact gets hard to maintain. Take the typography composite token, for example.

In issue #102, folks have said that the current list of properties is too restrictive. At the same time, I pointed out that in CSS alone there are 57 possible properties that could apply to typography, and proposed a model of a required base set + optional extended set of properties.

In order for implicit typing to work, the has to list out a single unambiguous type for every property that the composite token might have. There's also the possibility that a property might have two allowed types, which causes some additional complexity.

The other option is to explicitly type values of composite tokens:

{
  "shadow-token": {
    "$type": "shadow",
    "$value": {
      "color": {
        "$type": "color",
        "$value": "#00000088"
      },
      "offsetX": {
        "$type": "dimension",
        "$value": "0.5rem"
      },
      "offsetY": {
        "$type": "dimension",
        "$value": "0.5rem"
      },
      "blur": {
        "$type": "dimension",
        "$value": "1.5rem"
      },
      "spread": {
        "$type": "dimension",
        "$value": "0rem"
      }
    }
  }
}

Obviously this is much harder to type. But the benefit is that the spec doesn't have to maintain the list of type mappings, the parser doesn't have to implement that map, and that composite tokens can have "open" definitions which have a small set of required properties but allow for any additional properties on top of that.

This also blurs the lines between a composite type and a group, which is a much bigger topic.

@romainmenke
Copy link
Contributor

This is very close to an alternative I proposed here : #126 👍

@CITguy
Copy link

CITguy commented Jan 7, 2023

I could see this syntax being helpful for custom composite tokens. However, it doesn't provide any additional value for composite types that are already part of the spec.

Spec-defined, standard composite token types (e.g., typography, shadow, etc.) should have clearly-defined types for each of their properties. Otherwise, what's the point of defining any composite type in the spec?

@romainmenke
Copy link
Contributor

It is useful even for standard composite tokens :)
See : #201 (comment)

What if you want to describe line height in typography as "10px" and as 1.2.
Currently those are two different types, a dimension and a number.

The specification doesn't explain in any way or form how to disambiguate a composite token where a field can have one of multiple possible types. This is very limiting.

@CITguy
Copy link

CITguy commented Jan 7, 2023

What if you want to describe line height in typography as "10px" and as 1.2. Currently those are two different types, a dimension and a number.

In this case, I'd say that the issue is not with the syntax itself, but with the incomplete definition of the typography type. If composite token props are meant to have values that correspond to a non-composite type (e.g., number, color, dimension, etc.), then we need to make sure that non-composite types are defined in order to support their use within composite tokens.

The specification doesn't explain in any way or form how to disambiguate a composite token where a field can have one of multiple possible types. This is very limiting.

Technically, we already have this ambiguous definition with the "fontFamily" and "fontWeight" properties. A fontFamily token may be either a string or an array of strings. A fontWeight token may be either an enumerated string value (e.g., "thin", "regular", "bold", etc.) or a number (e.g., 100, 400, 700). However, if you really want to get technical, ANY token value (or composite property value) can also be an alias string, so even if a type is defined as a number, tooling technically needs to support a number OR a string (alias).

In the case of the typography "lineHeight" property, I'd propose defining a new "lineHeight" type such that it may be either a number (i.e., a multiplier) or an explicit dimension. Then, typography "lineHeight" would require its value to be a lineHeight token, instead of the ambiguous type that's currently documented.

alias<T> = string; // e.g., "{foo.bar.fizz.buzz}"
dimension = string | alias<dimension>; // format: "N(px|rem)"
fontWeight = number | string | alias<fontWeight>;
fontFamily = string | string[] | alias<fontFamily>;
lineHeight = number | dimension | alias<lineHeight>; // NEW!
typography = {
  "fontFamily" fontFamily;
  "fontSize" dimension;
  "fontWeight" fontWeight;
  "letterSpacing" dimension;
  "lineHeight" lineHeight;
} | alias<typography>;

@romainmenke
Copy link
Contributor

romainmenke commented Jan 7, 2023

Correct that there is no issue with a field supporting multiple types in principle.

But the specification doesn't explain or define how to parse this.
This is a critical and fundamental issue.
It can not be waved away :)

If the specification wants to support mixed-type fields it must add a section on how to analyse a value and determine which type it really it is.


Technically, we already have this ambiguous definition with the "fontFamily" and "fontWeight" properties

That is a bug, not a reason to add more ambiguity :)


To get a better understanding of the level of complexity it is worth reading the tokenizing steps from CSS : https://www.w3.org/TR/css-syntax-3/#consume-token

For Design Tokens this could be less elaborate but the specification should still define something similar.

How do I analyse a byte sequence and extract it's type and value?

@ilikescience
Copy link
Author

Returning to this after a bit of experience working with composite types, I think that we should have:

  1. Implicit types for properties specifically covered by the spec. eg, the spec may define the shadow type to be composed of a specific set of properties. This makes authoring more pleasant at the expense of some parser logic; however, it is unambiguous.
  2. Explicit types for anything not covered by the spec. eg, composites token might be "openly defined," allowing an author to add any number of relevant properties to the token beyond a certain required set. for this to work, the author should provide explicit types so that the token translator can correctly translate the token.

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

5 participants
@CITguy @kevinmpowell @ilikescience @romainmenke and others