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

Fix sass division deprecation #17940

Closed
wants to merge 2 commits into from
Closed

Fix sass division deprecation #17940

wants to merge 2 commits into from

Conversation

rafaelmaeuer
Copy link

I understand that:

  • I'm submitting this PR for reference only. It shows an example of what I'd like to see changed but
    I understand that it will not be merged and I will not be listed as a contributor on this project.

This PR fixes the outstanding deprecation of / division which will be removed in Dart Sass 2.0.0.
Building this package with webpack gives the following warning when using sass ^1.33.0:

DEPRECATION WARNING: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div(20em, 16)

More info and automated migrator: https://sass-lang.com/d/slash-div

   ╷
12 │ $fa-fw-width:          (20em / 16);
   │                         ^^^^^^^^^
   ╵
    node_modules/@fortawesome/fontawesome-free/scss/_variables.scss 12:25  @import
    node_modules/@fortawesome/fontawesome-free/scss/fontawesome.scss 5:9   @import
    style/fontawesome.scss 2:9                                             root stylesheet

DEPRECATION WARNING: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div(4em, 3)

More info and automated migrator: https://sass-lang.com/d/slash-div

  ╷
6 │   font-size: (4em / 3);
  │               ^^^^^^^
  ╵
    node_modules/@fortawesome/fontawesome-free/scss/_larger.scss 6:15     @import
    node_modules/@fortawesome/fontawesome-free/scss/fontawesome.scss 8:9  @import
    style/fontawesome.scss 2:9                                            root stylesheet

DEPRECATION WARNING: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div(3em, 4)

More info and automated migrator: https://sass-lang.com/d/slash-div

  ╷
7 │   line-height: (3em / 4);
  │                 ^^^^^^^
  ╵
    node_modules/@fortawesome/fontawesome-free/scss/_larger.scss 7:17     @import
    node_modules/@fortawesome/fontawesome-free/scss/fontawesome.scss 8:9  @import
    style/fontawesome.scss 2:9                                            root stylesheet

DEPRECATION WARNING: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div($fa-li-width * 5, 4)

More info and automated migrator: https://sass-lang.com/d/slash-div

  ╷
6 │   margin-left: $fa-li-width * 5/4;
  │                ^^^^^^^^^^^^^^^^^^
  ╵
    node_modules/@fortawesome/fontawesome-free/scss/_list.scss 6:16        @import
    node_modules/@fortawesome/fontawesome-free/scss/fontawesome.scss 10:9  @import
    style/fontawesome.scss 2:9                                             root stylesheet

The changes made in this PR conform to the sass deprecation guide:
https://sass-lang.com/documentation/breaking-changes/slash-div

@tagliala
Copy link
Member

Thanks. Can't merge, this should be fixed upstream

@tagliala
Copy link
Member

btw this change will affect node-sass compatibility (and break a lot of setups), ref: twbs/bootstrap#34051

For the moment, I suggest this: twbs/bootstrap#34051 (comment)

@robmadole
Copy link
Member

Thanks @rafaelmaeuer for the report and PR. But as @tagliala we'll have to consider this one more carefully. We want to support both node-sass and sass (Dart version). So this will take some careful thought. We're working on it.

@niektenhoopen
Copy link

@robmadole Isn't node-sass deprecated? (source: https://www.npmjs.com/package/node-sass => https://sass-lang.com/blog/libsass-is-deprecated)

@wunc
Copy link

wunc commented Jun 9, 2021

@rafaelmaeuer @robmadole I think changing the division to multiplication by decimals would also solve the deprecation while maintaining node-sass compatibility, i.e. no need to use the dart-sass specific math.div() function?

For example, just change line-height: (3em / 4) to line-height: (3em * 0.25)

@rafaelmaeuer
Copy link
Author

@wunc thanks, this is a really nice suggestion, I do prefer multiplication instead of division in general. I will update my PR regardingly.

@robmadole
Copy link
Member

@rafaelmaeuer no need to update the PR. The files for this repository are built using something else. We can't merge your changes.

@nurdism
Copy link

nurdism commented Jun 11, 2021

Could take the same approach angular took by adding a polyfill, something like this:

