`mixed_decls` produces too much noise for developers with mixins

Mister-Hope commented 1 month ago

With 1.77.7 release, my projects have warnings everywhere. I understand this breaking change with nested selectors, but with mixins, this change can be too BREAKING to migrate through a patch.

For example, I write a wrapper mixin and use it in a lot of places:

@mixin wrapper($pad: hope-config.$pad, $mobile: hope-config.$mobile) {
  max-width: var(--content-width, 740px);
  margin: 0 auto;
  padding: 2rem 2.5rem;

  @media print {
    max-width: unset;
  }

  @media (max-width: $pad) {
    padding: 1.5rem;
  }

  @media (max-width: $mobile) {
    padding: 1rem 1.5rem;
  }
}

Now, every usage of this wrapper is throwing warnings unless it's a bare @include, because the @include is placed at the top of the page (for overriding), and any declarations after it will trigger warning:

.class {
 @include wrapper;

 // any declarations will get a warning:
 background: white;
}

I am not satisfied with the dart team about this changes because:

mixins is not something that standard CSS file have, so it's still good to let users choose the previous behavior for it.
people will likely to place complicated rules inside mixins (that's mixins for), while they could be annoying bypassing this change

I will suggest the following things:

revert this change with 1.77.8 and release at least a minor (1.78.0)

This is the thing I couldn't stand the most, why should ANY package release any deprecations with patch??????
Hide the warnings for mixins (can provide the behavior opt-in but at least with an option)

There could be too much work for complicated styles with a lot of mixin used. I would suggest the previous behavior, but it seems that there will still be issues with v2.

Edited: Explanations and solutions from the below comments:

Dart team member admitted that it's not good to release deprecations with patch, and they say a future deprecation release will be minor🎉.
Currently this is just a warning with codes that will be inflected by new behaviors. There is no functionality change at the time.
This change's goal is aligning CSS behaviors (as mentioned in the docs), and mixins are influenced by it.
The activation of the change probably won't break any styles, as nesting selectors should have higher priority in most cases.

Solutions to get rid of the warnings:

If you think your codes are fine with the new behavior, you can bypass this warnings with 2 possible solutions:

opt-in the new behavior in codes
silence this new deprecation and wait until the new behavior is activated by a newer version

For the first one, there is a clear guideline in docs: https://sass-lang.com/documentation/breaking-changes/mixed-decls/

For the second one:

If you are using the API from sass directly:

sass.compile(content, {
 silenceDeprecations: ['mixed-decls']
})

If you are using webpack, you should also ensure api is set to modern or modern-compiler

{
 test: /\.(sa|sc|c)ss$/,
 use: [
   {
     loader: 'sass-loader',
     options: {
       api: 'modern',
       sassOptions: {
         silenceDeprecations: ['mixed-decls']
       }
     }
   }
 ]
}

If you are using Vite, use a custom logger to filter these warnings:

export default {
 css: {
   preprocessorOptions: {
     sass: {
       logger: {
         warn: (message, { deprecation, deprecationType }) => {
           if (deprecation && deprecationType.id === "mixed-decls") return;

           console.warn(message);
         },
       },
     },
     scss: {
       logger: {
         warn: (message, { deprecation, deprecationType }) => {
           if (deprecation && deprecationType.id === "mixed-decls") return;

           console.warn(message);
         },
       },
     },
   },
 },
};

Mister-Hope commented 1 month ago

I again examine the patch change.

It seems that the patch just reflects a new warning and behaves the old way, in this case I strongly recommend to provide a option to silence the warning with (all files / only mixins). No one is expecting his log to contain 100+ warnings which can not be silenced

Also, I think dart team should reconsider, what's the best behavior for mixins.

daniluk4000 commented 1 month ago

mixins is not something that standard CSS file have

They will though. But specification doesn't tell if we can rewrite rules after @apply. If we can, then this warning is even missleading.

maksymmamontov commented 1 month ago

fully agree with @Mister-Hope in here and don't see any particular reason this breaking change was introduced in the first place. Developers who expect full compatibility with native CSS will just use CSS without any preprocessors.

Mister-Hope commented 1 month ago

fully agree with @Mister-Hope in here and don't see any particular reason this breaking change was introduced in the first place. Developers who expect full compatibility with native CSS will just use CSS without any preprocessors.

It's ok to add such hint to help people migrate to the future v2 smoothly, but the biggest thing is that sass not only support nesting selectors, but also mixins, and the latter is a huge problems to all users.

Mister-Hope commented 1 month ago

Moreover, the deprecation message is too long (probably 20+ lines each), and with 100+ warnings, you can hardly fine other warnings and errors in console when integrating into a vite/webpack project.

Imagine previously your build log is clean with only <50 lines, and now it's more than 2000 lines. That is super annoying.

For me I am building tools like vuepress and vuepress-theme-hope (both >2k stars), I am not an end-point user which I can pin the deps easily. Downstream users are suffering this with my current code and I can only give them instructions pinning sass to an earlier version through advanced solutions like pnpm.overrides

ntkme commented 1 month ago

It seems that the patch just reflects a new warning and behaves the old way, in this case I strongly recommend to provide a option to silence the warning with (all files / only mixins).

These warnings can be silenced with { silenceDeprecations: ['mixed-decls'] } option: https://sass-lang.com/documentation/js-api/interfaces/options/#silenceDeprecations https://sass-lang.com/documentation/js-api/interfaces/deprecations/#mixed_decls

kotsius commented 1 month ago

The use of declarations after nested rules is currently deprecated in Sass, in order to notify users of the upcoming change and give them time to make their stylesheets compatible with it. In a future release, Dart Sass will change to match the ordering produced by plain CSS nesting.

Unfortunately, I could find no assurances that the "upcoming change" or the "future release" will only arrive with v2. If that is the case, it has better been made explicit. Alternatively, a few therapy recommendations would be fitting.

Mister-Hope commented 1 month ago

It seems that the patch just reflects a new warning and behaves the old way, in this case I strongly recommend to provide a option to silence the warning with (all files / only mixins).

These warnings can be silenced with { silenceDeprecations: 'mixed_decls' } option: https://sass-lang.com/documentation/js-api/interfaces/options/#silenceDeprecations https://sass-lang.com/documentation/js-api/interfaces/deprecations/#mixed_decls

This could be a nice workaround at the moment, but further improvement about mixins should still be discussed :)

