sortByReference inverted (and incorrect) when using DTCG standard

[chris-dura]

// config.mjs

{
  "platforms": {
    "css": {
      "transformGroup": "css",
      "files": [
        {
          "destination": "vars.scss",
          "format": "scss/variables",
          "options": {
              "outputReferences": true
          }
        }
      ]
    }
  }
}

v3 format:

{
  "colors": {
    "red": {
      "value": "#ff0000",
      "type": "color"
    },
    "primary": {
      "value": "{colors.red}",
      "type": "color"
    }
  }
}

Expected output, $colors-red is referenced after it is declared:

// Do not edit directly, this file was auto-generated.

$colors-red: #ff0000;
$colors-primary: $colors-red;

Change to DTCG format:

{
  "colors": {
    "red": {
      "$value": "#ff0000",
      "$type": "color"
    },
    "primary": {
      "$value": "{colors.red}",
      "$type": "color"
    }
  }
}

Incorrect output, $colors-red is referenced before it is declared.

// tokens.scss output

// Do not edit directly, this file was auto-generated.

$colors-primary: $colors-red;
$colors-red: #ff0000;

[jorenbroekema]

Oof that's not good, definitely seems like a bug, will take a look

[lukasoppermann]

Hey, any news here? I may be running into the same issue.

Also this example suggest to use dictionary, but the type needs TransformedTokens so dictionary.tokens: https://github.com/amzn/style-dictionary/blob/main/examples/advanced/format-helpers/sd.config.js#L38

This is currently blocking us from updating. I am using this in a custom format:

But because of the issue with the order I get an error: 🛑 Error trying to build Figma output: Error: Tries to reference base.color.neutral.8, which is not defined.!

Nevermind, I was actually having another issue and passing unfilteredTokens solved it:

Just if someone runs into this issue when updating, I replace

getReferences(refString, dictionary.tokens)

with

getReferences(refString, dictionary.tokens, {unfilteredTokens: dictionary.unfilteredTokens})

[lukasoppermann]

@jorenbroekema do we just need to make sure to use $value if usesDtcg is true in link

style-dictionary/lib/common/formatHelpers/sortByReference.js

Line 57 in 0cf8102

if (a.original && usesReferences(a.original.value)) {

and lines 59, 60, 64?

But it seems like the function does not have access to the Config object atm, right?

[jorenbroekema]

@lukasoppermann yeah that definitely needs to use original.$value if usesDtcg is true.

We should add the usesDtcg boolean prop to the options parameter (currently only has unfilteredTokens prop), and ensure that internally in Style Dictionary this sortByReference utility is called passing this option. As far as I know it is called from the format lifecycle where we do have the Config available.

PR welcome!

[timges]

Unfortunately ran into the same issue. I submitted a PR that should fix it for now 🫡

That being said, I feel like it might be considerable to refactor the way the usesDtcg option is used across the codebase in a whole. Right now, it's repeated a lot througout the project like:

usesDtcg ? '$value' : 'value'
// or
usesDtcg ? '$type' : 'type'

It actually appears to be only used this way.

There are small inconsistencies in how it's written exactly, which makes it a bit harder to track. I was thinking a utility function could help standardize this.
Another thought was to actually remove the usesDtcg option in the first place and just fall back to $value when value is undefined. Could already seal the deal, couldn't it?

I'm not that familiar with the codebase yet, as I didn't have tooooo much time to browse through everything, so I might be missing things here! Would love to get your feedback though 😊

[jorenbroekema]

Hey @timges yeah you're right, it's a bit of a code smell to repeat that ternary operator everywhere in the codebase.
Even if we were to abstract that to a simple utility function, it still wouldn't really improve it all that much because the root of the code smell is that fact that we have to pass an option "usesDtcg" all throughout the codebase constantly because so much of the codebase needs to access the token type / value.

I think, in hindsight, that it would have been better to just introduce a built-in processing step that always converts any dictionary to DTCG syntax no matter what. That way, we always only check for $value/$type/$description.

We may even be able to do this in a non-breaking change in v4. I'm gonna create an issue for this at least because it would certainly clean up the code by a lot and prevent us needing to do a bunch of DTCG syntax integration tests, we only do that in a limited amount of tests, so DTCG syntax test coverage just really isn't great right now.

Edit: Actually, this would always be a breaking change because in custom hooks that folks register, they'd get the tokens in DTCG format always, so it breaks the hooks for folks that still only use the old syntax. So this'll be a v5 improvement then..

[timges]

100% agree with you @jorenbroekema! Thanks for the feedback 🙏

I'd definitely go for a full switch from the current standard to DTCG syntax at some point. As you mentioned, since it's a breaking change anyway, it might be best to just drop support for the old standard with v5.

Test situation is a little concerning, tbh. The only other integration test suite that covers DTCG syntax seems to be w3c-forward-compat.test.js.

Perhaps we could eliminate the usesDtcg option without introducing a breaking change by always trying to access a value using the DTCG syntax first, and if it's undefined, falling back to the standard syntax. Also hacky, but at least test coverage would implicitly be guaranteed this way.

[jorenbroekema]

Test situation is a little concerning, tbh. The only other integration test suite that covers DTCG syntax seems to be w3c-forward-compat.test.js.

Actually there are a couple of other places where we also have a test that's for DTCG syntax but you'd have to search for them to find them in the regular test files.

trying to access a value using the DTCG syntax first, and if it's undefined, falling back to the standard syntax

I'm pretty sure that's what I went with the first time I tried to add forward compatibility with DTCG syntax but there were some issues with this approach, I don't remember exactly from the top of my head but it was something along the lines of:

• there can be token groups with name "value" in DTCG syntax, so if we use $value ?? value we might get a false-positive hit for a "token" when it's really just a token group. I know this to be a real world use-case for people, because they raised an issue for this in one of the prereleases when I used trial & error + fallback method for checking $prop ?? prop
• people were using mixed forms of old syntax and new syntax, by introducing auto-detection and introducing usesDtcg as a boolean flag, I've essentially explicitly said we don't support that (it introduces a whole can of worms on its own trying to deal with mixed format)

I'm not sure if it would improve test coverage either because the main reason we have these minor bugs when not taking account DTCG format is because we access token.value or token.original.value or token.type somehow and we forgot to check for $value/$type, whether we do so by trial and error + fallback approach or by checking the usesDtcg option doesn't seem that relevant for test coverage I think, you'd only get the coverage if you were always checking your tests with both old syntax JSON inputs and DTCG syntax JSON inputs and for most tests we just don't.

[timges]

there can be token groups with name "value" in DTCG syntax, so if we use $value ?? value we might get a false-positive hit for a "token" when it's really just a token group. I know this to be a real world use-case for people, because they raised an issue for this in one of the prereleases when I used trial & error + fallback method for checking $prop ?? prop
people were using mixed forms of old syntax and new syntax, by introducing auto-detection and introducing usesDtcg as a boolean flag, I've essentially explicitly said we don't support that (it introduces a whole can of worms on its own trying to deal with mixed format)

That makes a lot of sense! Dang it. Guess we reallly have to wait for v5 then, to properly improve here.

Anyways - thanks for the awesome exchange! 😊