style-dictionary icon indicating copy to clipboard operation
style-dictionary copied to clipboard

outputReferences not properly resolved when using custom name transform

Open spiess-demos opened this issue 8 months ago • 6 comments

I apply a custom name transform to my Scss transform group, to align Tailwind with legacy variable names. My config goes like that (for brevity i only show the Scss-related code here, repo here):

StyleDictionary.registerTransform({
  name: 'name/scss',
  type: 'name',
  transformer: (token) => {
    // "DEFAULT" is a Tailwind naming convention that should not be part of the Scss name
    if (token.path[1] === 'DEFAULT') {
      token.path.splice(1, 1)
    }
    return token.path.join('-')
  }
})

StyleDictionary.registerTransformGroup({
  name: 'custom/scss',
  transforms: StyleDictionary.transformGroup.scss.concat(['name/scss'])
})

const StyleDictionaryExtended = StyleDictionary.extend({
  source: [tokensPath],
  platforms: {
    scss: {
      transformGroup: 'custom/scss',
      buildPath: 'tokens/dist/scss/',
      files: files.map((filePath) => {
        return {
          destination: `_${filePath}.scss`,
          format: 'scss/variables',
          filter: (token) => token.filePath.includes(filePath),
          options: {
            outputReferences: true
          }
        }
      })
    },
  // ...
  }
})

I also keep legacy variables around, aliasing them to the new Tailwind variables:

{
  "rounded": {
    "none": {
      "value": "0"
    },
    "sm": {
      "value": "2px"
    },
    "smallest": {
      "value": "{rounded.sm}"
    },
    "DEFAULT": {
      "value": "3px"
    },
    "small": {
      "value": "{rounded.DEFAULT}"
    },
    "md": {
      "value": "4px"
    },
    "medium": {
      "value": "{rounded.md}"
    },
    "lg": {
      "value": "5px"
    },
    "large": {
      "value": "{rounded.lg}"
    },
    "full": {
      "value": "9999px"
    }
  }
}

Before v3.9.0, this resulted in the desired output:

$dp-rounded-full: 9999px;
$dp-rounded-lg: 5px !default;
$dp-rounded-md: 4px !default;
$dp-rounded: 3px !default;
$dp-rounded-sm: 2px !default;
$dp-rounded-none: 0;
$dp-rounded-large: $dp-rounded-lg;
$dp-rounded-medium: $dp-rounded-md;
$dp-rounded-small: $dp-rounded;
$dp-rounded-smallest: $dp-rounded-sm;

Since style-dictionary v3.9.0, the alias of the token that has been changed by the name transform ({rounded.DEFAULT}) is not being resolved properly anymore:

$dp-rounded-full: 9999px;
$dp-rounded-lg: 5px !default;
$dp-rounded-md: 4px !default;
$dp-rounded: 3px !default;
$dp-rounded-sm: 2px !default;
$dp-rounded-none: 0;
$dp-rounded-large: $dp-rounded-lg;
$dp-rounded-medium: $dp-rounded-md;
$dp-rounded-small: {rounded.DEFAULT};
$dp-rounded-smallest: $dp-rounded-sm;

Is there any way to work around this on my side, or is it a bug within the latest release of style-dictionary?

spiess-demos avatar Oct 30 '23 13:10 spiess-demos

FWIW, I'm seeing the same thing happen with my custom transform (or custom transform group?), when the value is 0.

I prefer to define tokens in px, not rem, so I replace the predefined size/rem transform in the predefined css transform group with the size/pxToRem transform.

/**
 * Transform group for the Web platform.
 * Extends the pre-defined `css` group.
 * - Replaces `size/rem` transform with `size/pxToRem` transform.
 * - Adds custom `size/relativeLineHeight` transform.
 */
module.exports = {
	name: "my-css-group",
	transforms: [
		// from pre-defined `css` group...
		"attribute/cti",
		"name/cti/kebab",
		"time/seconds",
		"content/quote",
		"content/icon",
		"size/pxToRem",
		"size/relativeLineHeight",
		"color/css",
	],
};
// Output [email protected]

$dialog-border-width: 0 !default;

// Output [email protected]

$dialog-border-width: {border.width.none.value} !default;

chris-dura avatar Nov 22 '23 22:11 chris-dura