nex3 commented 1 month ago

Let me address a few different issues here:

When and where should we debate this change?

If you want to debate upcoming changes to Sass, you need to do so when those changes are out for public review, not after they are approved and released in the language. We take great pains to make sure users have the chance to have a say in the design process of the language, but you have to actually participate in that process if you want your voice to be heard. This proposal was out for review from 30 April until 18 June, and we made public announcements on X and Mastodon soliciting comments.

We will very occasionally make changes to a feature after it's approved and landed, but the burden of proof for making adjustments is much higher. If you want a say in how Sass evolves, you need to participate in the discussions as they're happening, not come in with complaints after the fact.

Okay but is this change necessary?

Yes. It is a core design principle of Sass that any valid plain CSS that appears in a Sass file should have the same meaning as it does in plain CSS. Even if this particular application of that principle annoys, I guarantee that the principle as a whole is necessary to make Sass approachable and comprehensible. The moment a user can't rely on standard CSS semantics, they have to second-guess everything in a Sass file, which makes it impossible to learn or use with any confidence.

The use of mixins here is not an exception to this point. Mixins may not be in CSS yet, but they are transparent to CSS: writing styles in a mixin that you include should always behave the same as writing those styles directly. This is called referential transparency and is another core design principle (not just of Sass but of most programming languages). If moving a chunk of re-used styles into a mixin caused other declarations to be ordered differently, refactorings that should be safe start to have unpredictable and dangerous side-effects. That's something we have to avoid.

But shouldn't it be a bigger version bump?

It's generally our philosophy that it's valuable to release deprecation warnings as soon as possible. The role of a deprecation warning is to give developers time to prepare for a breaking change, and the more time you have to prepare the better. This means we don't want to hold them back for major version bumps.

That said, looking over the semver spec it does say:

Minor version Y (x.Y.z | x > 0) ... MUST be incremented if any public API functionality is marked as deprecated.

...so in keeping with that we will in future release new deprecations as minor version bumps rather than patch bumps.

The warnings are too noisy!

As @ntkme points out, you can silence specific deprecation warnings in Sass. If you're confident that the new ordering won't affect your styles, go ahead and silence this warning and you won't ever need to think about it again.

Unless you pass verbose: true or --verbose, Sass will only emit a given type of deprecation warning at most five times before silencing any future instances. However, if you're using Sass's JS API, it can only track these deprecations within a single stylesheet compilation. If you're using a framework like Vue, I recommend requesting that they implement a custom logger with the same behavior to limit the amount of deprecation noise by default in that circumstance as well.