@function private-div($a, $b) {
  @if (meta.function-exists('div', 'math')) {
    @return math.div($a, $b);
  }
  @else {
    @return $a / $b;
  }
}

angular/components@a4043f4#diff-4b0d3692fae1e26970f5365a78db4ff81acb362f79ef12252081df8b285d38e8

@tagliala
Copy link
Member

Could take the same approach angular took by adding a polyfill, something like this:

Does this work on libsass and ruby sass? I've tried on https://www.sassmeister.com/ against libsass 3.5.5 and it doesn't

@nurdism
Copy link

nurdism commented Jun 11, 2021

Could take the same approach angular took by adding a polyfill, something like this:

Does this work on libsass and ruby sass? I've tried on https://www.sassmeister.com/ against libsass 3.5.5 and it doesn't

hmmm your right, it doesn't work with libsass, looks like its a dartsass polyfill.

But libsass is deprecated, that being said, updating to dartsass is not hard at all, it should be a 1 - 1 API and most major loaders/packages support dartsass now. It may break some setups, but thats what comes with updating packages. Why support libsass when its been deprecated sense October (and ruby-sass in March of 2019).

@tagliala
Copy link
Member

@nurdism yes, we know, but we would like to preserve the compatibility.

I would go for multiplications instead of divisions. In the upstream repository we have some complex mixins, but it looks to me that the output scss only contains 3 divisions

The only problematic division may be 4/3, but for the moment I would personally prefer to write 1.3333333333 rather than dropping libsass support

@tagliala
Copy link
Member

The Bootstrap Team eventually re-implemented the division: twbs/bootstrap#34245

I think it is too much for FA's specific use case (4 / 3)

@tagliala tagliala mentioned this pull request Jun 25, 2021
7 tasks
@illdave
Copy link

illdave commented Jul 7, 2021

Thanks. Can't merge, this should be fixed upstream

For those of us older and apparently not hip enough developers, what does "fixed upstream" mean?

@tagliala
Copy link
Member

tagliala commented Jul 8, 2021

Hi @illdave , this repository contains the code produced in a private repo responsible of building both Font Awesome Free and Pro.

What you see here is the result of a building process and any changes done to this repo will be lost, so we need to fix this issue in another repository

@fxsalazar
Copy link

Could we have at least the flag activated to reduce the noise in the logs ? #17940 (comment)

@rafaelmaeuer
Copy link
Author

Just curious if there is any progress here in the meanwhile? Is there already an ETA for a fix in upstream?

@tagliala
Copy link
Member

Hi,

Just curious if there is any progress here in the meanwhile?

Nope sorry

Is there already an ETA for a fix in upstream?

No ETA yet, all the efforts are directed towards FA6 at the moment

@DB-CL
Copy link

DB-CL commented Jul 31, 2021

It may be useful to some people. You can patch the files on the fly with @wunc idea (multiplication instead of division).

  • on the root of your project create a patches folder
  • put a file named @fortawesome+fontawesome-free+5.15.3.patch inside this folder
  • insert this content into this file :
diff --git a/node_modules/@fortawesome/fontawesome-free/scss/_larger.scss b/node_modules/@fortawesome/fontawesome-free/scss/_larger.scss
index 27c2ad5..540612f 100644
--- a/node_modules/@fortawesome/fontawesome-free/scss/_larger.scss
+++ b/node_modules/@fortawesome/fontawesome-free/scss/_larger.scss
@@ -3,8 +3,8 @@
 
 // makes the font 33% larger relative to the icon container
 .#{$fa-css-prefix}-lg {
-  font-size: (4em / 3);
-  line-height: (3em / 4);
+  font-size: (4em * 0.3333333);
+  line-height: (3em * 0.25);
   vertical-align: -.0667em;
 }
 
diff --git a/node_modules/@fortawesome/fontawesome-free/scss/_list.scss b/node_modules/@fortawesome/fontawesome-free/scss/_list.scss
index 8ebf333..f78c1ca 100644
--- a/node_modules/@fortawesome/fontawesome-free/scss/_list.scss
+++ b/node_modules/@fortawesome/fontawesome-free/scss/_list.scss
@@ -3,7 +3,7 @@
 
 .#{$fa-css-prefix}-ul {
   list-style-type: none;
