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

outputReferences not properly resolved when using custom name transform #1037

Closed
spiess-demos opened this issue Oct 30, 2023 · 10 comments
Closed

Comments

@spiess-demos
Copy link

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?

@chris-dura
Copy link

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 style-dictionary@v3.8.0

$dialog-border-width: 0 !default;

// Output style-dictionary@v3.9.0

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

@chris-dura
Copy link

chris-dura commented Nov 22, 2023

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.

wmfgerrit pushed a commit to wikimedia/design-codex that referenced this issue Dec 5, 2023
This reverts commit b38796c.

Reason for revert: Update broke deprecated token dist files, see amzn/style-dictionary#1037

Change-Id: Ibc5962f0ff282360ebe6c3fa7a1498cda16da537
spiess-demos added a commit to demos-europe/demosplan-ui that referenced this issue Dec 8, 2023
style-dictionary 3.9 causes the tokens build to
output invalid scss.
See amzn/style-dictionary#1037
spiess-demos added a commit to demos-europe/demosplan-ui that referenced this issue Dec 8, 2023
style-dictionary 3.9 causes the tokens build to output invalid scss.
See amzn/style-dictionary#1037
spiess-demos added a commit to demos-europe/demosplan-ui that referenced this issue Jan 19, 2024
With style-dictionary 3.9 the way we transformed
variables (within the name transform, by changing
the tokens path) did not work anymore. By moving the
(slightly adapted) logic into a custom format, we can
safely update style-dictionary.

see
- https://github.com/amzn/style-dictionary/blob/17f4cb2f30bd002dfd55d6ef8c5bee4138de8d64/lib/common/formatHelpers/formattedVariables.js#L46
- amzn/style-dictionary#1037
@spiess-demos
Copy link
Author

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 added a commit to demos-europe/demosplan-ui that referenced this issue Jan 19, 2024
* build(deps-dev): bump style-dictionary from 3.8.0 to 3.9.2

Bumps [style-dictionary](https://github.com/amzn/style-dictionary) from 3.8.0 to 3.9.2.
- [Release notes](https://github.com/amzn/style-dictionary/releases)
- [Changelog](https://github.com/amzn/style-dictionary/blob/main/CHANGELOG.md)
- [Commits](amzn/style-dictionary@v3.8.0...v3.9.2)

---
updated-dependencies:
- dependency-name: style-dictionary
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps-dev): bump style-dictionary from 3.8.0 to 3.9.2

Bumps [style-dictionary](https://github.com/amzn/style-dictionary) from 3.8.0 to 3.9.2.
- [Release notes](https://github.com/amzn/style-dictionary/releases)
- [Changelog](https://github.com/amzn/style-dictionary/blob/main/CHANGELOG.md)
- [Commits](amzn/style-dictionary@v3.8.0...v3.9.2)

---
updated-dependencies:
- dependency-name: style-dictionary
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps-dev): bump style-dictionary from 3.8.0 to 3.9.2

Bumps [style-dictionary](https://github.com/amzn/style-dictionary) from 3.8.0 to 3.9.2.
- [Release notes](https://github.com/amzn/style-dictionary/releases)
- [Changelog](https://github.com/amzn/style-dictionary/blob/main/CHANGELOG.md)
- [Commits](amzn/style-dictionary@v3.8.0...v3.9.2)

---
updated-dependencies:
- dependency-name: style-dictionary
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* fix: move variables transformation into formatter

With style-dictionary 3.9 the way we transformed
variables (within the name transform, by changing
the tokens path) did not work anymore. By moving the
(slightly adapted) logic into a custom format, we can
safely update style-dictionary.

see
- https://github.com/amzn/style-dictionary/blob/17f4cb2f30bd002dfd55d6ef8c5bee4138de8d64/lib/common/formatHelpers/formattedVariables.js#L46
- amzn/style-dictionary#1037

* chore: do not lock style-dictionary anymore

* chore: add reference to original fn

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hans Spieß <spiess@demos-deutschland.de>
Co-authored-by: spiess-demos <40452344+spiess-demos@users.noreply.github.com>
@chris-dura
Copy link

@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

@spiess-demos
Copy link
Author

@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 spiess-demos reopened this Jan 22, 2024
@chris-dura
Copy link

@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.

@jookshub
Copy link

   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.

@jorenbroekema
Copy link
Collaborator

jorenbroekema commented Apr 25, 2024

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
Copy link
Collaborator

jorenbroekema commented Apr 25, 2024

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?

@spiess-demos
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants