feat(storybook): update sass loaders and use dart-sass #8228
Conversation
|
DCO Assistant Lite bot All contributors have signed the DCO. |
|
I have read the DCO document and I hereby sign the DCO. |
|
Deploy preview for carbon-elements ready! Built with commit 2619e8b |
|
Deploy preview for carbon-components-react ready! Built without sensitive environment variables with commit 2619e8b https://deploy-preview-8228--carbon-components-react.netlify.app |
| }, | ||
| sourceMap: true, | ||
| }, | ||
| }; | ||
|
|
||
| const fastSassLoader = { | ||
| loader: 'fast-sass-loader', | ||
| loader: require.resolve('fast-sass-loader'), |
There was a problem hiding this comment.
I don't think we should be using and recommending fast-sass-loader as we begin to embrace sass modules. It doesn't support @use, but beyond that, the ways it achieves a faster compile time don't seem to match with how @use is intended to work with @import and vice-versa.
In addition to namespacing, there are a few important differences between
@useand@import:
@useonly executes a stylesheet and includes its CSS once, no matter how many times that stylesheet is used.@useonly makes names available in the current stylesheet, as opposed to globally.- Members whose names begin with
-or_are private to the current stylesheet with@use.- If a stylesheet includes
@extend, that extension is only applied to stylesheets it imports, not stylesheets that import it.
fast-sass-loader uses regex to grab all the imports and hoist them all into one file. It then caches the result to avoid recompiling. I tried patching it locally to modify the regex to include @use, but when it passes the result off to sass it still can't resolve files, even the built in modules fail. If the resolution problem was fixed, I don't think we'd see appropriate namespacing control as everything would essentially be global. I suspect it would also impact usages of @forward.
To my eye, @use sort of turns the value proposition of the library upside down. Especially after @joshblack saw compile times significantly improve with sass modules, #7525
There was a problem hiding this comment.
I pushed a commit to remove fast-sass-loader and use sass-loader in both dev and prod. I tested a fresh storybook dev (yarn start) and see an increase in build time:
fast-sass-loader |
sass-loader |
|---|---|
| 42s for manager | 51s for manager |
| 53s for preview | 1min 46s for preview |
Recompile time should be less. I also think we'll see a decrease when using sass modules? Also it might help if we imported component-level modules per *-story.js file instead of including all styles globally for the whole storybook?
I'm didn't add the fiber optimization as it's been deprecated for some time. It was updated yesterday noting it's incompatible with Node v16.
All in, I think this should be ready to review now. The deploy preview should show the CSS Grid styles in the Auto Columns story as an example. Once approved I can remove the example story.
|
bump @andreancardona @emyarod when you get a chance to review! @tay1orjones just pushed up updates and moved this from draft. |
Working locally I'm seeing quite a slowdown in both initial startup, as well as any style updates. It's taking me ~50s for style changes to appear in the storybook when running locally. Anything we can do to speed up dev speed? |
Update storybook config and loader dependencies to use dart-sass, enables use of sass modules in the storybook.
Changelog
Changed
Removed
fast-sass-loaderTesting / Reviewing
The deploy preview should show the CSS Grid styles in the Auto Columns story as an example. Once approved I can remove the example story.