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

Regression in transitive transform from version 3.9.0 #1124

Closed
pascalduez opened this issue Mar 18, 2024 · 5 comments
Closed

Regression in transitive transform from version 3.9.0 #1124

pascalduez opened this issue Mar 18, 2024 · 5 comments

Comments

@pascalduez
Copy link
Contributor

pascalduez commented Mar 18, 2024

Greetings,

prior to version 3.9.0 applying a transformation on transitive tokens used to work fine, but from version 3.9.0 the transformation is not applied anymore and only the reference output.
Maybe introduced in #1002.

Please find a reproduction here https://github.com/pascalduez/style-dictionary-transform-repro
Stripped down obviously.

{
  "length": {
    "base": {
      "type": "dimension",
      "value": "4"
    },
    "1x": {
      "type": "dimension",
      "value": "{length.base}",
      "$extensions": {
        "scale": 1
      }
    },
    "2x": {
      "type": "dimension",
      "value": "{length.base}",
      "$extensions": {
        "scale": 2
      }
    },
    "3x": {
      "type": "dimension",
      "value": "{length.base}",
      "$extensions": {
        "scale": 3
      }
    }
  }
}
 'length': {
    type: 'value',
    transitive: true,
    matcher: token =>
      token.type === 'dimension' && Object.hasOwn(token, '$extensions'),
    transformer: token => {
      return token.value * token.$extensions.scale;
  },
},

style-dictionary < 3.9.0 (expected)

:root {
  --length-base: 4;
  --length-3x: 12;
  --length-2x: 8;
  --length-1x: 4;
}

style-dictionary >= 3.9.0

:root {
  --length-base: 4;
  --length-3x: var(--length-base);
  --length-2x: var(--length-base);
  --length-1x: var(--length-base);
}
@jorenbroekema
Copy link
Collaborator

jorenbroekema commented Mar 18, 2024

This is actually a bugfix rather than a regression.

Firstly, I believe your expected outcome should be:

:root {
  --length-base: 4;
  --length-3x: 12;
  --length-2x: 8;
  --length-1x: var(--length-base);
}

Let me know if that's correct, I'm assuming by your issue that you're using outputReferences: true

I'll try to explain now why this is not a regression but rather a bugfix, it's a bit complex though...

A design token, fundamentally speaking, cannot be a reference to another token while also being a transitively transformed version of itself in the output, this is a contradiction. For example, your length-3x is a reference to the base length token, but it's a different version of that, so it doesn't actually refer to the base length token anymore when we get to the final output, that reference was only functional during the transformation. The reason < 3.8 works in your scenario is because it cannot find the reference anymore because the value was altered, so it ditches the reference and just puts the resolved value there, and it appears as if it's "working", but in fact, the token in the source is a reference, so when you say "output references", it's contradicting that a bit by not spitting out a reference, while in the token source it was a reference.

Summary: a token reference in the source does not guarantee that this reference remains intact in the output. Sometimes that's ok, but often it resulted in very buggy behavior.

What this means is in 3.8 you could get some very strange outcomes, which I encourage you to test for yourself:

{
  "length": {
    "base": {
      "type": "dimension",
      "value": "4"
    },
    "11x": {
      "type": "dimension",
      "value": "{length.base}",
      "$extensions": {
        "scale": 11
      }
    }
  }
}

Output 3.8:

:root {
  --length-base: 4;
  --length-11x: var(--length-base)4;
}

So why does it do the above?? Because in 3.8, it would do a regex replace on the resolved value, trying to see if in the resolved value it can find the reference token's value, and if so, replace it with a reference. In this case, we have 44 because 4*11 = 44, and then it tries to find 4 (ref token value) in the 44 (resolved token value) and do this regex replace.
Knowing this, you may actually think that the expected outcome would be: var(--length-base)var(--length-base) I don't recall whether the regex replace was using 'g' flag. Maybe it did, maybe it didn't, either way, the output is wrong ;)

tokens-studio/sd-transforms#13 here's an issue that encountered this specific bug, and there's a few more but that one is the main place we discussed about how to fix it. Here's another example tokens-studio/sd-transforms#121

