Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sass: / is deprecated for division #4096

Closed
tetchel opened this issue May 25, 2021 · 5 comments · Fixed by #4164
Closed

sass: / is deprecated for division #4096

tetchel opened this issue May 25, 2021 · 5 comments · Fixed by #4164
Assignees

Comments

@tetchel
Copy link

tetchel commented May 25, 2021

https://github.com/sass/dart-sass/releases/tag/1.33.0

https://sass-lang.com/documentation/breaking-changes/slash-div

If sass version 1.33.0 or newer is installed, the console is absolutely flooded by "/ is deprecated for division" errors when patternfly builds.

@redallen
Copy link
Contributor

We're still using node-sass instead of dart-sass. I'll start working on upgrading it today.

@redallen
Copy link
Contributor

Unfortunately adding @use "sass:math"; ... math.div might break the following node-sass SASS consumers:

  • 3Scale
  • Foreman-Console
  • Foreman-Tools
  • Insights-Curiosity
  • Kiali-App
  • Kogito
  • Konveyor-Shared-Library
  • Migration-Analytics
  • Migration-Toolkit-for-Containers
  • OpenShift-Admin-Console (will break for sure)
  • Operatorhub
  • Quay
  • RHEL-Composer
  • Red-Hat-Developers
  • Settings-Catalog
  • Settings-RBAC

For this reason we've opted not to replace node-sass with dart-sass but instead find a clever workaround for the division operator.

We want to support dart-sass whenever our next breaking change release is. In the meantime we encourage consumers to adopt the sass package instead of node-sass using the migrator tool.

@garrett
Copy link

garrett commented Jun 10, 2021

We're looking to switch to Dart Sass in the Cockpit project as well and are hit by this too. After looking at this a bit, I have some ideas.

PatternFly side

One idea I had: Changing all division to multiplication instead:

$foo / 2$foo * .5
$bar / 100$bar * .01

And so on. But it's a bunch of busywork (but some of these easier cases should be scriptable)... and what happens to something like $foo / $bar?

In most cases, the division could be replace with a in-browser calc() too.

So switching to multiplication for the easy cases and wrapping the rest in calc() (or any other CSS function that supports math, such as min(), max(), clamp(), abs()) might be a good way forward for PatternFly, to support both LibSass and Sass Dart (and projects that use either).

Project side

For the time being, from Cockpit's perspective, we're wondering if we should:

  1. do the migration from LibSass to Dart Sass
  2. make sure to lock Dart Sass to < 2 (as this is when inline division is expected to go away). Specifically, we'll probably stick to Dart Sass transpiled to JavaScript at version ~1.32.0 as 1.36 introduces the division deprecation warning noise (and there doesn't seem to be a way to turn that off for now).
    • note: LibSass (and Ruby Sass) are deprecated, so the current version of Dart Sass is still going to be better than the other variants
  3. switch to a multiplication for, use calc(), or precompute the value ourselves (basically the suggestions above)
  4. ignore the division errors from PatternFly

This way, we could use Dart Sass and PatternFly while we wait for a future-Dart-Sass-friendlier PatternFly. Once PF is ready for Dart 2.x, we could then also remove our lock and switch to Dart 2.x.

We're looking into this. Other teams might want to as well... and we're happy to share the outcome, especially if this works.


Ideally, the Sass team would add sass::math with math.div() to LibSass for ease porting pain, so we wouldn't be in this mess. But they haven't, and there aren't any indications (and I'm aware of) that they will.

@nex3
Copy link

nex3 commented Jun 10, 2021

Sass team lead here! I wanted to pop in and say, first of all, that we're very sorry for the friction here. We really wanted this transition to be smoother: we actually accepted the proposal for the /-as-division deprecation two years ago, and filed a P1 issue with LibSass to support it around the same time. We held off landing it in Dart Sass for so long because of how much easier warning-free support for both Dart Sass and LibSass would have been if it were possible to support both platforms.

Unfortunately, the LibSass team (which works basically independently of the rest of the Sass team) has been steadily losing bandwidth over the past five years, and they just don't have the ability to add new features anymore. This is why we eventually declared LibSass deprecated and, six months afterwards, started the process of releasing the /-as-division deprecation in LibSass.

I think you're on the right track by encouraging your users to migrate to Dart Sass ASAP. It's likely that staying on LibSass will continue to produce more and more pain, between incompatibilities with changes to the Sass language on one hand and lack of support for new CSS features on the other. But I also understand not wanting to break your existing users if you can help it. To that end, here's a workaround you can use to continue supporting both Dart Sass and LibSass for the time being while also silencing these warnings:

// _div.import.scss
@forward "sass:math" show div;
// _div.scss
@function div($number1, $number2) {
  @return $number1/$number2;
}

If you include both these files in your library, you can write @import "div" and get a div() function that will work on LibSass and avoid producing errors on Dart Sass. It relies on the fact that Dart Sass supports the new module system and LibSass does not, so Dart Sass will prefer files with the extension .import.scss. If you want to support older versions of Dart Sass as well (>=1.23.0), you could also dynamically check whether math.div is defined:

// _div.import.scss
@use "sass:map";
@use "sass:math";
@use "sass:meta";

$-use-math-div: map.has-key(meta.module-functions("math"), "div");

@function div($number1, $number2) {
  @return if($-use-math-div, math.div($number1, $number2), $number1/$number2);
}

I wouldn't recommend either of these as long-term solutions, but they can at least get you unpinned from earlier Dart Sass versions until we land support for the silencing dependency warnings in JS.

allisonkarlitskaya added a commit to allisonkarlitskaya/cockpit that referenced this issue Jun 11, 2021
This partially reverts commit 4063e3d,
dropping our custom loader to call the sassc commandline tool, and
bringing back sass-loader, and then makes further changes.

We are now using Dart Sass (which we already had in our package.json for
some reason).  Pin the version to strictly less than 1.33 to avoid an
annoying warning about division (which we don't have a lot of control
over, since most cases of its are coming from PatternFly, who are
currently deciding what to do about this issue, themselves).

See patternfly/patternfly#4096 for some more
discussion.
@mcoker mcoker added this to the Breakaway Sprint (2021.09) milestone Jun 11, 2021
allisonkarlitskaya added a commit to allisonkarlitskaya/cockpit that referenced this issue Jun 11, 2021
This partially reverts commit 4063e3d,
dropping our custom loader to call the sassc commandline tool, and
bringing back sass-loader, and then makes further changes.

We are now using Dart Sass (which we already had in our package.json for
some reason).  Pin the version to strictly less than 1.33 to avoid an
annoying warning about division (which we don't have a lot of control
over, since most cases of its are coming from PatternFly, who are
currently deciding what to do about this issue, themselves).

See patternfly/patternfly#4096 for some more
discussion.
martinpitt pushed a commit to cockpit-project/cockpit that referenced this issue Jun 14, 2021
This partially reverts commit 4063e3d,
dropping our custom loader to call the sassc commandline tool, and
bringing back sass-loader, and then makes further changes.

We are now using Dart Sass (which we already had in our package.json for
some reason).  Pin the version to strictly less than 1.33 to avoid an
annoying warning about division (which we don't have a lot of control
over, since most cases of its are coming from PatternFly, who are
currently deciding what to do about this issue, themselves).

See patternfly/patternfly#4096 for some more
discussion.
@redallen redallen self-assigned this Jun 17, 2021
@patternfly-build
Copy link

🎉 This issue has been resolved in version 4.115.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants