Remove extra types in tokens ast

[dabbott]

I'm removing these since they aren't used anywhere in the compiler, and I got confused for a sec about what they were for

[mathieudutour]

no it's actually pretty useful to discriminate between tokens

[dabbott]

I agree that a type for specific kinds of tokens is useful and could be worth adding, but I find the level of abstraction of these ones misleading - they contain an irrelevant type field, e.g. ColorToken contains type: "color"

Maybe something like this could work, although the data has a different shape: https://github.com/target/compiler-of-android-for-lona/blob/d1855de1cc9921eda550df4a4b0f803a7ccd158d/src/tokens/color.ts#L5-L8

[mathieudutour]

I don't agree, and I'm using them in the doc generation IIRC.

For example you can have a function like this:

function handleColors(token: ColorToken) {
  const name = token.qualifedName.join('.')
  const value = token.value // you know that it's a color and don't have to check for it
}

But yes this format is super weird, I think I asked you about all this nesting the first time I saw it. Now that we don't use Reason, we could simplify it a lot:

{
  files: [
    {
      type: 'flatTokens'
      inputPath: string
      outputPath: string
      name: string
      content: Array<
        {
          qualifiedName: Array<string>
          type: 'color',
          value: ColorValue
        }
      | {
        qualifiedName: Array<string>
        type: 'textStyle',
        value: TextStyleValue
        }
      >
    } | 
    {
      type: 'doc'
      inputPath: string
      outputPath: string
      name: string
      contents:...
    }
  ],
  flatTokensSchemaVersion: string
}

eg. remove 3 level of nesting

[dabbott]

Can you point me to where you're using ColorToken in docs generation so I can take a look? I'm imagining at some point before the handleColors call there's a check for type === 'color' so that TS can downcast safely?

Sure, we can revisit the JSON file format. I think each level of nesting is there for a reason, so we'd need to talk about each one. E.g. tokens: (qualifiedName, value, type) vs (qualifiedName, (value, type)). Is it better to say a variable name is bound to a value and a type? Or is a variable name bound to a variable, which is a (value, type) pair? I personally prefer the nested version when just looking at it, but I don't feel strongly about it, and I think both are pretty be reasonable. Maybe it's easier to operate on the flat version.

---

Part of the problem might be I don't know TS too well. Is there a better way to do the following?

I want to represent tokens like this in JSON (leaving off names to highlight the issue I'm facing):

{ "type": "color", "value": { "css": "red" } }
{ "type": "shadow", "value": { "x": 0, "y": 1 } }

I don't really like this:

type ColorToken: { type: 'color'; value: { css: string; }; }
type ShadowToken: { type: 'shadow'; value: { x: number; y: number; }; }

Cause TS knows it's a shadow even without the type: 'shadow'. What I really want is to strip out type when deserializing:

type ColorToken: { css: string; }
type ShadowToken: { x: number; y: number; }

Ok, perfect. Now how do I represent an array of tokens?

type Token = ColorToken | ShadowToken
type TokenList = Token[]

Cool. But how do I know the type of the Token when operating on the list, e.g. filtering the list to just be Color tokens? Detecting the key css to determine if something is a color would be awful. Ok, so let's plop a type back in:

type Token = { type: 'color', value: ColorToken } | { type: 'shadow', value: ShadowToken }

Hey wait... that's the exact same shape I had a second ago lol. Only now Token and ColorToken are somehow at different nesting levels, so that naming is also confusing.

type ColorTokenValue =  { css: string; }
type ShadowTokenValue = { x: number; y: number; }
type Token = { type: 'color', value: ColorTokenValue } | { type: 'shadow', value: ShadowTokenValue }

And that's how I landed on what we have now. A Token has a type and a TokenValue, and a TokenValue is a union of a bunch of more specific types like ColorTokenValue. But what I really want is the TS compiler to just know without the explicit type field.

I took the names out of this example in order to not conflate the decision of whether to add a separate level of nesting or not (which I don't feel strongly about either way) with these type shenanigans.

[mathieudutour]

Can you point me to where you're using ColorToken in docs generation so I can take a look?

Can't find it anymore. Maybe I ended up not using it - but I did add it for a reason 😄 It was coming from https://github.com/airbnb/Lona/blame/master/compiler/core/types/index.d.ts a few months ago. Yeah you need to check that you have a color token before it to the function, but I find it a lot cleaner to pass only the tokens it needs to handle to functions that handle only one kind of token.

---

How about just

type ColorToken =  { type: 'color', css: string; }
type ShadowValue = { type: 'shadow', x: number; y: number; }
type Token = ColorToken | ShadowToken

and TS is pretty smart about it

switch (token.type) {
  case 'color': {
   // here token.css is correctly typed
  }
}

That's how every AST are implemented in JS

[dabbott]

This definitely seems compact and easy to work with, but I find it kind of confusing that there isn't a clear separation between the type/metadata of the data, and the data itself. How do I know that type isn't just some property of shadows, e.g. it could be a radial shadow or something? It would be nice if I could tell at a glance. E.g. it's ugly, but if we called it _type, that would indicate metadata to me

For token types specifically, we could also have a built-in function that does the filtering, cause I had to do the same filtering too: https://github.com/target/compiler-of-android-for-lona/blob/d1855de1cc9921eda550df4a4b0f803a7ccd158d/src/index.ts#L22-L26. Once I confirmed the type field, I discarded the extra nesting level though

[mathieudutour]

I think we need to consider type as a "reserved keyword", and be consistent about where we put it (always the first property, etc.). If you want to add another type the shadows, then you use shadowType. I don't think having _type and type would be a good idea anyway.

For token types specifically, we could also have a built-in function that does the filtering, cause I had to do the same filtering too

Yeah we could have a function that returns the tokens by type instead of by files (which is why I needed those type IIRC):

(tokens: ConvertedWorkspace) => ({ colors: ColorTokens[], shadows: ShadowTokens[], textStyles: TextStyleToken[] })