-  margin-left: $fa-li-width * 5/4;
+  margin-left: $fa-li-width * 1.25;
   padding-left: 0;
 
   > li { position: relative; }
diff --git a/node_modules/@fortawesome/fontawesome-free/scss/_variables.scss b/node_modules/@fortawesome/fontawesome-free/scss/_variables.scss
index 68a0750..2150342 100644
--- a/node_modules/@fortawesome/fontawesome-free/scss/_variables.scss
+++ b/node_modules/@fortawesome/fontawesome-free/scss/_variables.scss
@@ -9,7 +9,7 @@ $fa-version:           "5.15.3" !default;
 $fa-border-color:      #eee !default;
 $fa-inverse:           #fff !default;
 $fa-li-width:          2em !default;
-$fa-fw-width:          (20em / 16);
+$fa-fw-width:          (20em * 0.0625);
 $fa-primary-opacity:   1 !default;
 $fa-secondary-opacity: .4 !default;
  • and finally in your package.json add "postinstall": "npx patch-package" under the "scripts" section

This will automatically patch the files after a npm install and the deprecation warnings will disappear.

This is a temporary solution.

@rafaelmaeuer
Copy link
Author

rafaelmaeuer commented Aug 4, 2021

as for me copy & paste didn't work, here are the steps to patch it yourself:

  • install patch-package: yarn add patch-package --dev yarn add patch-package
  • add postinstall in package.json scripts: "postinstall": "yarn patch-package" "postinstall": "patch-package"
  • replace divisions by multiplications like Fix sass division deprecation #17940 (comment)
  • create patch: patch-package @fortawesome/fontawesome-free
  • apply changes: yarn

Edit: I replaced commands when you want to use patch-package with docker in prod builds
(read https://github.com/ds300/patch-package#dev-only-patches)

@mletoullec
Copy link

I'm sorry, but 1/3 is not worth 0.3333333.

You must use math.div (1, 3) instead

font-size: (4em / 3);

font-size: math.div(4em, 3);

@tagliala
Copy link
Member

tagliala commented Aug 9, 2021

@mletoullec you can use math.div if you don't need libsass support

Otherwise, you can use 1.3333333333em directly or * 0.33333333333 (eleven 3s), since dart-sass is limited to 10 digits of precision and the output will be the same as 4em / 3

@robmadole
Copy link
Member

Alright, folks. We are going to follow Bootstrap on this one and implement a divide function. We'll get this into 6 and 5. It will still support libsass (and all the other older implementations) and be dart-sass 2.0 compatible.

@illdave
Copy link

illdave commented Aug 18, 2021 via email

@kgrammer
Copy link

kgrammer commented Sep 15, 2021

Could we have at least the flag activated to reduce the noise in the logs ? #17940 (comment)

This is an older question, but for anyone still struggling with the deprecation noise, with Dart Sass you can add the "--quiet" option and the deprecation warning will be suppressed.

@realtebo
Copy link

Please backport fixes to FA 4
A lot of old themese in production builds cannot be upgraded and are Fa4 -based

@jasonlundien
Copy link
Member

A fix was released with 6.0.0-beta2.

@bobbyg603
Copy link

Alright, folks. We are going to follow Bootstrap on this one and implement a divide function. We'll get this into 6 and 5. It will still support libsass (and all the other older implementations) and be dart-sass 2.0 compatible.

Thanks for adding this to the 6.0.0-beta2 release! Are there still plans to add this to 5? @jasonlundien @robmadole

@robmadole
Copy link
Member

@bobbyg603 we will release this for 5 but right now we are hyper-focused on 6. So I don't have a schedule for a 5 release at the moment.

@bobbyg603
Copy link

@robmadole totally understand, thanks for the heads up!

@jcaianirh
Copy link

Thanks for adding this to the 6.0.0-beta2 release! Are there still plans to add this to 5? @jasonlundien @robmadole

+1 for seeing this backported to 5 @robmadole

@V-iktor
Copy link

V-iktor commented Dec 10, 2022

SassError: Undefined operation "2em * fa-divide(5, 4)"

Using nextjs default sass compiler, any ideas?

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

Successfully merging this pull request may close these issues.