When will the breaking change happen?

Dart Sass's compatibility policy states:

There is one exception where breaking changes may be made outside of a major version revision. It is occasionally the case that CSS adds a feature that's incompatible with existing Sass syntax in some way. Because Sass is committed to full CSS compatibility, we occasionally need to break compatibility with old Sass code in order to remain compatible with CSS.

In these cases, we will first release a version of Sass that emits deprecation warnings for any stylesheets whose behavior will change. Then, at least three months after the release of a version with these deprecation warnings, we will release a minor version with the breaking change to the Sass language semantics.

As such, the release that removes these deprecations and begins emitting interleaved declarations in-place will be no earlier than 9 October 2024. I can't give a more specific date than that because the details depend on developer bandwidth and team prioritization which is always difficult to predict.

Mister-Hope commented 1 month ago

From your message, I understand your team do great work try to make some comming changes to everyone before landing. But definitely there are a lot of developers like me are relying a lot of packages, so I believe there are a lot of People like me, which only have time to review major changes with some common packages we use while they are in beta.

As @ntkme points out, you can silence specific deprecation warnings in Sass. If you're confident that the new ordering won't affect your styles, go ahead and silence this warning and you won't ever need to think about it again.

So if I understand everything well:

whether the changes are added, this deprecation message will keep exisiting, our downstream users may be annoyed by such kind of deprecations, And silence these kind of warnings from our side may shadow some warnings which they actually need to take attention in their code

Then unless developers choose to add & { } around declarations inside all mixins and around those declarations who are below under such mixins (Sounds like a huge work to a bunch of developers), this cannot be solved in any future version of sass right?

nex3 commented 1 month ago

In an upcoming version of Sass, the default will change and mixed declarations will be emitted in the order they were written, rather than being hoisted to the top of the style rule. See https://sass-lang.com/d/mixed-decls for an example of the difference. This change is unlikely to cause style breakages because nested rules generally have higher specificity anyway, but it's not impossible so we need to release a deprecation warning in order to give developers time to prepare.

I understand that this is annoying. It would be much simpler for everyone—including me—if CSS kept the original semantics for mixed declarations, which matched Sass's. But we have to deal with the world as it is, and given our core design principles there's really only one path forward.

Mister-Hope commented 1 month ago

You are not answering my question directly. Thanks for the reply, and your reply answered most of my questions and I believe should solve others concern.

But, one more thing that should be important to most developers.

In the new version are the current issues I am reporting doing the new way without warnings, or I still must opt-in & {} in a lot of places to avoid that warning?

If this is the former way, then we can tell users to hide this warning in the next coming three or maybe more months, But it is the latter way, I think I still need to take time to add the &{ }.

This is confusing me because the standard css does not require &{} But meanwhile it is reasonable for sass To require it for hints

msev commented 1 month ago

It seems that the patch just reflects a new warning and behaves the old way, in this case I strongly recommend to provide a option to silence the warning with (all files / only mixins).

These warnings can be silenced with { silenceDeprecations: ['mixed_decls'] } option: https://sass-lang.com/documentation/js-api/interfaces/options/#silenceDeprecations https://sass-lang.com/documentation/js-api/interfaces/deprecations/#mixed_decls

I can't get this option to work, here's an extract from my webpack config:

{
  test: /\.(sa|sc|c)ss$/,
  use: [
    {
      loader: 'sass-loader',
      options: {
        implementation: require('sass'),
        sassOptions: {
          silenceDeprecations: ['mixed_decls']
        }
      }
    }
  ]
}

ntkme commented 1 month ago

https://webpack.js.org/loaders/sass-loader/#api

You have to use modern or modern-compiler api to use the depreciation options.

msev commented 1 month ago

https://webpack.js.org/loaders/sass-loader/#api

You have to use modern or modern-compiler api to use the depreciation options.

Thanks @ntkme!

It's mixed-decls instead of mixed_decls

nex3 commented 1 month ago

You are not answering my question directly. Thanks for the reply, and your reply answered most of my questions and I believe should solve others concern.

But, one more thing that should be important to most developers.

In the new version are the current issues I am reporting doing the new way without warnings, or I still must opt-in & {} in a lot of places to avoid that warning?

If this is the former way, then we can tell users to hide this warning in the next coming three or maybe more months, But it is the latter way, I think I still need to take time to add the &{ }.

