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

Handle Sass 1.33 and math.div #1221

Merged
merged 2 commits into from Feb 26, 2022
Merged

Handle Sass 1.33 and math.div #1221

merged 2 commits into from Feb 26, 2022

Conversation

AprilArcus
Copy link
Contributor

@AprilArcus AprilArcus commented Oct 19, 2021

Since sass 1.33.0, the / operator is no longer supported. Instead, the new math.div() function must be imported via @use "sass:math";.

Many projects lack the resources to upgrade from Bootstrap 3.x, but nevertheless must maintain compatibility with current versions of the sass compiler in order to support other sass dependencies which are tracking the sass compiler's current feature set.

This PR introduces logic to convert the LESS / operator to the new sass math.div() notation. The usual perils of attempting to parse a parenthesis language with with regular expressions notwithstanding, this code appears to be sufficiently robust to serve future needs.

@cutterbl
Copy link

@glebm Any idea when this PR might get brought in? Very much an issue.

@glebm
Copy link
Member

glebm commented Dec 16, 2021

@AprilArcus This gem has a runtime dependency on sassc, which doesn't support math.div IIRC. Perhaps that needs to be updated here as well?

@glebm
Copy link
Member

glebm commented Dec 16, 2021

@glebm Any idea when this PR might get brought in? Very much an issue.

@cutterbl If it's an issue for you, thank @AprilArcus and use their branch for now 😐

@AprilArcus
Copy link
Contributor Author

This gem has a runtime dependency on sassc, which doesn't support math.div IIRC. Perhaps that needs to be updated here as well?

I wasn't aware that this package produces a Ruby gem as well as a node module. If support for the Ruby gem is a hard requirement in order to merge, then we may be blocked. https://sass-lang.com/ruby-sass states that "sassc gem is the most seamless way to move away from Ruby Sass", but sassc's own README.md states that both it and the libsass library which it wraps are deprecated. Are you aware of any ruby gems which wrap dart-sass? I found this relevant conversation at rubyonrails.org, but I confess that I've been out of the Ruby ecosystem for many years and I'm not even sure what criteria users of this gem would use to judge an ideal upgrade path.

@@ -136,7 +137,7 @@ mark,
// -------------------------

