Repository navigation
Pass the modern style Sass option instead of the legacy outputStyle - #1528
Merged
Merged
Conversation
Kocal
approved these changes
Aug 1, 2026
stof
reviewed
Aug 6, 2026
| outputStyle: 'expanded', | ||
| // CSS minification is handled by the CSS minimizer, not by Sass. | ||
| // `style` is the modern Sass API option, the legacy `outputStyle` | ||
| // is ignored by sass-loader. |
Member
There was a problem hiding this comment.
The part about outputStyle should be removed. A comment in the code is meant to explain the code, not the diff producing it (explaining the diff is the job of the commit message)
Contributor
Author
There was a problem hiding this comment.
Good point, done — trimmed the comment down to just what the code does, leaving the diff rationale to the commit message.
`outputStyle` belongs to the legacy Sass JS API. sass-loader 16 and 17 only support the modern API, where that option is named `style`, so the value was silently ignored and sass-loader applied its own production default of `style: compressed` instead. Sass therefore minified the CSS itself in production, which is exactly what the comment says Encore leaves to the CSS minimizer, and it made Dart Sass prefix bundles containing non-ASCII characters with a UTF-8 BOM. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
X-Coder264
force-pushed
the
fix-sass-modern-style-option
branch
from
August 6, 2026 16:52
28aa8a2 to
cb808c1
Compare
stof
approved these changes
Aug 6, 2026
Member
|
Thanks @X-Coder264 ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
lib/loaders/sass.tspassessassOptions: { outputStyle: 'expanded' }, butoutputStylebelongs to the legacy Sass JS API (render()/renderSync()). sass-loader 16 and 17 — the range Encore declares (^16.0.1 || ^17.0.0) — only support the modern API, where the option is namedstyle, and unknown keys are ignored bycompileStringAsync(). Encore's value therefore had no effect and sass-loader's own production default applied instead (src/utils.js#L285-L288):So in
encore productionSass minified the CSS itself, which is what the comment (and #638 / #639) says Encore leaves to the CSS minimizer. This PR just uses the option name the modern API actually reads.Why it matters beyond output formatting
Dart Sass prepends a UTF-8 BOM (instead of
@charset "UTF-8") when compressed output contains non-ASCII characters — common in practice, e.g. any project importingbootstrap-sassglyphicons. Until postcss 8.5.23 that BOM was silently dropped whenenablePostCssLoader()was used; postcss 8.5.24 changed to "Preserve the BOM after the processing", so it now reaches the CSS minimizer, and lightningcss cannot parse a leading BOM (parcel-bundler/lightningcss#338) — it lexes U+FEFF as an ident, soBOM+ license comment + first selector fails withUnexpected token Ident("html"). That breaksconfigureCssMinimizerPlugin((options, MinimizerPlugin) => { options.minify = MinimizerPlugin.lightningCssMinify; }), whichUPGRADE.mdrecommends as the Encore 7 replacement for the removed default CSS minifier.With this change Sass emits
@charset "UTF-8";instead of the BOM, which every CSS minifier parses:Behaviour change worth flagging
Projects that have not opted into CSS minification will now genuinely ship unminified CSS from Sass entries in production, where Sass used to compress it for them by accident. That matches what
UPGRADE.mdalready documents for 7.0 ("CSS minification is no longer enabled by default … otherwise CSS is not minified in production"), so I have treated it as the bug rather than as intended behaviour — but tell me if you would like aCHANGELOG.mdentry and/or anUPGRADE.mdnote and I will add it.Two things I deliberately left out, happy to add either if you disagree:
charset: false— projects that want a minifier to keep leading/*! … */license banners need it, because lightningcss only preserves loud comments at the very start of the file and@charsetnow sits in front of them. That felt like a user-side choice rather than an Encore default, since it drops the encoding declaration for everyone.outputStyle— since sass-loader 16 dropped the legacy API, there is no supported configuration in which the old name did anything, so there is nothing to deprecate.Tests
test/loaders/sass.js— the existing options-callback assertion is updated, andgetLoaders() basic usagenow asserts the defaultsassOptionsexplicitly, so a legacy option name cannot silently come back.test/functional.js— new production-mode test asserting Sass does not minify the CSS itself (h2 {survives in the emitted asset).pnpm vitest run test/loaders/sass.js,pnpm type-check,pnpm lintandpnpm fmt:checkall pass locally. I could not runtest/functional.jsin my environment (no Chrome for the PuppeteerbeforeAll), so I verified that test's expectation by building the fixture through the patched Encore directly:h2 { … }expanded before this change wash2{…}.