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

Compatibility with gulp-sass 4.1.1 broken with bootstrap-sass 3.4.3 and 3.4.2 #1226

Open
boenrobot opened this issue Feb 28, 2022 · 24 comments

Comments

@boenrobot
Copy link

boenrobot commented Feb 28, 2022

At the company I work for, we have a project that uses NodeJS to compile our assets with gulp. Specifically, gulp-sass 4.1.1.

With the release of 3.4.3, we got the following error on our builds.

[08:52:22] Starting 'styles'...
{ Error: vendor/bootstrap-sass-official/assets/stylesheets/bootstrap/_variables.scss
Error: Invalid CSS after "...     floor(math": expected expression (e.g. 1px, bold), was ".div($grid-gutter-w"
        on line 369 of vendor/bootstrap-sass-official/assets/stylesheets/bootstrap/_variables.scss
        from line 8 of vendor/bootstrap-sass-official/assets/stylesheets/_bootstrap.scss
        from line 43 of src/scss/main.scss
>> vbar-padding-horizontal:        floor(math.div($grid-gutter-width, 2)) !defa
   ------------------------------------------^

There is a newer gulp-sass version (5.0.0), but that one requires gulp 4 and Node 12, making that upgrade difficult (we're using gulp 3 and Node 10).

Regardless, breaking compatibility in a patch version is kind of a bad move, so I thought I'd report it. For now, we've just move back to 3.4.1 (as 3.4.2 seems was also affected, but I guess we hadn't updated since its release).

@boenrobot boenrobot changed the title Compatibility with gulp-sass 4.1.1 broken with bootstrap-sass 3.4.3 Compatibility with gulp-sass 4.1.1 broken with bootstrap-sass 3.4.3 and 3.4.2 Feb 28, 2022
@ptjuanramos
Copy link

I believe that this should be a breaking change and not being part of 3.4.x version.

@AprilArcus
Copy link
Contributor

AprilArcus commented Feb 28, 2022

@boenrobot, some thoughts:

1.) Gulp-sass 4.1.1 supports dart-sass. You can resolve your problem by changing the sass compiler. Remove "node-sass" from your package.json and replace it with "sass": "1.33.0 - 1.49.0". Then, in your Gulpfile.js:

var sass = require('gulp-sass');
sass.compiler = require('sass'); // was require('node-sass');

Note that dart-sass 1.33 is the first version with support for math.div, and 1.49 is the last version with support for NodeJS 10.x.

2.) Node 10 has been EOL since April 30, 2021; remaining on that version is not likely to be a viable long-term strategy for you.

@ptjuanramos You're right, but Bootstrap 4 is already its own thing, so it's not clear what the best way to signify this change would be.

@aaronbrooks-powerschool

We are getting the same issues. Not using gulp-sass. Node version 12.17.0. Would like to know how we could get this resolved. Same error message,
sass/assets/stylesheets/bootstrap/_variables.scss: Invalid CSS after "... floor(math": expected ")", was ".div($grid-gass/assets/stylesheets/bootstrap/_variables.scss: Invalid CSS after "... floor(math": expected ")", was ".div($grid-gutt..."utt..." ) Run with --trace to see the full backtrace

@AprilArcus
Copy link
Contributor

Not using gulp-sass.

You will need dart-sass (that is, the sass package on npm) version 1.33 or greater. I can help you if you'll tell me what build tools you use to compile sass. For example, do you use webpack with sass-loader? Something else?

@aaronbrooks-powerschool

We use Grunt, and bootstrap-sass installed from bower.json. We can look into updating our project structure, although it was jarring to see these older projects fail seemingly overnight. Seeing that version 3.4.1 did not have the latest changes merged in, we were able to use that and get our builds working again.

@AprilArcus
Copy link
Contributor

AprilArcus commented Feb 28, 2022

Grunt-sass supports dart-sass since version 3.0.0. If you want to upgrade, I suggest making sure you are on Grunt-sass 3.x, and passing dart-sass to grunt-sass's sass.options.implementation parameter.

it was jarring to see these older projects fail seemingly overnight

With respect to this, it's good practice to use a lockfile, such as package-lock.json (NPM) or yarn.lock (Yarn) to guard against unexpected updates to your dependencies.

@boenrobot
Copy link
Author

boenrobot commented Mar 1, 2022

@AprilArcus Thank you for the suggestion.

2.) Node 10 has been EOL since April 30, 2021; remaining on that version is not likely to be a viable long-term strategy for you.

I'm well aware, but it's not exactly my decision to make. Due to the huge changes between gulp 3 and gulp 4, we need to effectively fully rewrite all of our gulp scripts (of which we have like a dozen ones; mostly the same, but with minor differences between them), and that takes time that my bosses are not willing to allow right now, in favor of bug fixes and features within the applications themselves. This migration is present "on the todo list", but lower in priority.

You're right, but Bootstrap 4 is already its own thing, so it's not clear what the best way to signify this change would be.

A minor release (e.g. 3.5.0) maybe? The higher the version vector that can afford an increment, the better.

@wadtech
Copy link

wadtech commented Mar 1, 2022

Caught this by coincidence in a project so that would have been an exciting deploy. Please don't use patch releases for breaking changes.

@AprilArcus
Copy link
Contributor

AprilArcus commented Mar 1, 2022