This is confusing me because the standard css does not require &{} But meanwhile it is reasonable for sass To require it for hints

Once the breaking change is released, there will be no need to write & {}. Mixed declarations will be compiled in-order by default, as shown in the examples in https://sass-lang.com/d/mixed-decls.

It's standard when producing a deprecation message to provide a way of writing styles that will work the same in both older versions and newer (in this case upcoming) versions of the language. That's what & {} does: it allows you to opt into the new behavior no matter what version you (or your users) are using.

The reason plain CSS doesn't have a similar migration plan is that plain CSS nesting is new enough that there's a negligible corpus of existing users who might be broken by this change. The same is not true for Sass, which has had the existing nesting semantics for almost two decades.

Mister-Hope commented 1 month ago

Hi, @nex3, webpack solution should be fine as sass-loader can opt-in to use the new compile API.

I was wondering if there is any workaround to silence the warnings in Vite, see https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/css.ts#L2148. Vite is still using an old one, and this issue just leaves as is for quite a long time: https://github.com/vitejs/vite/issues/14689.

So does it mean telling users to fix sass to 1.77.6 is the only way for downstream developers before anyone pickup https://github.com/vitejs/vite/issues/7116

maksymmamontov commented 1 month ago

Okay but is this change necessary?

Yes. It is a core design principle of Sass that any valid plain CSS that appears in a Sass file should have the same meaning as it does in plain CSS. Even if this particular application of that principle annoys, I guarantee that the principle as a whole is necessary to make Sass approachable and comprehensible. The moment a user can't rely on standard CSS semantics, they have to second-guess everything in a Sass file, which makes it impossible to learn or use with any confidence.

The main issue in here is if native CSS is structured incorrectly (and in this case it's still quite questionable!) it won't break the whole website builds.

Mister-Hope commented 1 month ago

I edited my first comment and add a conclusion, and I hope it helps newcomers to quickly review what is going on here. @nex3 If would be great if you can check if there are issues in it.

nex3 commented 1 month ago

I was wondering if there is any workaround to silence the warnings in Vite, see https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/css.ts#L2148. Vite is still using an old one, and this issue just leaves as is for quite a long time: vitejs/vite#14689.

So does it mean telling users to fix sass to 1.77.6 is the only way for downstream developers before anyone pickup vitejs/vite#7116

The modern JS API has been available for two and a half years, so it's disappointing to see major frameworks still not supporting it.

You can also silence specific warnings by implementing a custom logger, which is supported in the legacy API. You can build in logic to such a logger to filter out specific deprecation warnings, including this one.

maksymmamontov commented 1 month ago

@nex3 could you please reopen this thread as the issue is still ongoing and quite active and hasn't been resolved yet. it couldn't be just closed as minor release of SASS introduced totally Breaking Changes that are linked to multiple other projects.

ntkme commented 1 month ago

Totally Breaking Changes

While deprecation warnings are annoying, they are not breaking change. They are just telling you to be prepared for an upcoming breaking change. As of now, the output behavior has not been changed. It's really up to the frameworks authors and end users to either update the sass code or just silence the warnings.

Introducing a new deprecation in a patch release was a mistake by semantic versioning definition, but honestly I don't think the situation will be any better if it was properly introduced in a minor release as most of the users have dependencies locked down at major version. In my opinion, the most frustrating part of the issue is that sass already provided updated infrastructure in new JS API with deprecation API for fine-grained control of deprecation warnings, but the web bundlers make it difficult (webpack) or impossible (vite) to be used. The gap in user experience between standalone sass and third party integration of sass is problematic, and need to be addressed by those bundlers.

Meanwhile, I do think there are things we can improve for the deprecation process. Lots of people complained about communications despite this change has been announced via multiple channels ahead of time. Here are some ideas to make it better:

We have vendor tests for the most popular sass frameworks in the CI which currently is only run during the release. We should run those for all the PRs as well. When we're preparing for new deprecation, we can check which major frameworks are affected, and open GitHub issues on their repositories ahead of time as an early notice, in hope that they can publish a fix before the deprecation is rolled out.
We can consider publish edge snapshot builds of sass to npm to allow testing of upcoming changes or unreleased feature, that frameworks like bootstrap can setup nightly CI runs to check against sass edge snapshot automatically by configuring all deprecations to be fatal. To make it work, we also have to change sass spec process publish edge snapshot for spec change early on and give time for testing edge snapshot before changes are actually released.

nex3 commented 1 month ago

I like those ideas, but I'm concerned that they might take a lot of effort from us in exchange for potentially minimal value. Given how slow most downstream users seem to be to make changes even when they are directly contacted (see Vite's 2.5-year-old issue to use the new compiler API), it's not clear to me that a test-ahead scheme would actually make much of a difference.

