twbs / bootstrap-sass

Official Sass port of Bootstrap 2 and 3.
http://getbootstrap.com/css/#sass
MIT License
12.59k stars 3.53k forks source link

Handle Sass 1.33 and math.div #1221

Closed AprilArcus closed 2 years ago

AprilArcus commented 2 years ago

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 commented 2 years ago

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

glebm commented 2 years ago

@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 commented 2 years ago

@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 :neutral_face:

AprilArcus commented 2 years ago

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.

AprilArcus commented 2 years ago

Looks great! I'd love to see this merged and released, @glebm šŸ™

still need to address some bugs noted upthread

AprilArcus commented 2 years ago

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

glebm commented 2 years ago

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

AprilArcus commented 2 years ago

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 commented 2 years ago

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

glebm commented 2 years ago

Published 3.4.2 to npm only

ashimg commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

It looks like dart-sass was added to angular-devkit/build-angular in https://github.com/angular/angular-cli/pull/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. (https://github.com/angular/angular-cli/pull/21455, release notes).

ashimg commented 2 years ago

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 commented 2 years ago

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

ashimg commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

If you are able to upgrade to it, version 12.13 of @angular-devkit/build-angular includes sass 1.34.0 thanks to https://github.com/angular/angular-cli/pull/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 commented 2 years ago

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 commented 2 years ago

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

vinhspm commented 2 years ago

@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