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

4.4.0 add function compilation is wrong #29743

Closed
719media opened this issue Nov 26, 2019 · 14 comments · Fixed by #29755, twbs/blog#123 or #29763
Labels

Comments

@719media
Copy link
Contributor

@719media 719media commented Nov 26, 2019

The bootstrap 4.4 add function doesn't quite work right with sass compilation.

As an example, paste the following

@function add($value1, $value2, $return-calc: true) {
  @if $value1 == null {
    @return $value2;
  }

  @if $value2 == null {
    @return $value1;
  }

  @if type-of($value1) == number and type-of($value2) == number and comparable($value1, $value2) {
    @return $value1 + $value2;
  }

  @return if($return-calc == true, calc(#{$value1} + #{$value2}), #{$value1} + #{$value2});
}

$line-height-base:            1.5 !default;
$input-btn-padding-y:         .375rem !default;
$border-width:                1px !default;
$test-height: add($line-height-base * 1em, add($input-btn-padding-y * 2, $border-width, false)) !default;

.test-height {
  height: $test-height;
}

into https://www.sassmeister.com/

You'll notice that the compiled css has some wonkiness:

.test-height {
  height: calc(1.5em + 0.75rem1px);
}

One option is to interpolate the plus sign in the function, like so:
@return if($return-calc == true, calc(#{$value1} + #{$value2}), #{$value1 '+' $value2});

@719media 719media changed the title 4.4 add 4.4 add function compilation is wrong Nov 26, 2019
@ivan-aksamentov

This comment has been minimized.

Copy link

@ivan-aksamentov ivan-aksamentov commented Nov 26, 2019

@719media
I can reproduce that with both sass and node-sass.
In my case webpack PostCSS loader (following after sass loader) receives malformed CSS and errors out the build:

ERROR in ./src/styles/global.scss
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleBuildError: Module build failed (from ./node_modules/postcss-loader/src/index.js):
JisonLexerError: Lexical error on line 1: Unrecognized text.

  Erroneous area:
1: 1.5em + 0.75rem2px
^...........^

Rolling back to 4.3.1 cancels the issue.

P.S. out of curiosity: what words "Erroneous area" could mean in my case?
P.P.S. update: ah, I got it. "Erroneous area" is what is shown between arrows. This rem2 made me think that maybe CSS have recently gotten a way to express surface area somehow.

@brianroadifer

This comment has been minimized.

Copy link

@brianroadifer brianroadifer commented Nov 26, 2019

I noticed this as well when using the custom-select. Looks like it would affect the subtract function as well. Oddly enough the main Bootstrap website has it working.

@mdo

This comment has been minimized.

Copy link
Member

@mdo mdo commented Nov 26, 2019

@nicolomonili

This comment has been minimized.

Copy link

@nicolomonili nicolomonili commented Nov 27, 2019

Same here,

  • macOS
  • Dart Sass 1.23.7
  • bootstrap 4.4

When compile from sources, the generated css for .form-control, .form-control-sm.... classes is wrong

.form-control {
    display: block;
    width: 100%;
    height: calc(1.5em + 0.75rem2px); <---
   ....
}

Schermata 2019-11-27 alle 10 48 06

Schermata 2019-11-27 alle 10 46 19

@MartijnCuppens

This comment has been minimized.

Copy link
Member

@MartijnCuppens MartijnCuppens commented Nov 27, 2019

I can reproduce this with Dart Sass, by running

sass scss/bootstrap.scss dist/css/bootstrap-sass.css

@twbs/team, we probably need to release a patch for this this week

Gonna look into this this evening.

@XhmikosR

This comment has been minimized.

Copy link
Member

@XhmikosR XhmikosR commented Nov 27, 2019

I can release 4.4.1 when ready, just ping me.

Now, even if we did have a dart sass build test, we wouldn't have noticed this either since compilation didn't fail.

I think we should add a note in the docs as a warning about the Sass implementation we use, and if people use another one issues might arise.

@XhmikosR XhmikosR changed the title 4.4 add function compilation is wrong 4.4.0 add function compilation is wrong Nov 27, 2019
@XhmikosR XhmikosR pinned this issue Nov 27, 2019
@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Nov 27, 2019

@twbs/css-review I checked this. #29743 (comment)) is a solution but it doesn't work on ruby-sass - https://www.sassmeister.com/gist/b8f21c26b379106438151b01ffd629ca

Can we ignore the ruby-sass ? because it is already unsupported.

If we concern this, #{$value1} #{'+'} #{$value2} can be used alternative
https://www.sassmeister.com/gist/e4e0a9a44be5abd14b078a254a4d5079.

@XhmikosR

This comment has been minimized.

Copy link
Member

@XhmikosR XhmikosR commented Nov 27, 2019

I think we should keep Ruby Sass support for v4. For v5, and assuming we make it clear in our docs with a huge warning that we only use one Sass implementation and add more info, we can drop support for Ruby Sass.

@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Nov 27, 2019

Sorry,

#29743 (comment)) is a solution but it doesn't work on ruby-sass

is wrong. That support all compiler (dart-dass, libsass and Sass ). I misunderstood the code:
#{$value1} '+' #{$value2}.

@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Nov 28, 2019

OMG! Same issue in the subtract!

dart-sass:

el {
  padding: subtract(1rem, subtract(1em, 1px, false)); // output invalid calc(1rem - 1em-1px);
}
@XhmikosR

This comment has been minimized.

Copy link
Member

@XhmikosR XhmikosR commented Nov 28, 2019

I thought you guys tested it...

Please ping me when there's the final and real fix in a PR because I will need to redo the whole thing.
At least I didn't proceed with releasing 4.4.1 yet :P

@MartijnCuppens

This comment has been minimized.

Copy link
Member

@MartijnCuppens MartijnCuppens commented Nov 28, 2019

Damn it, I thought that would work without spaces. Nice catch, @ysds.

@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Nov 28, 2019

I should have confirmed it, but I overlooked something...

@XhmikosR

This comment has been minimized.

Copy link
Member

@XhmikosR XhmikosR commented Nov 28, 2019

All good, let's just make sure everything is fine with all Sass implementations this time and ping me in the PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.