Not a defect, but related to the changes made to outputReferences in the repo, and maybe worth adding a unit test or failing the builds with an error...

Related issue...

I (mistakenly) defined a reference with extra space after and before the braces and it compiled fine; however in v3.9.0 the output has changed.

	"link-icon-fill": {
		value: "{ color.primary.value }",
	}
// output: v3.8.0

$link-icon-fill: $color-primary !default;

// output: v3.9.0

$link-icon-fill: { color.primary.value } !default;

If it is required to have no spaces after/before the braces in a reference, then the build should fail and throw an error.

chris-dura avatar Nov 22 '23 23:11 chris-dura

I managed to resolve my issue by moving the variable transform logic from the name transform to a customized version of formattedVariables:

StyleDictionary.registerFormat({
  name: 'scss/customVariables',
  formatter: ({dictionary, options, file}) => {
    const { outputReferences, themeable = false, formatting} = options
    let { allTokens } = dictionary

    if (outputReferences) {
      allTokens = [...allTokens].sort(sortByReference(dictionary))
    }

    const propertyFormatter = createPropertyFormatter({ outputReferences, dictionary, format: 'sass', formatting, themeable })

    const tokens = allTokens
      .map(token => {
        let formattedVar = propertyFormatter(token)

        // "DEFAULT" is a Tailwind convention that should not be part of the Scss name
        formattedVar = formattedVar.replace(/-DEFAULT/g, '')

        return formattedVar
      })
      .filter(function(strVal) { return !!strVal })
      .join('\n')

    return fileHeader({file, commentStyle: 'short'}) + '\n' + tokens + '\n'
  }
})

const StyleDictionaryExtended = StyleDictionary.extend({
  source: [tokensPath],
  platforms: {
    scss: {
      transformGroup: 'custom/scss',
      buildPath: 'tokens/dist/scss/',
      files: files.map((filePath) => {
        return {
          destination: `_${filePath}.scss`,
          format: 'scss/customVariables',
          filter: (token) => token.filePath.includes(filePath),
          options: {
            outputReferences: true
          }
        }
      })
    },
  // ...
  }
})

spiess-demos avatar Jan 19 '24 09:01 spiess-demos

@spiess-demos -- I still think this is an open issue. You found a workaround; however, it's still a defect in StyleDictionary 3.9.x, I believe since I was seeing similar behavior outside of name transforms

chris-dura avatar Jan 20 '24 00:01 chris-dura

@chris-dura i was unsure if i simply used sd in an unintended way before. Reopening this as it might be still an issue.

spiess-demos avatar Jan 22 '24 07:01 spiess-demos

@chris-dura i was unsure if i simply used sd in an unintended way before. Reopening this as it might be still an issue.

@spiess-demos -- Maybe the maintainers will disagree (and I haven't had time to test the latest SD releases), but v3.9.0 is definitely NOT resolving outputReferences like v3.8.0 was... so, to me, that's a defect.

If the new behavior in v3.9.0 is actually what is expected, then I guess we'll have to workaround it, like you have.

chris-dura avatar Jan 24 '24 03:01 chris-dura

   const cssLines = dictionary.allTokens
      .sort(sortByReference(dictionary))
      .map(hexToRgba)
      .map((token) => {
        console.info('hexToRgba result: ', token.value);
        return token;
      })
      .map(formatter)
      .map((token) => {
        console.info('formatter result:', token);
        return token;
      });

Hello 👋 the above code is having similar issues since the update from 3.8.0 to 3.9.2 I suspect due to the changes to createPropertyFormatter or the outputReferences

formatter result with 3.8.0: --element-muted: rgba(204,219,232,0.4); formatter result with 3.9.0 and 3.9.2: --element-muted: rgba(var(--global-colors-light-light-blue), 0.40);

In both cases the hexToRbga conversion worked and replaced the token value properly.

Current browsers seem to have issues with the var inside of rgba and are not displaying the proper colors.

I haven't dug deep enough yet to determine weather I need to adjust our code or if this is an issue of 3.9.0 but will roll-back for now as a quick-fix.

jookshub avatar Apr 24 '24 12:04 jookshub

Hey @jookshub , quick background, the outputReferences utility used to, in 3.8.x, output refs by doing a find & replace action on the token's resolved final value, e.g.

"rgba(204,219,232,0.4)".replace("#ccdbe866", "var(--element-muted)")

Because the HEX value was transformed to RGBA, this find and replace action fails, so you end up with just rgba(204,219,232,0.4) in the output. However, this way of outputting refs could lead to issues pretty easily, imagine a token value of: {ref} * 11, and imagine that ref resolves to 8 this is then transformed by a math resolve transform so it becomes 88, this means our find replace looks like this: "88".replace("8", "var(--ref)") which leads to an incorrect output: var(--ref)8 or if a global flag was used for the reg, var(--ref)var(--ref). This issue happened quite often for people and it's quite puzzling to understand how or why it happens, so to fix it was pretty high priority

Therefore, we introduced a fix (both in 3.9.0 and v4 branch) to this issue by running a find-replace on the token.original.value, which means the find and replace looks like this: "{ref} * 11".replace("{ref}", "var(--ref)") -> var(--ref) * 11

The downside of this approach is that we're looking at the original untransformed value, so the resolve math transform work has been undone, which is also not ideal but at least it fixes the more egregious issue.

Another user has also noticed that this causes a regression and they raised an issue for that. My initial conclusion is that this regression is a good tradeoff and that using outputReferences should be avoided for tokens that have been transitively transformed.

Therefore I introduced a feature that allows you to conditionally output refs on a per token basis, and I introduced an easy utility function that identifies for a token whether outputting a ref would undo the work of a transitive transformed, this feature is documented here and even has a detailed live demo, but it will require you to migrate to v4 latest prerelease to use this, v4 branch is still unstable but will be getting a full release in June this year :)

