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

The if function is special: should be lazy. #107

Closed
kaj opened this issue May 10, 2021 · 4 comments
Closed

The if function is special: should be lazy. #107

kaj opened this issue May 10, 2021 · 4 comments

Comments

@kaj
Copy link
Owner

kaj commented May 10, 2021

The if function is special in that it's argument should be lazily evaluated. Bootstrap 5 (and probably other common scss code) relies on this.

I've tried running the cmd line on the new bootstrap 5 scss files but I get:

Error: $weight: Expected -20 to be within 0 and 100.
    ,
156 |   @return mix(white, $color, $weight);
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    '
    ../../Downloads/bootstrap-5.0.0/scss/_functions.scss 156:11  import
   ../../Downloads/bootstrap-5.0.0/scss/bootstrap-grid.scss 10:9  root stylesheet
    ,
166 |   @return if($weight > 0, shade-color($color, $weight), tint-color($color, -$weight));
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    '
    ../../Downloads/bootstrap-5.0.0/scss/_functions.scss 166:57  import
    ../../Downloads/bootstrap-5.0.0/scss/bootstrap-grid.scss 10:9  root stylesheet
    ,
291 | $link-hover-color:                        shift-color($link-color, $link-shade-percentage) !default;
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    '
    ../../Downloads/bootstrap-5.0.0/scss/_variables.scss 291:43  import
    ../../Downloads/bootstrap-5.0.0/scss/bootstrap-grid.scss 11:9  root stylesheet

The JS implementation (using dart-sass) from their package.json seem to compile it without errors. Now according to their docs mix should be a value in between 0-100 so the error should be legit.
It would useful to vendor a few common Sass libraries and compile them as additional tests. It also gives an idea of which missing feature is commonly used.

Originally posted by @Keats in #3 (comment)

What happens in the bootstrap compiling error above is that both arguments to the if on line 166 is evaluated (even though only the $if-true one is needed) and evaluating the $if-else value causes an error.

@kaj kaj closed this as completed in 0cae23b May 13, 2021
@kaj
Copy link
Owner Author

kaj commented May 13, 2021

Note: One down, n to go. The issue with the if function is resolved, but rsass still cant handle bootstrap.

kaj added a commit that referenced this issue Jun 1, 2021
Progress: 3727 of 6171 tests passed in dart-sass compatibility mode.

* `value::Unit` and `value::ListSeparator` has new alternatives.
* The `List` alternative in `sass::Value` and `css::Value` is modified.
* The `Use` alternative in `sass::Item` is modified, and `Forward` added.

* Most of `@forward` and some more of `@use` is now supported.
  PR #109 and #110.
* Handle unknown units.  PR #101.
* List can be undecided between beeing comma-separated and beeing
  space-separated.  PR #102.
* Improved parameter handling and returned values of the supported
  selector functions.  PR #103.
* Implement `meta.module_variables` and `meta.module_functions`.
* Implement `math.div` function.
* Improved parameter checking for `hwb`, `alpha`, and `invert`
  functions in `sass:color` module.
* Support slash-separated lists. PR #111.
* The `if` function evaluates its arguments lazily.  Issue #107.
* The `--include-path` cli argument is now named `--load-path`.
* At least some documentation on all public items.
* Update sass-spec test suite to 2021-05-24.  Also include "other"
  files (for `@use` and `@import`) in rust code for the suite.

Tested with rustc 1.52.1, 1.50.0, 1.48.0, 1.45.2,
1.53.0-beta.3 (82b862164 2021-05-22), and
1.54.0-nightly (657bc0188 2021-05-31).
@Keats
Copy link

Keats commented Jun 2, 2021

@kaj I'm running into this again on master with bootstrap 5.0.1

(venv) ~/C/p/rsass (master|…) [2] $ ./target/debug/rsass ~/Downloads/bootstrap-5.0.1/scss/bootstrap.scss
Error: $weight: Expected -20 to be within 0 and 100.
    ,
156 |   @return mix(white, $color, $weight);
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    '
    /Users/vincentprouillet/Downloads/bootstrap-5.0.1/scss/_functions.scss 156:11  import
    /Users/vincentprouillet/Downloads/bootstrap-5.0.1/scss/bootstrap.scss 10:9  root stylesheet
    ,
166 |   @return if($weight > 0, shade-color($color, $weight), tint-color($color, -$weight));
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    '
    /Users/vincentprouillet/Downloads/bootstrap-5.0.1/scss/_functions.scss 166:57  import
    /Users/vincentprouillet/Downloads/bootstrap-5.0.1/scss/bootstrap.scss 10:9  root stylesheet
    ,
291 | $link-hover-color:                        shift-color($link-color, $link-shade-percentage) !default;
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    '
    /Users/vincentprouillet/Downloads/bootstrap-5.0.1/scss/_variables.scss 291:43  import
    /Users/vincentprouillet/Downloads/bootstrap-5.0.1/scss/bootstrap.scss 11:9  root stylesheet

@kaj
Copy link
Owner Author

kaj commented Jun 2, 2021

Strange, I seem to get a different error. Are you sure your ./target/debug/rsass is up to date? Note that a cargo build won't update it unless you have --features=commandline as an argument.

@Keats
Copy link

Keats commented Jun 2, 2021

Sorry I forgot about the flag :(

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

2 participants