Repository navigation
refactor(sass): replace the deprecated if() function with @if and or - #670
Draft
desig9stein wants to merge 1 commit into
Draft
desig9stein wants to merge 1 commit into
desig9stein wants to merge 1 commit into
Conversation
- Every conditional in sass/ and the themes spec is an @if/@else block or an `or`/`and` expression, so Sass 1.95+ prints no if-function warning and older Sass still compiles. The compiled CSS does not change. - _optional-shades, _default-variant and bem--not-selector replace the repeated nested if() calls. - The bem separators are `!default`, so `@use ... with (...)` can set them. - sass-embedded is 1.107.1 in theming, mcp and preview, and tests/deprecations.test.ts fails the suite on any deprecation warning. Closes #641.
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.
Closes #641.
Draft for review of the approach proposed in #641 (comment).
Problem
Sass 1.95+ deprecates the legacy
if()function, so every consumer compile on a current Sass prints[if-function]warnings fromsass/. Compiling the e2e theme with sass-embedded 1.107.1 on master prints 74 of them. The repo pinssass-embedded ~1.92.1, which predates the deprecation, so CI never saw them.The new
if(sass($c): $a; else: $b)syntax is not an option while Prettier formatssass/**in CI:Undefined operation "(21,) < 1.07").;that follows a)or a string.(background,)into(background)in 36 untouched files, which changes their Sass type.Change
Sass
sass/is an@if/@elseblock or anor/andexpression. Noif()call is left, including in the tile-manager theme from feat(tile-manager): add theme and MCP metadata #667._optional-shades(),_default-variant()andbem--not-selector()replace the nestedif()calls that repeated the same branch.$bem--sep-elem,$bem--sep-modand$bem--sep-mod-valare!default. Before, they readmeta.variable-exists(), which is always false inside a module, so@use ... with (...)could not set them. They keep the same values.Dependencies
sass-embeddedis~1.107.1inpackages/theming,packages/mcpandpackages/preview. In theming and mcp it is a runtime dependency, because the MCP server compiles CSS with it, so consumers install 1.107.1 instead of 1.92.1.Tests
tests/deprecations.test.tscompiles the e2e theme with a logger and fails on any deprecation warning.tests/_themes.spec.scssuses an_fallback()helper instead of the 16 copies of the sameif().Tests
npm run lint: 0 errors.npm run build: passes with 0 deprecation warnings.npm test: 42 files, 1522 tests pass.deprecations.test.tsfails on master'ssass/with 74[if-function]warnings and passes on this branch.The compiled output does not change. Each comparison is byte-identical:
dist/json,dist/tailwind, e2etheme.css), master vs branch on the same compilerKnown limit
deprecations.test.tscompiles only the e2e theme, which covers one component theme. The all-themes compile above shows no deprecations today, but a future deprecation in a component theme outside the e2e theme would not fail CI. One option is to passfatalDeprecationsto the sass-true run intests/sass.test.ts, so every spec fails on a deprecation. I left that out to keep this PR to the migration.Notes
scss/at-if-no-parentheses --fixstrips the outer parentheses of@if ($a and $b) or ($c and $d), which leaves invalid Sass. The one mixed condition in_generator.scssstays unparenthesized, which followsandbinding tighter thanor.