Introduce the Token Listing plugin

[Sidnioulz]

Changes

I added the token listing plugin :)

How to Review

• Run tests for the plugin
• Build your own token listing for your fav design system and provide feedback :)

What I expect to be wrong

I'm unhappy with how originalValue works. I think it's less useful in the listing than the aliasChain you use internally and I intend to review the RFC accordingly.

I have not updated the JSON schema and RFC in some time. I'll do that on Thursday.

[changeset-bot]

🦋 Changeset detected

Latest commit: 06a8a4b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[cloudflare-workers-and-pages]

Deploying terrazzo with    Cloudflare Pages

Latest commit:	06a8a4b
Status:	🚫  Build failed.

View logs

[drwpow]

This all looks great! I think I’d still like to see a couple tests, even if they’re very simple smoke tests. But once those are in, would be happy to merge & release.

After talking over it, too, since we ended up with a pretty backwards-compatible change (at least far as I can tell), I’d be happy to just make this a standard release! I only mentioned a “beta” in the off-chance we landed on something that was too experimental, etc. But I think this new direction looks pretty stable from an API perspective.

[drwpow]

Completely aside, one open question I’ve had is that logger.error() throws an error. But given that it’s configurable, TypeScript doesn’t treat code that follows as dead code. Perhaps the only way to fix it is just separate error() from errorWithoutThrow() or something, rather than the { continueOnError: boolean } option. AFAIK TypeScript does do a good job of “remembering” when a function throws

[Sidnioulz]

I think you can do that but you'd probably need to massage the input types a bit.

/** Log an error message (always; can’t be silenced) */
  error(entry: LogEntry & { continueOnError: true }): undefined;
  error(entry: LogEntry & { continueOnError: false | undefined }): never;

  error(entry: LogEntry) {
    this.errorCount++;
    const message = formatMessage(entry, 'error');
    if (entry.continueOnError) {
      // biome-ignore lint/suspicious/noConsole: this is a logger
      console.error(message);
      return;
    }
    if (entry.node) {
      throw new TokensJSONError(message);
    } else {
      throw new Error(message);
    }
  }

And then this shows dead code after returning:

const moreSpecificEntry: LogEntry & { continueOnError: false } = {
      ...entry,
      continueOnError: false,
    };
    this.error(moreSpecificEntry);

This would work more easily:

error(entry: LogEntry, continueOnError: true): undefined;
  error(entry: LogEntry, continueOnError: false): never;

  error(entry: LogEntry, continueOnError: false | true) {
    ...
  }

[Sidnioulz]

Throwing means reaching a non-satisfiable type for a type inference system, which TS supports through never. And it also supports signature overrides on functions, so the only difficulty is ensuring that param types can always properly be inferred to a single signature at check time rather than runtime. continueOnError must always be statically hardcoded and never be a variable.

[pvignau]

Nice Work ! Works like a charm.

I'd have some changes to suggest, not on the code but the feature.

We discussed about the absolute filepath on the source.resource key. It'd be really great to have a relative path or maybe a configuration key to force this attr.

Also, I have a question for mode handling, I don't know what to think about but let's take these examples :

{
      "$name": "color.bg.accent.strong.enabled",
      "$type": "color",
      "$value": {
        "colorSpace": "srgb",
        "components": [
          0,
          0.47843137254901963,
          0.592156862745098
        ],
        "alpha": 1,
        "hex": "#007a97"
      },
      "$extensions": {
        "app.terrazzo.listing": {
          "names": {
            "scss": "--color-bg-accent-strong-enabled"
          },
          "originalValue": "{core.color.blue.500}",
          "previewValue": "#007a97",
          "source": {
            "resource": "file:///Users/pyvignau/workspace/ftven/galaxie/packages/new-tokens/src/dtcg/tokens.json",
            "loc": {
              "start": {
                "line": 76,
                "column": 22,
                "offset": 2474
              },
              "end": {
                "line": 85,
                "column": 12,
                "offset": 2764
              }
            }
          }
        }
      }
    },
    {
      "$name": "color.bg.accent.strong.enabled",
      "$type": "color",
      "$value": {
        "colorSpace": "srgb",
        "components": [
          0,
          0.47843137254901963,
          0.592156862745098
        ],
        "alpha": 1,
        "hex": "#007a97"
      },
      "$extensions": {
        "app.terrazzo.listing": {
          "names": {
            "scss": "--color-bg-accent-strong-enabled"
          },
          "originalValue": "{core.color.blue.500}",
          "previewValue": "#007a97",
          "mode": "light",
          "source": {
            "resource": "file:///Users/pyvignau/workspace/ftven/galaxie/packages/new-tokens/src/dtcg/tokens.json",
            "loc": {
              "start": {
                "line": 76,
                "column": 22,
                "offset": 2474
              },
              "end": {
                "line": 85,
                "column": 12,
                "offset": 2764
              }
            }
          }
        }
      }
    },
    {
      "$name": "color.bg.accent.strong.enabled",
      "$type": "color",
      "$value": {
        "colorSpace": "srgb",
        "components": [
          1,
          1,
          1
        ],
        "alpha": 1,
        "hex": "#ffffff"
      },
      "$extensions": {
        "app.terrazzo.listing": {
          "names": {
            "scss": "--color-bg-accent-strong-enabled"
          },
          "originalValue": "{core.color.blue.500}",
          "previewValue": "#ffffff",
          "mode": "dark",
          "source": {
            "resource": "file:///Users/pyvignau/workspace/ftven/galaxie/packages/new-tokens/src/dtcg/tokens.json",
            "loc": {
              "start": {
                "line": 76,
                "column": 22,
                "offset": 2474
              },
              "end": {
                "line": 85,
                "column": 12,
                "offset": 2764
              }
            }
          }
        }
      }
    },

