amzn / style-dictionary

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

Possible bug or need for documentation: Files run async #1341

Open lukasoppermann opened 2 months ago

lukasoppermann commented 2 months ago

Hey, I spent two days debugging my code because I got a racing condition with files.

It seems that from v3 to v4 there was a change and now the files defined in the files object run in parallel.

A common approach before this change was to write general fornats first and then overwrite the specific files afterwards, e.g.

    files: [
      {
        destination: `${outputFile}`,
        format: `css/advanced`,
        filter: token => isSource(token)
        options: {
          showFileHeader: false,
          outputReferences: false,
          descriptions: false,
          queries: getCssSelectors(outputFile),
          ...options?.options,
        },
      },
      {
        destination: `${outputFile}`,
        format: `css/customMedia`,
        filter: token => isSource(token) && token.$type === 'custom-viewportRange',
        options: {
          showFileHeader: false,
        },
      }
]

This does not work anymore as sometimes the second file is generated while the first one is still writing to the same file.

While I appreciate this may be "incorrect" usage, I would prefer the old way. If this is not desired, maybe an information that the files are not written syncronously but async would be helpful.

jorenbroekema commented 2 months ago

Hmm yeah I think I would classify this under incorrect usage. The API was not designed for the purpose of sequentially writing to the same file. I'd have to know a little bit more about your use case to understand why you would want multiple files to be sequentially overwritten, because to me it seems like it just completely un-does the work of the previous file, so then I'm tempted to say "the first one shouldn't be there if the second one always overwrites it anyways"

lukasoppermann commented 2 months ago

Hey @jorenbroekema,

the use case is just simplicity. I can run all tokens through a default setup and filter down afterwards and overwrite the files that have special formats.

Now I have to add filters to every file like token is not of type ABC, etc.

If you feel it should be like this, maybe a "warning" could still be added, as the user does not know that the files are written async and v3 used to write in sync.

jorenbroekema commented 2 months ago

https://styledictionary.com/version-4/migration/#asynchronous-api we can add a caution thingie here to mention that most methods in SD that tend to loop over stuff (e.g. platforms & files) now run in parallel and if you were relying on them running in sequence, that would break.

This was a pretty huge performance improvement for many use cases so I don't want to revert that

lukasoppermann commented 2 months ago

This was a pretty huge performance improvement for many use cases so I don't want to revert that

Totally get it and there are ways to work around it, e.g. adding more filtering to every step. I just think it is important to mention.

Personally I feel like it would be better to mention it here: https://styledictionary.com/reference/config/#platform

E.g.

Files to be generated for this platform asynchronously.

Or somewhere here.

Charlene-Bx commented 1 month ago

Running on the same issue. I have a design system divided into themes, each of which has breakpoints. Since the files have identical paths, I'm obliged (at least that's how it seemed to me after many tests) to generate them separately by file so as not to “overwrite” them. I then use an action to merge surrounding them with the desired media queries, all together. But now that's not synchronous, sometimes it starts by medium media queries, and sometimes by the large one, thus modifying the output order too.