A minor release (e.g. 3.5.0) maybe? The higher the version vector that can afford an increment, the better.

I think that's sensible; what do you think @glebm? Is it possible to revoke 3.4.2 and 3.4.3 from npm and republish as 3.5.0?

I do want to note that a breaking change on a 3.5.0 release would also violate semver, though.

@AprilArcus
Copy link
Contributor

Due to the huge changes between gulp 3 and gulp 4

As I noted upthread, you don't need to upgrade to Gulp 4. Your existing versions will meet your needs, since you have gulp-sass 4.1.1 and gulp-sass supports dart-sass since 4.0.2.

@ptjuanramos
Copy link

A minor release (e.g. 3.5.0) maybe? The higher the version vector that can afford an increment, the better.

I think that's sensible; what do you think @glebm? Is it possible to revoke 3.4.2 and 3.4.3 from npm and republish as 3.5.0?

I do want to note that a breaking change on a 3.5.0 release would also violate semver, though.

No to intrude, but the second option in my opinion makes sense 😃

@boenrobot
Copy link
Author

As I noted upthread, you don't need to upgrade to Gulp 4. Your existing versions will meet your needs, since you have gulp-sass 4.1.1 and gulp-sass supports dart-sass since 4.0.2.

Yes, I got that part. Thank you again. We'll consider doing that.

With the gulp 3 vs gulp 4 thing, I was referring to your bigger comment about Node 10 being EOL.

SCSS is not the only thing we have in our build pipeline. If we "just" switch to Node 12, we need versions of everything in the pipeline that is compatible with Gulp 3 and Node 12. However, several of the other libs we're using in the gulp scripts don't have such versions. They have Gulp 4 compatible versions that also require Node 12+, and their gulp 3 compatible versions only support up to Node 10.

Maybe there's a way to isolate those dependencies and replace them with analogous ones that have better compatibility, similarly to how you did with dart-sass, but trying to do that ourselves also requires time we might as well spend on rewriting to Gulp 4.

@stof
Copy link

stof commented Mar 2, 2022

A solution to keep compatibility with libsass would be to copy the divide() function defined in Bootstrap 4 and 5 and use it instead of math.div

@AprilArcus
Copy link
Contributor

A solution to keep compatibility with libsass would be to copy the divide() function defined in Bootstrap 4 and 5 and use it instead of math.div

That's an interesting idea. Links:

@AprilArcus
Copy link
Contributor

@stof I opened a PR incorporating your suggestion here: #1229

@mrleblanc101
Copy link

mrleblanc101 commented Mar 9, 2022

Wow, this is not very semver.
This include a breaking change, it shouldn't be in a patch release.
The dart-sass math.div change just broke many projects.
Please consider reverting the change and releasing this in a new major/minor version, not in a patch.

@mrleblanc101
Copy link

mrleblanc101 commented Mar 9, 2022

Even using lockfile, this will break project because by default npm/yarn/bower will not lock the patch version of a dependancy. So "bootstrap-sass": "^3.4.1", will update to 3.4.3 and break every project.
If the change is released under 3.5 or 4.0, then it wouldn't be an issue.

@AprilArcus
Copy link
Contributor

by default npm/yarn/bower will not lock the patch version of a dependancy

@mrleblanc101 I think I understand this to mean that if you were to invoke npm install --save bootstrap-sass you would end up with a ^ pin in your package.json. package.json is not the same thing as a lockfile, which provides an additional layer of defense against potentially semver-violating breaking changes. You can learn more about lockfiles at https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json and https://classic.yarnpkg.com/lang/en/docs/yarn-lock/.

@mrleblanc101
Copy link

If I run npm upgrade it shouldn't break the project since breaking change should NEVER be published in patch version ! Also this is used in many old project which use BOWER which doesn't have a lockfile.
Please respect semver like npm.js require for publishing package...

@fahmilatib
Copy link

If the change is released under 3.5 or 4.0, then it wouldn't be an issue

^3.4.1 would still pick up 3.5 unless it's ~3.4.1

@mrleblanc101
Copy link

mrleblanc101 commented Mar 16, 2022

@fahmilatib You are right and since ^ is the default in most package manager (npm/yarn/bower) it should be release as 4.0.0

Can't believe it hasn't been fixed already as this problem is so easy to fix and avoid by RESPECTING semver. https://semver.npmjs.com/

@AprilArcus
Copy link
Contributor

AprilArcus commented Mar 16, 2022

Please don't raise your voice. We all know how Semver works, and we're all doing our best. I'm sure you can see that with Bootstrap 4 being a separate product, the bootstrap-sass maintainers cannot cut an unrelated release of Bootstrap 3 and call it "bootstrap-sass@4.0.0".

@mrleblanc101
Copy link

mrleblanc101 commented Mar 16, 2022

Yes it can. The version of this package has nothing to do with the version of bootstrap it install.
At already has diverged from the latest published version of Bootstrap 3.
And since there will be NEVER be a bootstrap-sass version of Bootstrap 4 because it's not natively compatible with Sass, it doesn't matter.
https://docs.npmjs.com/about-semantic-versioning
Screen Shot 2022-03-16 at 9 43 41 AM

@AprilArcus
Copy link
Contributor

I'm afraid that as I'm not a maintainer and not responsible for selecting the version number published to NPM, I can't help any further.

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

No branches or pull requests

8 participants