These are two tokens with different modes light and dark, previewValue is correct but the original value is the same on both (uses the light mode value)
This is not a false value as long as the absolute default value for me is the light one but the original value for the dark mode is wrong where blue.500 is not #ffffff.

Cheers !

[Sidnioulz]

This all looks great! I think I’d still like to see a couple tests, even if they’re very simple smoke tests. But once those are in, would be happy to merge & release.

Do you have specific things in mind? I'm at 84% branch coverage at the moment and not sure in which direction to take things to make tests better.

[Sidnioulz]

We discussed the absolute filepath on the source.resource key. It'd be really great to have a relative path or maybe a configuration key to force this attr.

    {
      "$name": "color.bg.accent.strong.enabled",
      /* ... */
      "$extensions": {
        "app.terrazzo.listing": {
          /* ... */
          "source": {
            "resource": "file:///Users/pyvignau/workspace/ftven/galaxie/packages/new-tokens/src/dtcg/tokens.json",
            "loc": {
              "start": { "line": 76, "column": 22, "offset": 2474 },
              "end": { "line": 85, "column": 12, "offset": 2764 }
            }
          }
        }
      }
    },

@drwpow how do you think the Resolver Spec will affect that?

Right now, to provide source info on a token, I copy your own internal source object. It's great as it points to the correct resource and location, but it's absolutely resolved.

Instead, I want it to be resolved relatively so the resolver.json config. If resolver.json points to external files, I want the same exact file paths:

>             "resource": "path/to/resource.json",
>       // or "resource": "file://<root>/path/to/resource.json",
>             "loc": { /* ... */ }             }

If it has inline tokens, I want to point to the resolver file itself:

>             "resource": "file://<root>/resolver.json",
>             "loc": { /* ... */ }             }

I'm also not entirely sure whether Token Listing should embed resolver configs, because of the risk posed by large inline token sets. WDYT?

[Sidnioulz]

These are two tokens with different modes light and dark, previewValue is correct but the original value is the same on both (uses the light mode value)
This is not a false value as long as the absolute default value for me is the light one but the original value for the dark mode is wrong where blue.500 is not #ffffff.

That was a bug, thanks for catching it. I'm in the process of fixing it!

[Sidnioulz]

Oh, apologies—for some reason when I looked at it, the test file didn’t load (I think I was using the new GitHub UI which I’ve had problems with files not loading at times). I do see your test and it looks great.

All good, I did have part of the format not tested, and believe it or not, there was a bug exactly there. 🙃

[Sidnioulz]

From the standpoint of Terrazzo, though, we still do resolve things absolutely in the end because that’s how we track circular and/or invalid $refs. So the URL behavior like we’re doing will probably stay. But also, if you choose to express things in terms of $ref syntax, that will work too, more than likely (again, since #530 merged)

Hm, makes sense. Yeah, I'd like to steal some of your time to talk about that sync. I do want to ensure that I output something as DTCGish as possible, so that I have better interop out of the box with other tools and data hosting contexts.

[Sidnioulz]

@drwpow I can't check the Cloudflare Pages logs so can't find what the error is. Builds do pass locally.

Also unsure what to do with the Windows UT failures. It seems I can no longer use path.relative so I don't know how to properly resolve those source paths I'm retrieving from Terrazzo. Any ideas?

[drwpow]

This is optional when extends: "//" exists according to their documentation. We can delete this!

[Sidnioulz]

If there is no harm in it, having it would avoid having to re-remove it from all packages when running biome migrate. Here as well, happy to revert!

[drwpow]

Why this change?

[Sidnioulz]

I ran biome on the whole monorepo and it moved. So I figured I might as well embrace the linter's preference. Happy to revert that commit if you prefer though.

[drwpow]

There’s no test folder in this package. So this will ship all *.test.* files to npm. IMO we shouldn’t ship test files to npm, so moving them in there is preferable.

Either that, or change this .npmignore file to also ignore **/*.test.*, but then it’s inconsistent with the rest

[Sidnioulz]

I'm confused. I think I might be too tired to understand what you're saying. What should I change?

For the record I copied this from plugin-css, so any adjustment made here might need to be made on many/all other packages.

[drwpow]

Suggestion: for Windows, IMO it is good to test the snapshot. So we could do something that just checks for e.g.

.replace(/"resource": "file:\/\/.*\/build-default\/tokens\.json", '"resource": "file://<root>/build-default/tokens.json"')

[Sidnioulz]

Thanks for the suggestion!

I'll first rework the code to use URLs because it's what I should have done, and if I'm lucky, it might fix the issue.

I really really want those URLs to be stable across build environments until I can make something more in line with resource names in resolver.json.

[drwpow]

I hear you but “stable across all build environments” is very tricky! There’s not only POSIX vs Windows, this may also have different behavior in containers as well as tools like Bazel. It’s generally rare for tools that deal with resolution to be platform-agnostic; even bundlers don’t do this. Anyways, if the “juice is worth the squeeze” go for it, but there should be clear advantages

[pvignau]

previewValue depending on the mode is fixed ! Thanks :)