sass / dart-sass

The reference implementation of Sass, written in Dart.
https://sass-lang.com/dart-sass
MIT License
3.9k stars 352 forks source link

False negatives with mixed declarations, nested CSS #2334

Closed saulyz closed 1 week ago

saulyz commented 1 week ago

Just after upgrading to ^1.77.7 we've got deprecation messages warnings that suddenly got massive. The same code base has been around for more than 1 year and is applied in a high traffic prod site.

The example notices are

Deprecation Warning: Sass's behavior for declarations that appear after nested
rules will be changing to match the behavior specified by CSS in an upcoming
version. To keep the existing behavior, move the declaration above the nested
rule. To opt into the new behavior, wrap the declaration in `& {}`.

More info: https://sass-lang.com/d/mixed-decls

  ┌──> ../le-grand-k2/src/styles/base/responsive-typography.scss
139│           #{$prop}: $val_mobile;
  │           ^^^^^^^^^^^^^^^^^^^^^ declaration
  ╵
  ┌──> ../le-grand-k2/src/styles/base/media-queries.scss
5 │ ┌     @media (min-width: $point_val) {
6 │ │         @content;
7 │ │     }
  │ └─── nested rule
  ╵

    ../le-grand-k2/src/styles/base/responsive-typography.scss 139:9  -responsive-rules-by-token()
    ../le-grand-k2/src/styles/base/responsive-typography.scss 191:9  styles-regular()
    ../le-grand-k2/src/styles/base/responsive-typography.scss 42:3   output-styles()
    ../le-grand-k2/src/styles/base/typography.scss 9:5               @content
    ../le-grand-k2/src/styles/base/global-styles-control.scss 8:9    body-wrap-restricted()
    ../le-grand-k2/src/styles/base/typography.scss 7:1               @use
    ../le-grand-k2/src/styles/base/_index.scss 6:1                   @use
    style.scss 12:1                                                  root stylesheet

Warning: 173 repetitive deprecation warnings omitted.
Run in verbose mode to see all warnings.

The issue part with "declaration" is in the file responsive-typography.scss

@mixin _responsive-rules-by-token($token, $prop) {
    $val_mobile: brand.get-var(typography-mobile-#{$token}-#{$prop});
    $val_tablet: brand.get-var(typography-tablet-#{$token}-#{$prop});
    $val_desktop: brand.get-var(typography-desktop-#{$token}-#{$prop});

    @if $prop == 'font-family' {
        $val_mobile: brand.get-var(typography-mobile-#{$token}-#{$prop}), var(--localized-font-fallback), var(--default-font-fallbacks);
        $val_tablet: brand.get-var(typography-tablet-#{$token}-#{$prop}), var(--localized-font-fallback), var(--default-font-fallbacks);
        $val_desktop: brand.get-var(typography-desktop-#{$token}-#{$prop}), var(--localized-font-fallback), var(--default-font-fallbacks);
    }

    @if _token-value-is-same-in-screens($token, $prop) {

        #{$prop}: $val_mobile;   // -- PROBLEM SPOT from the notice

    } @else if _token-value-is-different-in-screens($token, $prop) {

        @include media-queries.until(md) {
            #{$prop}: $val_mobile;
        }
        @include media-queries.between(md, lg) {
            #{$prop}: $val_tablet;
        }
        @include media-queries.from(lg) {
            #{$prop}: $val_desktop;
        }

    } @else {
        // this relies on the logic of possible overlaps

        @if $val_mobile == $val_tablet and $val_tablet != $val_desktop {
            @include media-queries.until(lg) {
                // mobile extends to tablet
                #{$prop}: $val_mobile;
            }
            @include media-queries.from(lg) {
                #{$prop}: $val_desktop;
            }
        }
        @if $val_mobile != $val_tablet and $val_tablet == $val_desktop {
            @include media-queries.until(md) {
                #{$prop}: $val_mobile;
            }
            @include media-queries.from(md) {
                // tablet extends to desktop
                #{$prop}: $val_tablet;
            }
        }

    }
}

Since this is purely a mixin I do not find any relevance with the documentation.

The next part is "nested rule" in the file media-queries.scss

@use '../setup/breakpoints';

@mixin from($name, $breakpoints: breakpoints.$width__breakpoints) {
    $point_val: map-get($breakpoints, $name);
    @media (min-width: $point_val) {
        @content;
    }
}

@mixin until($name, $breakpoints: breakpoints.$width__breakpoints) {
    $point_val: map-get($breakpoints, $name);
    @media (max-width: $point_val - 1px) {
        @content;
    }
}

@mixin between($lower, $upper, $breakpoints: breakpoints.$width__breakpoints) {
    $from_point_val: map-get($breakpoints, $lower);
    $until_point_val: map-get($breakpoints, $upper);

    @if $from_point_val != null and $until_point_val != null {
        @media (min-width: $from_point_val) and (max-width: $until_point_val - 1px) {
            @content;
        }
    } @else if $until_point_val == null {
        @include breakpoint-from($lower, $breakpoints) {
            @content;
        }
    } @else if $from_point_val == null {
        @include breakpoint-until($upper, $breakpoints) {
            @content;
        }
    }
}

This breaking change is new to me, I might not fully understand its impact to the large extent, but the spots in my utility mixins does not sound relevant to anything that is mentioned in documentation.

nex3 commented 1 week ago

Whether the declarations come from a mixin or not is irrelevant—they'll still end up emitted in a different order afte rthe breaking change lands. This is particularly relevant for media queries, because they don't change the specificity of the rules in question, so order is the only way to distinguish their precedence.

I'm going to close this out. If you have a specific behavior you'd like changed, feel free to follow up, but I believe this warning is legitimate and you should consider wrapping #{$prop}: $val_mobile in & {} to check whether its styling with change in the future.