amzn / style-dictionary

A build system for creating cross-platform styles.
https://styledictionary.com
Apache License 2.0
3.87k stars 543 forks source link

Deep merge of files only until token #951

Closed lukasoppermann closed 1 year ago

lukasoppermann commented 1 year ago

Hey, so I am running into an issue where I have a file light.json5 and want to overwrite some tokens in this file for another build by using both light.json5 and light.extended.json5 in the source.

My issue is that if a token exists in light with properties that are not present in light.extended, they get added, e.g.:

// light.json5
// ...
{
  // ...
  linkColor: {
    value: "blue",
    alpha: 0.8
  }
}
// light.extended.json5
// ...
{
  // ...
  linkColor: {
    value: "green",
  }
}

I will end up with a combination like this in light.extended

// light.extended.json5
// ...
{
  // ...
  linkColor: {
    value: "green",
    alpha: 0.8
  }
}

Expected behavior

What I would expect is that the deep merge only goes until the token level and so everything inside linkColor would be replaced by what is present in light.extended.

Is this something that could be changed, or is this needed and thus will be kept? @chazzmoney @dbanksdesign

Sidnioulz commented 1 year ago

Hi @lukasoppermann!

Just wanted to share that if I were to sort my tokens that way, I would want to keep the other attributes. Let's consider some examples of things other users could do.


In the first example, I use attributes on my original token to ensure it uses the right transform, or to pass down metadata to my format:

// light.json5
{
  linkColor: {
    value: "blue",
    alpha: 0.8,
    attributes: {
      category: 'color',
      someImportantThing: 'value'
    }
  }
}

Your proposal could cause the destruction of attributes.


In the second example, let's say I use composite values:

// light.json5
{
  link: {
    value: {
      color: "blue",
      alpha: 0.8,
      weight: "bold",
    }
  }
}

Then how should the merge happen? Should values be appended if arrays? Deep merged if objects? Overridden in all circumstances? Could you proposal result in more values having to be duplicated in light.extended.json5 for the use cases where a single part of the value is changed?


Finally, let's consider the multi-value tokens proposal I'm working on (and using in my org):

// light.json5
{
  linkColor: {
    value: "#3333aa",
    value_hiContrast: "#0000ff",
    alpha: 0.8
  }
}

If I used your proposed JSON merge to, e.g., implement multi-brands on top of my multi-value tokens, I wouldn't be able to modify a single value for a token and would again need to duplicate.


Instead, how about passing null to the properties you want to delete? That would allow removing properties during a merge, without threatening other use cases where undefined properties in the light.extended.json5 file should be preserved.

lukasoppermann commented 1 year ago

Hey, I am currently passing null. The problem I am having is that it is easy to forget to add a prop to add extensions.

However I see your reasoning. I am currently working on solving this with a file check beforehand.

Thanks for your comment.