jorenbroekema avatar Apr 25 '24 11:04 jorenbroekema

Oh and btw to more accurately answer the original post in this issue:

StyleDictionary.registerTransform({
  name: 'name/scss',
  type: 'name',
  transformer: (token) => {
    // "DEFAULT" is a Tailwind naming convention that should not be part of the Scss name
    if (token.path[1] === 'DEFAULT') {
      token.path.splice(1, 1)
    }
    return token.path.join('-')
  }
})

You probably want to change this to:

StyleDictionary.registerTransform({
  name: 'name/scss',
  type: 'name',
  transformer: (token) => {
    const clonedPath = structuredClone(token.path);
    // "DEFAULT" is a Tailwind naming convention that should not be part of the Scss name
    if (token.path[1] === 'DEFAULT') {
      clonedPath.splice(1, 1);
    }
    return clonedPath.join('-');
  }
});

Here's a demo to show that it works in the latest v4 prerelease of style-dictionary at least (which is why I'm closing the issue, but feel free to reopen if it's still an issue in 3.9.x somehow, I'll investigate further).

The reason why we need to do a clone before we .splice is because we don't want to mutate the token path itself by doing .splice on a non-primitive (array) in JavaScript, because the token path is used to resolve references later down the line, and it's really important that the value hasn't changed by that time. In 3.8.x the way outputReferences was done and how the ref is resolved is slightly different which is why this issue didn't show itself in that version, but it's a general problem of your transform mutating important token metadata, that just so happens to show itself in 3.9.x onwards.

Also just a general tip for your transform, you've hardcoded the position in the path array to be 1 for "DEFAULT", I'm assuming it could happen that this "DEFAULT" is in the path at a different location, so it might make sense to go through each path item and splice out "DEFAULT" at any index it occurs?

jorenbroekema avatar Apr 25 '24 11:04 jorenbroekema

@jorenbroekema thanks for coming back to the original question!

The reason why we need to do a clone before we .splice is because we don't want to mutate the token path itself by doing .splice on a non-primitive (array) in JavaScript, because the token path is used to resolve references later down the line, and it's really important that the value hasn't changed by that time. In 3.8.x the way outputReferences was done and how the ref is resolved is slightly different which is why this issue didn't show itself in that version, but it's a general problem of your transform mutating important token metadata, that just so happens to show itself in 3.9.x onwards.

This explains a few things!

Also just a general tip for your transform, you've hardcoded the position in the path array to be 1 for "DEFAULT", I'm assuming it could happen that this "DEFAULT" is in the path at a different location, so it might make sense to go through each path item and splice out "DEFAULT" at any index it occurs?

In my use case it is only at position 1, but you'd be right otherwise.

spiess-demos avatar Apr 25 '24 11:04 spiess-demos