.page-header {
padding-bottom: (($line-height-computed / 2) - 1);
padding-bottom: math.div(($line-height-computed, 2) - 1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be padding-bottom: (math.div($line-height-computed, 2) - 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, christmas. I suppose I will need to write a proper parser after all.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AprilArcus Any plans to make the two fixes outlined by @aEvgenn ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at kikyous/bootstrap3-sass . Suitable for a temporary solution. There will be only one trouble - replacing the package name in the paths bootstrap-sass -> bootstrap3-sass

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AprilArcus Any plans to make the two fixes outlined by @aEvgenn ?

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

// Horizontal dividers
//
// Dividers (basically an hr) within dropdowns and nav lists

@mixin nav-divider($color: #e5e5e5) {
height: 1px;
margin: (($line-height-computed / 2) - 1) 0;
margin: math.div(($line-height-computed, 2) - 1) 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be margin: (math.div($line-height-computed, 2) - 1) 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link

@jagoncalves14 jagoncalves14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I'd love to see this merged and released, @glebm 🙏

@AprilArcus
Copy link
Contributor Author

Looks great! I'd love to see this merged and released, @glebm 🙏

still need to address some bugs noted upthread

@AprilArcus
Copy link
Contributor Author

AprilArcus commented Feb 22, 2022

Addressed issues noted by @aEvgenn. @glebm, what else needs to happen for this to be mergeable?

@AprilArcus AprilArcus force-pushed the sass-1.33 branch 2 times, most recently from 0b7b7c2 to dd38faa Compare February 22, 2022 21:39
@glebm
Copy link
Member

glebm commented Feb 26, 2022

The ruby gem is still an issue, because sassc does not support math.div

@AprilArcus
Copy link
Contributor Author

I understand that, but as you know short of writing a novel Gem to wrap dart-sass, there is nothing we can do for users of the Ruby gem here.

That fact notwithstanding, this work has value to developers in the NPM ecosystem. Can we publish this to NPM without updating the Gem?

@glebm
Copy link
Member

glebm commented Feb 26, 2022

Yeah, we probably could do this from a separate branch. I'll have a look.

@glebm glebm changed the base branch from master to sass-modules February 26, 2022 10:30
@glebm glebm merged commit 3c6cbc0 into twbs:sass-modules Feb 26, 2022
@glebm
Copy link
Member

glebm commented Feb 26, 2022

Published 3.4.2 to npm only

@@ -334,24 +335,24 @@ $grid-gutter-width: 30px !default;
//** Point at which the navbar becomes uncollapsed.
$grid-float-breakpoint: $screen-sm-min !default;
//** Point at which the navbar begins collapsing.
$grid-float-breakpoint-max: ($grid-float-breakpoint - 1) !default;
$grid-float-breakpoint-max: math.div($grid-float-breakpoint - 1) !default;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct?
I'm getting this error
(same thing @ line 351)

SassError: Missing argument $number2.
  ┌──> node_modules\bootstrap-sass\assets\stylesheets\bootstrap\_variables.scss
338│ $grid-float-breakpoint-max: math.div($grid-float-breakpoint - 1) !default;
  │                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ invocation
  ╵
  ┌──> sass:math
1 │ @function div($number1, $number2) {
  │           ━━━━━━━━━━━━━━━━━━━━━━━ declaration

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep looks wrong. @AprilArcus Can you please fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will address this right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: #1225

$navbar-padding-horizontal: floor(($grid-gutter-width / 2)) !default;
$navbar-padding-vertical: (($navbar-height - $line-height-computed) / 2) !default;
$navbar-padding-horizontal: floor(math.div($grid-gutter-width, 2)) !default;
$navbar-padding-vertical: (math.div($navbar-height - $line-height-computed), 2) !default;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$navbar-padding-vertical: (math.div($navbar-height - $line-height-computed), 2) !default;
$navbar-padding-vertical: math.div(($navbar-height - $line-height-computed), 2) !default;

// Navbar vertical align
//
// Vertically center elements in the navbar.
// Example: an element has a height of 30px, so write out `.navbar-vertical-align(30px);` to calculate the appropriate top margin.

@mixin navbar-vertical-align($element-height) {
margin-top: (($navbar-height - $element-height) / 2);
margin-bottom: (($navbar-height - $element-height) / 2);
margin-top: (math.div($navbar-height - $element-height), 2);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
margin-top: (math.div($navbar-height - $element-height), 2);
margin-top: math.div(($navbar-height - $element-height), 2);
margin-bottom: math.div(($navbar-height - $element-height), 2);

@AprilArcus AprilArcus mentioned this pull request Feb 28, 2022
akikoskinen added a commit to City-of-Helsinki/tunnistamo that referenced this pull request Mar 1, 2022
It's a good practice to lock dependency versions. But there's a specific
reason why this needs to be done now.

Tunnistamo isn't compatible with bootstrap-sass version 3.4.2. That
version switches [1] to a newish Sass syntax that isn't backwards
compatible [2]. Tunnistamo uses the django-sass-processor package to do
Sass preprocessing. That package uses the libsass library, which is
deprecated and will never support any newer Sass syntax [3]. As a
result, Tunnistamo is stuck with an old Sass syntax version unless the
preprocessor system is changed to something more modern.

In this commit the bootstrap-sass is locked to version 3.4.1 which still
uses the older syntax that libsass supports.

[1] twbs/bootstrap-sass#1221
[2] https://sass-lang.com/documentation/breaking-changes/slash-div
[3] https://sass-lang.com/blog/libsass-is-deprecated
@ashimg
Copy link

ashimg commented Mar 2, 2022

I am getting this error when using bootstrap-sass in angular application. This started happening right after the division fix was applied.

`Module build failed (from ./node_modules/sass-loader/lib/loader.js):

$navbar-padding-horizontal: floor(math.div($grid-gutter-width, 2)) !default;
^
Invalid CSS after "... floor(math": expected expression (e.g. 1px, bold), was ".div($grid-gutter-w"
in node_modules/bootstrap-sass/assets/stylesheets/bootstrap/_variables.scss (line 369, column 46)
ℹ 「wdm」: Failed to compile.`

How can this be resolved?

@AprilArcus
Copy link
Contributor Author

AprilArcus commented Mar 2, 2022

@ashimg Make sure you are using sass-loader 7.1.0 or later. Be sure that dart-sass (that is, the npm package called sass, not the npm package called node-sass) version 1.33 or later is listed in your package.json. Follow sass-loader's instructions to set the implementation option to dart-sass.

@ashimg
Copy link

ashimg commented Mar 8, 2022

Thank you @AprilArcus. For angular 8+, in order to use the sass-loader (dart-sass) you mentioned, is it necessary to have webpack? The way I am understanding the instructions, it assumes webpack.

@AprilArcus
Copy link
Contributor Author

AprilArcus commented Mar 8, 2022

Thank you @AprilArcus. For angular 8+, in order to use the sass-loader (dart-sass) you mentioned, is it necessary to have webpack? The way I am understanding the instructions, it assumes webpack.

Yes, sass-loader is a Webpack-specific loader. I inferred that you were using sass-loader and Webpack on the basis of your stack trace, which indicated an error emerged from sass-loader:

Module build failed (from ./node_modules/sass-loader/lib/loader.js):

It sounds like you aren't sure if you are in fact using Webpack. It may be the case that you are using a higher-level tool that wraps Webpack, such as Create React App or Next.js. If you tell me more about your build system, I can help you figure out how to configure it.

@ashimg
Copy link

ashimg commented Mar 9, 2022

@AprilArcus , yes you are correct. I was not sure about it as I did not find a webpack.config.js file in the angular project I am working with. I found out that it is wrapped within angular-devkit:
angular-devkit/build-angular@0.1001.7 --> webpack@4.44.1

There is a package.json file specifying the bootstrap-sass and other packages which get installed by npm install. There is no other sass package specified.
In angular.json file there is a reference to the bootstrap-sass script "./node_modules/bootstrap-sass/assets/javascripts/bootstrap.min.js",
However, the error stack trace is coming from _vendor.scss file which has:
$icon-font-path: '~bootstrap-sass/assets/fonts/bootstrap/';
@import '~bootstrap-sass/assets/stylesheets/bootstrap';
If I comment these two lines out the error stops, so it seems like the sass loader is getting invoked somewhere automatically.

Full error:
ERROR in Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Undefined function.

369 │ $navbar-padding-horizontal: floor(math.div($grid-gutter-width, 2)) !default;
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

node_modules/bootstrap-sass/assets/stylesheets/bootstrap/_variables.scss 369:42 @import
node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss 8:9 @import
src/assets/styles/_vendor.scss 2:9 @import
src/app/app.component.scss 2:9 root stylesheet

@AprilArcus
Copy link
Contributor Author

AprilArcus commented Mar 9, 2022

It looks like dart-sass was added to angular-devkit/build-angular in angular/angular-cli#13950. This change was first shipped in version 0.800.0 (release notes). This ought to be included in your existing version pin for angular-devkit/build-angular, which is later (0.1001.7), so clearly something isn't going the way it's supposed to.

If we zero in on the area where the sass implementation is actually selected in your version of angular-devkit/build-angular

let sassImplementation: {} | undefined;
try {
  // tslint:disable-next-line:no-implicit-dependencies
  sassImplementation = require('node-sass');
} catch {
  sassImplementation = require('sass');
}

we can see that it will first try to find node-sass, and only resort to dart-sass (i.e. sass) if it doesn't find it. "sass": "1.26.10" is required in the package.json for angular-devkit/build-angular@0.1001.7 and node-sass is not. Thus, it appears that node-sass is ending up in your node_modules somehow — perhaps unintentionally — and angular-devkit/build-angular is detecting it and preferring it to dart-sass.

I suggest running npm explain node-sass or yarn why node-sass to figure out which package is bringing in the rogue dependency, and then removing it.

Alternatively, if you are able to upgrade angular-devkit/build-angular to version 13.0.0 or greater, this version removes support for node-sass entirely. (angular/angular-cli#21455, release notes).

@ashimg
Copy link

ashimg commented Mar 14, 2022

Hi @AprilArcus , it seems a bit odd since I have the sass and not the node-sass according to the npm command.
(note: explain was not an option available for npm newer and older versions, npm list seems to be the one.)
npm list sass gives:
@angular-devkit/build-angular@0.1001.7. --> sass@1.26.10
npm list node-sass gives empty result.

Is the node-sass somehow coming from bootstrap-sass itself?
P.S: Currently unable to upgrade to devkit version >= 13 yet, so having to work something out in current version.

@AprilArcus
Copy link
Contributor Author

try require.resolve('node-sass') in the node REPL

@ashimg
Copy link

ashimg commented Mar 14, 2022

@AprilArcus

\>require.resolve('node-sass'):

Thrown: { Error: Cannot find module 'node-sass' at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15) at Function.resolve (internal/modules/cjs/helpers.js:33:19) code: 'MODULE_NOT_FOUND' }

\>require.resolve('sass')
'node_modules/sass/sass.dart.js'

\>require.resolve('bootstrap-sass'):
'node_modules/bootstrap-sass/assets/javascripts/bootstrap.js'

@AprilArcus
Copy link
Contributor Author

AprilArcus commented Mar 14, 2022

Okay. It looks like your setup is correctly choosing dart-sass over node-sass, but the version is too low. Try adding "sass": "^1.33.0" to your package.json.

@ashimg
Copy link

ashimg commented Mar 14, 2022

@AprilArcus it seems it still installs the lower version as part of angular-devkit and the newer sass separately as higher version. It then uses the lower version coming from angular-devkit itself as I still get the error related to floor(math.div())

# npm list sass
├─┬ @angular-devkit/build-angular@0.1001.7
│ └── sass@1.26.10
└── sass@1.49.9

@AprilArcus
Copy link
Contributor Author

AprilArcus commented Mar 16, 2022

If you are able to upgrade to it, version 12.13 of @angular-devkit/build-angular includes sass 1.34.0 thanks to angular/angular-cli#20844.

If not, and if you have Yarn or a recent version of NPM you can force your current version of @angular-devkit/build-angular to resolve sass to the version you have specified by adding a field to package.json.

for NPM ≥ 8.3.0, see https://docs.npmjs.com/cli/v8/configuring-npm/package-json#overrides. Note that NPM 8.3.0 first shipped with NodeJS 17.3.0, which is very recent.

  "overrides": {
    "sass": "1.33.0",
  }

for Yarn, see https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/ (Yarn 1.x) or https://yarnpkg.com/configuration/manifest#resolutions (Yarn 2.x)

  "resolutions": {
    "sass": "1.33.0",
  }

If you have neither NPM ≥ 8.3.0 nor Yarn available, you could consider using the package https://github.com/rogeriochaves/npm-force-resolutions instead.

@vinhspm
Copy link

vinhspm commented May 9, 2022

I'm getting this error while compiling scss with ruby sass 3.7.4, this happened when i tried to run "sass app.scss app.css"

Error: Invalid CSS after "...point-max: math": expected selector or at-rule, was ".div($grid-floa..."
on line 338 of node_modules/bootstrap-sass/assets/stylesheets/bootstrap/_variables.scss
from line 8 of node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss

_variables.scss:338
$grid-float-breakpoint-max: math.div($grid-float-breakpoint - 1) !default;

@AprilArcus
Copy link
Contributor Author

@vinhspm you should stick with Version 3.4.1 if you are compiling with Ruby Sass.

@vinhspm
Copy link

vinhspm commented May 11, 2022

@vinhspm you should stick with Version 3.4.1 if you are compiling with Ruby Sass.

yeah thank you i used that version and it worked

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.

None yet

8 participants