On the other hand, it does come at a certain degree of cost. Not only would it consume precious bandwidth from our already-stretched-thin team, it also means it'll take longer for users who can directly update their stylesheets to see the deprecation warnings, which means that either they'll have less time to update or the new deprecation-blocked features will take longer to land—both of which are user-negative.

All that is to say: if any frameworks are interested in proactively working with us to avoid deprecation warnings, I'm definitely interested in developing a process for that, but I'm not optimistic about something fully driven from our end.

(It is worth noting that we don't necessarily need to publish snapshot builds; we already have the technology for releasing "future deprecations" which users can opt into on demand.)

ntkme commented 1 month ago

It is worth noting that we don't necessarily need to publish snapshot builds; we already have the technology for releasing "future deprecations" which users can opt into on demand.

Maybe for all the new deprecations we are going to add, they should be announced and released as "future deprecations" for at least one minor release cycle (or some fixed time) before they are activated, even for the fast-tracking deprecations. - That is to say, no deprecations will be activated immediately at release. Then it's up to framework developers to monitor sass announcements and test new future deprecations before they are activated for the end users, given us a chance to reduce direct impact on end users.

nex3 commented 1 month ago

I'd be all for that if we had any indication that even a few frameworks would actually monitor them. But I don't know that such a thing is realistic, given how downstream consumers have historically ignored any changes that didn't print visible warnings on the command line.

kemenaran commented 3 weeks ago

It seems that, in the specific case of mixins, it is quite painful to write code that doesn't trigger the deprecation warning.

For instance, this code:

.alert-warning
  @include alert
  @include warning        
  background-color: yellow

must be rewritten (as far as I understand) as below, in order not to trigger the deprecation warning:

.alert-warning
  @include alert
  &
    @include warning         
    &
      background-color: yellow

A question : would it be possible for sass not to trigger this warning in case of mixins?

Or to have a separate deprecation warning switch for mixins, so that the mixins case can be silenced/ignored?

nex3 commented 3 weeks ago

@kemenaran The recommended way to handle those mixins is to update them to use & {} internally, and then to write:

.alert-warning
  background-color: yellow
  @include alert
  @include warning

As I explained above, we can't exempt mixins from this because mixins will be included in the eventual change to output order. And separating out a different deprecation name would just make more annoying for users to silence all deprecations in the common case that that's what they want.

maksymmamontov commented 3 weeks ago

@nex3 could you please just think about this small example in terms of how much code had to be refactored across billions of projects because of this change that is forced by a preprocessor that still compiles everything in CSS....

nex3 commented 3 weeks ago

@maksymmamontov I'm sympathetic to that, but again our hands are tied here for the most part. We can't just diverge from CSS here; it may not seem like a big deal right now when CSS nesting is new and not yet widely used, but in five or ten years when developers have thoroughly internalized CSS's semantics as the logical way for nesting to work it would be a huge problem for Sass to work differently. Really, if you're unhappy with this breaking change, you should take it up with the CSS working group—we're just following their lead.

kotsius commented 2 weeks ago

@kemenaran The recommended way to handle those mixins is to update them to use & {} internally, and then to write:
.alert-warning
  background-color: yellow
  @include alert
  @include warning
As I explained above, we can't exempt mixins from this because mixins will be included in the eventual change to output order. And separating out a different deprecation name would just make more annoying for users to silence all deprecations in the common case that that's what they want.

This approach works, I was able to silence several hundred warnings without adding a single &{…} block. However, every change I made had to do with the placement of root declarations relative to nested blocks. Even when the nested blocks contained a '&', the order of these blocks relative to sibling blocks did not seem to affect the deprecation warnings at all. I now assume that eliminating all warnings makes my SCSS fully ready for the Dart Sass adaptation to plain CSS nesting.

BenceSzalai commented 1 week ago

I've just faced this issue now and thinking about from the perspective of how it ~is~ will be affecting the effective result of the compilation. Because I'm afraid it'll force us to write code that produces larger CSS output, and that is something I find concerning.

Let's say I have this code:

@mixin form_input {
  color: red;
  &:disabled {
    color: gray;
  }
}

textarea.large.green {
  height: 300px;
  @include form_input;
  color: green;
}

If I understand correctly it produces this CSS:

textarea.large.green {
  height: 300px;
  color: red;
  color: green;
}
textarea.large.green:disabled {
  color: gray;
}

So here color is repeated, but that's a small overhead for the convenience of writing my code this way.

My problem is that right now the above code says: use all the default styles from my mixin, but override color to green. If I change my usage to:

textarea.large.green {
  height: 300px;
  color: green;
  @include form_input;
}

It no longer works, because color: red; from the mixin comes later and would override color: green.:

textarea.large.green {
  height: 300px;
  color: green;
  color: red;
}
textarea.large.green:disabled {
  color: gray;
}

But if I leave it the way it is (aside from the warnings until this change goes live) I am now producing a CSS that not only duplicates the color declaration (which I admin is a suboptimality in the original code too), but now it duplicates the whole selector.:

textarea.large.green {
  height: 300px;
  color: red;
}
textarea.large.green:disabled {
  color: gray;
}
textarea.large.green {
  color: green;
}

Indeed this is a fictive example, but I have some mixins that are quite large, they define many styles and I want to occasionally override some of those. If the selector is also long (not like in the example above), which can happen, than I'm bloating my final CSS with the long selector being repeated, which can be significantly more useless overhead in terms of css size than just overriding a property.

Another thing I quite liked and used 1000s of times in my codebases is to put overrides beside what is being overridden for improved readability. This naturally resulted in code where styles are interleaved with subselector override styles and media queries. This is a huge benefit as opposed to CSS, that I can see every potential override for a style together. This becomes pretty much impossible now, unless I'm willing to have CSS output that repeats the same selector over and over again. S.g. like this:

.some-element {
  color: red;
  &.green {
    color: green;
  }
  &.blue {
    color: blue;
  }

  background-color: white;
  &:hover {
    background-color: black;
  }
  &:disabled {
    background-color: gray;
  }

  min-width: 800px;
  @include for-tablet {
    min-width: 400px;
  }
  @include for-mobile {
    min-width: unset;
  }
}

Very easy to see certain aspects defined together. But now I have to rewrite these to:

.some-element {
  color: red;
  background-color: white;
  min-width: 800px;

  &.green {
    color: green;
  }
  &.blue {
    color: blue;
  }

  &:hover {
    background-color: black;
  }
  &:disabled {
    background-color: gray;
  }

  @include for-tablet {
    min-width: 400px;
  }
  @include for-mobile {
    min-width: unset;
  }
}

By looking at the top of the code I am no longer aware that min-width has overrides. It is a huge disadvantage compared to how I could organize my code so far.

I understand that the window to discuss this change has passed. Fair enough. Although I think it is a bit week argument as I'd suggest that the vast majority of SASS users would not be actively involved with development channels, so it is a bit like in The Hitchhiker’s Guide to the Galaxy:

All the planning charts and demolition orders have been on display at your local planning department in Alpha Centauri for 50 of your Earth years, so you’ve had plenty of time to lodge any formal complaint and it’s far too late to start making a fuss about it now.

Anyway, But I'm not here to complain, instead I'd like to suggest to introduce a syntax that keeps the previous way of working. Just like now the warnings can be avoided by explicitly opting for the declaration order output by using & { }, there could be another syntax that would mean:

Please don't apply this rule in a repeated declaration, but instead hoist it to the top of the selector just like it was in the legacy behaviour.

This would not contradict the principle of having any valid CSS compile to identically if copied into an SCSS file, as this syntax would be non-existent in CSS.

I'm not a language designer, but it could be similar to @at-root { ... }, e.g. @hoist { }. This way I could keep the logical order in my SCSS files which is a huge improvement on readability compared to being forced to follow particular order for declarations if I want to keep my output file size reasonable.

So I could still write code with related aspects in one place:

.some-element {
  color: red;
  &.green {
    color: green;
  }
  &.blue {
    color: blue;
  }

  @hoist {
    background-color: white;
  }
  &:hover {
    background-color: black;
  }
  &:disabled {
    background-color: gray;
  }

  @hoist {
    min-width: 800px;
  }
  @include for-tablet {
    min-width: 400px;
  }
  @include for-mobile {
    min-width: unset;
  }
}

without producing repeated selector in the output.

The concept needs a bit of thinking through for mixins still, as should the whole mixin be wrapped in @hoist or if the @hoist should be used inside the mixin body, but I'm sure there is a way to make the current behaviour still possible with additional syntax. Because the current behaviour offers far superior flexibility which should be kept and not sacrificed as long as there's a way to keep it.

EDIT: I've just realised that the warning text says To keep the existing behavior, move the declaration above the nested rule. which is objectively untrue for many of the cases I've showed above, as moving those rules to the top changes meaning and breaks code behaviour. How this message was conceived?

nex3 commented 1 week ago

I sympathize with you. I know this change feels disruptive, and if I had my way CSS would continue to match Sass's behavior and we wouldn't have to make it. But that's not the world we live in, and we have to make the best of this one.

The burden of evidence necessary to add a new language feature is very large, and it's almost never something we do just to save a few bytes in the output (especially since repeated byte sequences are already very amenable to gzip compression).

If you're concerned about output size, I recommend looking into the possibility of a post-processing step that combines style rules if it's provably safe to do so. For example,

textarea.large.green {
  height: 300px;
  color: red;
}
textarea.large.green:disabled {
  color: gray;
}
textarea.large.green {
  color: green;
}

is provably identical to

textarea.large.green {
  height: 300px;
  color: green;
}
textarea.large.green:disabled {
  color: gray;
}

because the second style rule has higher specificity than the third anyway. cssnano might be a good place to look for or request a feature like that.

EDIT: I've just realised that the warning text says To keep the existing behavior, move the declaration above the nested rule. which is objectively untrue for many of the cases I've showed above, as moving those rules to the top changes meaning and breaks code behaviour. How this message was conceived?

This isn't "objectively untrue". In your first example, "above the nested rule" means immediately before &:disabled {. The fact that this is in a mixin and the declaration is not makes that a bit tricky, but it's still the best way to preserve the old behavior.

webdevnerdstuff commented 1 week ago

Is it just me or does it feel like CSS is getting dumber in it's process to get "smarter"? I have found Sass/SCSS far superior for many years, and now I feel like Sass is having to dumb itself down because CSS wants to try to reinvent the wheel because they fell way behind in the game and are still eating Sass' dust.

BenceSzalai commented 1 week ago

@nex3 Thank you for your continued support regarding these changes. I really appreciate that you put real thought into the concerns raised, albeit having certain hard limits on what is possible in terms of solutions and/or workarounds.

Sure, adding a language feature is not done by a random suggestion in a closed issue. I was just trying to tests the waters if anyone else sees any value in the idea at all.

But I can see how a dedicated tool could be better at solving these issues. I assume those probably could also eliminate repeated properties overriding each other, such as color: green; color: red; under the same selector in my example. SO if one more tool can fix it and bring also additional benefit, that's probably good enough to live with.

Just for fun I'd also note that data transfer size of css is not the only concern, but the time/cpu it takes the browser to match the selectors. I admit I don't know if browsers would first consolidate subsequent identical selectors before starting the matching against the DOM, but the DOM matching procedure can be in some cases a concern. I know that with the powerful hardware we have today it is fine, but Chromium have just introduced a new developer tool to analyse the performance of selector matching, which suggest it is a legit concern for some. I try to believe the code I make uses the least amount of CPU both for speed and for CO2 footprint, so for me it is certainly an interesting aspect. I assume most devs probably don't care at all and their attitude might be the healthier one...

This isn't "objectively untrue". In your first example, "above the nested rule" means immediately before &:disabled {. The fact that this is in a mixin and the declaration is not makes that a bit tricky, but it's still the best way to preserve the old behavior.

"But is it tho? Is it tho?" :P

Imho "a bit tricky, but it's still the best way" and "keep the existing behavior" are still two much different claims. I don't say what you said is not reasonable, and i think all said in this topic is respectable, but I keep up that the statement is untrue and is a bit too generous assuming everyone's usecase will just be fine, all needed is just reshuffle some lines here and there. I have mixins used in mixins used in mixins..., with overrides on multiple levels. I don't know, I might be crazy and my code may be one big bowl of mess, but for me it was superior encapsulation and advanced usage of the available language features. So far SASS was powerful enough to handle it, and now it is not any more.

I literally have no way to keep the existing behaviour. Impossible. Hence the statement is untrue. And it downplays the impact of this change, which is somewhat lacking approach in terms of sincerity.

Again, I'm not here to fight. I just like to say things as they seem to me. Still, I appreciate your attendance to this issue, and thaks for your suggestion of alternatives. I'll look into them.

EDIT: Checked cssnano, it cannot fix it, because it can only merge identical selectors if they are adjacent. Which makes sense because merging them otherwise could be breaking depending on what is in between and how that relates to the DOM. So when SASS outputs these properties in declaration order is basically a lossy transformation and from looking at the output it is no longer possible to determine if the rules are separated for a valid reason or not. So merging them can be an issue. It can only be decided on the source level if the merging is desired and should be done or not. So a @hoist { } feature would make sense, because that is the only way to keep the ability for stylesheet authors to define the styles in any order they like, as it was possible so far. Or SASS is loosing this feature for good. Which again - i know I'm like a broken record - is a huge step back for my codebase and I don't think I'm making very special things. Overriding mixins is I'd assume a very common usage pattern.

Also regarding introducing new language feature: the good news is that the implementation is already there: the current behaviour. So it is not like it'd be something very challenging to implement... I know that's not the key issue, but still. It just feels like a shame to throw away all those logic capable of so much and replace with inferior solution without giving users a way to keep using it...

Anyway. I'm going trough the stages of grief. Will stop spamming here eventually at some point....

EDIT2: Although I am willing to put effort into a proposal for @hoist { } or similar feature that would create a path to keep using SASS in more advanced ways than CSS spec restricts it to.

cascornelissen commented 1 week ago

FWIW, I want to echo @BenceSzalai's concerns. We're running into similar issues on due to this change on a codebase with 25 repositories with about 300 occurrences. We're not quite sure what the correct way forward would be. To give some context, we have a setup where we use Sass mixins like follows (simplified):

// block.scss
@use '~sass-mq/mq';

@mixin horizontal-whitespace() {
    margin: 1em;

    @include mq.mq() {
        margin: 0.5em;
    }
}

@mixin vertical-whitespace() {
    margin: 2em;

    @include mq.mq() {
        margin: 1em;
    }
}

.selector {
    @include horizontal-whitespace();
    @include vertical-whitespace();
    margin-top: 1rem;
}

The only correct way to handle this, seems to be the following? Is this really the right/best way to do this?

.selector {
    & {
        @include horizontal-whitespace();
    }

    & {
        @include vertical-whitespace();
    }

    & {
        margin-top: 1rem;
    }
}

BenceSzalai commented 1 week ago

The only correct way

Which still results in the .selector selector 5 times in the output as opposed to the previous 2, which is the real minimum number needed to cover the 2 sets of stylings: one without a media query and one with a media query.

nex3 commented 1 week ago

@cascornelissen Here's how I'd recommend writing that for forwards-compatibility:

// block.scss
@use '~sass-mq/mq';

@mixin horizontal-whitespace() {
    & {margin: 1em}

    @include mq.mq() {
        margin: 0.5em;
    }
}

@mixin vertical-whitespace() {
    & {margin: 2em}

    @include mq.mq() {
        margin: 1em;
    }
}

@use 'block' as *;

.selector {
    margin-top: 1rem;
    @include horizontal-whitespace();
    @include vertical-whitespace();
}

Specifically:

Declarations directly within mixins should be wrapped in & {} (for now) if there's any chance that the mixins will be included after nested rules (which will be the case for most mixins). It's better to write this once, in the mixin, rather than adding it as a wrapper around the mixin everywhere it's called.
Prefer moving declarations before mixins to wrapping them in & {}. This preserves the existing behavior, and won't need to be removed later once we're CSS-compatible.

We're not going to add a @hoist-style rule unless CSS itself does. The use-cases within Sass here are essentially identical to those within plain CSS, and adding more unique at-rules is a high cost that we don't want to bear without correspondingly high value. In this case, it would still require a universal migration from users to an output that's not clearly better (the only benefit is reduced file size which will eventually be a non-issue).

cascornelissen commented 1 week ago

@nex3, thanks so much for the detailed write-up, much appreciated! 🫶🏼

I ended up simplifying the example a bit more so that I could play around with the suggestions in the playground, see this link. The only part that wasn't working with just these changes was the possibility to overwrite whatever was being defined inside the mixin. But, as can be seen in this link, that's still possible with the wrapping of the subsequent rules inside another & {} block. I'll try out these changes in some of our codebases later this week.

sass / dart-sass