Eventually me and Germain opened an issue on this library repo #974 and I created a Pull request which was released in 3.9, this fixed the bug but at the tradeoff that transitive transforms "work" is being undone when putting back the reference in the output. So while that can be seen as a regression, it's a tradeoff we felt was worth it to fix the bugs.

The conclusion was that transitive transforms that change the final value of a token compared to how it is in the source, is just not compatible with outputReferences feature.

However, to address this regression, we could add a relatively small fix where instead of undoing the work of transitive transforms, outputReferences will detect the diff between design token value in the source vs after transformations, and just not output a ref for such tokens in the output, while outputting refs for tokens that have not changed. In this way, I believe we still fix the bugs in 3.8 without adding this regression for users like yourself. I'll try to experiment with that a bit this week!

@jorenbroekema
Copy link
Collaborator

However, to address this regression, we could add a relatively small fix where instead of undoing the work of transitive transforms, outputReferences will detect the diff between design token value in the source vs after transformations, and just not output a ref for such tokens in the output, while outputting refs for tokens that have not changed.

Okay so I tried doing this and it became apparent that we cannot distinguish whether a value transform was an actual change to the value and thus a reference should not be outputted, or whether a value transform was merely a minor formatting change where you do still want to output the ref, so I don't think my idea for the fix works.

I did write up some ideas at one point that I still want to explore to improve this 97f26a8 , so I'm not giving up just yet, just not entirely sure if this will make it to v4

@pascalduez
Copy link
Contributor Author

pascalduez commented Mar 27, 2024

Hi @jorenbroekema,

thanks a lot for your thorough reply and analysis.
This make total sense, and somehow I always knew that this particular transformer and tokens declarations were a bit tinkered and subject to changes.
I remember to have tried many ways but couldn't make them work. I guess mostly because in a transformer you don't have access to the whole dictionary, so can't retrieve the base value.

Let me know if that's correct, I'm assuming by your issue that you're using outputReferences: true

That's correct. Although I had to tweak a bit the output (converting px to rem) to circumvent an issue #873.
Which really looks like the one you described.

As a result, I would say probably don't spend too much time on this given all the work you already have on the v4.
I'll try to scaffold this transformation in another way.

Without even trying maybe:

{
  "length": {
    "base": {
      "type": "dimension",
      "value": "4"
    },
    "1x": {
      "type": "dimension",
      "value": 1,
      "$extensions": {
        "scale": true
      }
    },
    "2x": {
      "type": "dimension",
      "value": 2,
      "$extensions": {
        "scale": true
      }
    },
    "3x": {
      "type": "dimension",
      "value": 3,
      "$extensions": {
        "scale": true
      }
    }
  }
}
'length': {
    type: 'value',
    transitive: true,
    matcher: token =>
      token.type === 'dimension' && Object.hasOwn(token, '$extensions'),
    transformer: token => {
     // Find a way to grab the `scale` value, maybe a platform option after all. Like `basePxFontSize`.  
      return token.value * scale;
  },
},

@pascalduez
Copy link
Contributor Author

Keep it simple as they say: pascalduez/style-dictionary-transform-repro#1

@jorenbroekema
Copy link
Collaborator

It appears I managed to found a pretty neat trick to introduce a fix without the caveat that formatting-only transforms are not considered.
#1154 is the PR that fixes this and the trick I do is diffing between the token value and the resolved refs version of the original value, but since I run resolveReferences on the transformed dictionary, a diff should only occur if there was a transitive transform that made a change to the token value.

Bit complicated but all you need to do to fix your scenario is apply the new outputReferencesTransformed utility function, see docs, but in summary:

import { outputReferencesTransformed } from 'style-dictionary/utils';

export default {
  platforms: {
    css: {
      transformGroup: 'css',
      transforms: ['ts/color/css/hexrgba'],
      files: [
        {
          destination: 'vars.css',
          format: 'css/variables',
          options: {
            outputReferences: outputReferencesTransformed,
          },
        },
      ],
    },
  },
};

where that transform 'ts/color/css/hexrgba' is just an example transitive transform I picked to demo this

@pascalduez pascalduez changed the title Regression in transitive transform since from version 3.9.0 Regression in transitive transform from version 3.9.0 Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants