feat(tokens): support latest style-dictionary build#2500
Conversation
File metricsSummaryTotal size: 2.25 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailsaccordion
assetcard
calendar
card
colorhandle
colorslider
dropzone
menu
swatch
table
thumbnail
underlay
well
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-2500--spectrum-css.netlify.app |
676fef5 to
2c41fa6
Compare
9548f0f to
deeeac9
Compare
16f409e to
c0f9463
Compare
|
098738b to
dc002cf
Compare
dc002cf to
ae16d2a
Compare
ae16d2a to
6cd570b
Compare
dd0694c to
b565823
Compare
b565823 to
20705b6
Compare
20705b6 to
9164f9f
Compare
9164f9f to
9880b3a
Compare
cbbffd8 to
e1a3792
Compare
|
Something to evaluate: https://github.com/storybookjs/storybook/pull/29676/files |
22c51f3 to
58d3a83
Compare
58d3a83 to
621f3d0
Compare
8ea4ca8 to
cfbd9f9
Compare
| fi | ||
|
|
||
| # If there are changes, capture the changes and upload them as an artifact | ||
| - name: Capture changes |
There was a problem hiding this comment.
Doing this as a separate task helps us identify issues more easily in CI
There was a problem hiding this comment.
Is there a way to validate this? I see the moved command now has a --staged flag; could you explain why?
There was a problem hiding this comment.
Yeah for sure, the --staged flags ensures we're only diffing the changes detected, meaning those we staged in the previous step. By separating it into its own step instead of including it above, the logs are now easier to read. The more command line steps you run together, the harder it is to distinguish the results in the log output.
jawinn
left a comment
There was a problem hiding this comment.
I didn't notice any specific issues here and mostly just had questions! Some additional validation instructions and PR description of the various changes would be helpful, as it was a little hard to review all the pieces in a single commit.
I wasn't able to check off the one validation instruction that is there now, as the size of the files have changed slightly, probably due to changes since it was originally written; they were only a few KB different.
| fi | ||
|
|
||
| # If there are changes, capture the changes and upload them as an artifact | ||
| - name: Capture changes |
There was a problem hiding this comment.
Is there a way to validate this? I see the moved command now has a --staged flag; could you explain why?
marissahuysentruyt
left a comment
There was a problem hiding this comment.
I think this looks good. As far as I can tell, things are working as they were before. When I checked the file sizes of the dist/css, mine didn't exactly line up to your validation notes- is that an issue? They were really close, but not exact - that's all.
My main question is just how else would you like me to validate this, or is there anything else you want me to check? Is there anything that's going to change once this is merged that we should be on the lookout for?
castastrophe
left a comment
There was a problem hiding this comment.
Made a few updates based on this feedback and added more context. Let me know if you want to sync soon and I can talk through any remaining questions as well.
@marissahuysentruyt It's a tough one to validate yeah because the goal is that everything works the same but now instead of CJS, we're leveraging ESM syntax and the latest release of style-dictionary. It's maybe a subtle performance gain in how fast it runs but overall, hopefully, no significant change. As for the numbers being off, we probably had a token change since I wrote up the test cases so the file sizes may have updated. |
marissahuysentruyt
left a comment
There was a problem hiding this comment.
As you noted, this branch seems to be running the same as before. 👍 I don't think I see any glaring issues, so I'm good to approve. Thanks also for the extra context and the few extra commands to help us validate!
Description
When adding an experimental windows-latest run, I found that the style-dictionary compilation was failing pretty terrifically. This PR aims to fix that by wrapping pathing in path.join so that it can be rendered in a windows-friendly format.
Update: Our windows compatibility issues are much more complex than anticipated. Those updates, while excellent for hardening the tooling here, did not solve the problem(s). The scope of this pull request is no longer to support Windows builds for CI, but instead of handle the breaking change migration for style-dictionary.
Hardening steps
Update the .gitattributes to force windows to use lf instead of crlf so that the generated mods files don't show a diff after the build.
Switch the style-dictionary build to use the vanilla command instead of the nx executor.
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
yarn builder tokens. Expect the token build to create the following assets:Validation steps
This can be validated via CI which will try to run the tokens build in windows. If the task fails, this PR did not work.
To-do list