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

Compilation issue with newer version of SASS #4032

Closed
vkbansal opened this issue Apr 1, 2020 · 20 comments · Fixed by #5216
Closed

Compilation issue with newer version of SASS #4032

vkbansal opened this issue Apr 1, 2020 · 20 comments · Fixed by #5216

Comments

@vkbansal
Copy link

vkbansal commented Apr 1, 2020

When trying to import to blueprint using SASS, error is encountered.

Environment

  • Package version(s):
    • @blueprintjs/core: 3.24.0
    • webpack: 4.42.1
    • sass: 1.26.3
    • sass-loader: 8.0.2
  • Browser and OS versions: MacOS Catalina 10.15.4

Steps to reproduce

  1. Create a SASS file and add @import "~@blueprintjs/core/src/blueprint";
  2. Try compiling it using webpack.

Actual behavior

ModuleBuildError: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: compound selectors may no longer be extended.
Consider `@extend .bp3-menu-item, :hover` instead.
See http://bit.ly/ExtendCompound for details.

   ╷
19 │       @extend .#{$ns}-menu-item:hover;
   │               ^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  node_modules/@blueprintjs/core/src/components/menu/_submenu.scss 19:15  root stylesheet

Expected behavior

There should be no error.

@adidahiya
Copy link
Contributor

From my quick googling it seems like you can work around this by using node-sass instead of sass (Dart-Sass). This feature has not been deprecated in node-sass yet. Longer term it looks like we'll need to refactor some of this Sass code.

@vkbansal
Copy link
Author

vkbansal commented Apr 2, 2020

I tried with node-sass too. Same error.

@DTrejo
Copy link

DTrejo commented Apr 21, 2020

If you add a , , it compiles for sass + parcel at least.
Note that I've yet to actually look at my UI to confirm it looks correct.

node_modules/@blueprintjs/core/src/components/menu/_submenu.scss

-@extend .#{$ns}-menu-item:hover;
+@extend .#{$ns}-menu-item, :hover;

@omeid
Copy link

omeid commented Sep 25, 2020

node-sass is deprecated in favour of sass (dart-sass) in Node 14. So this needs addressing sooner than later.

@adidahiya adidahiya self-assigned this Sep 28, 2020
@nickensoul
Copy link

Hi everyone!

Just remind that node-sass is officially deprecated now. We're using node-sass for now in our project, but will glad to know any plans and news about Blueprint's migration to dart-sass.
Thanks!

@stolenng-investing
Copy link

Hey,
Having this issue also, any plans on resolving it ? i can try also ?

@adidahiya
Copy link
Contributor

This blog post talks about the tradeoffs between the sass versions. It sounds like dart-sass's JS package will slow down our compilation time, but if it's good enough for the bootstrap project, it's probably good enough for us...

I will work on this some time in the next 6-8 weeks, but in the meantime it's open for contributions. I will need to see a performance comparison in this project.

@ChristianIvicevic
Copy link
Contributor

Just wanted to point out that sass doesn't install native dependencies like node-sass does which sometimes causes issues on macOS devices and needs a proper Xcode toolchain. This is one of the reasons I personally prefer sass whenever possible.

@eyalw
Copy link

eyalw commented Mar 18, 2021

Hi, any plans to migrate to sass? : )

@switz
Copy link
Contributor

switz commented Dec 28, 2021

I can't upgrade to node 16 because node-sass isn't compatible with it. Any plans to prioritize the upgrade to sass?

@ksr79
Copy link

ksr79 commented Dec 29, 2021

Would it be an option to reopen and merge #4429, at least the @extend part, although this is included in #4820? node-sass is long outdated, and this is might be the single change that blocks me from switching to dart-sass.

@adidahiya
Copy link
Contributor

I understand the importance of this issue. We're planning to migrate to dart-sass within the next month or two.

@adidahiya adidahiya added P1 and removed P2 labels Jan 4, 2022
@lwouis
Copy link

lwouis commented Feb 15, 2022

@DTrejo

If you add a , , it compiles for sass + parcel at least. Note that I've yet to actually look at my UI to confirm it looks correct.

node_modules/@blueprintjs/core/src/components/menu/_submenu.scss

-@extend .#{$ns}-menu-item:hover;
+@extend .#{$ns}-menu-item, :hover;

This breaks the CSS thus visuals of the MenuItem component. Namely, they have text underline and color of a normal link.

The usage of SASS by Blueprint is quite advanced, so I'm not sure what's going on. It seems to be an issue with CSS selector priority. The attributes from @mixin menu-item are no longer top priority with the change above, and the basic a:hover from Blueprint takes precedence.

@franciscohanna92
Copy link

@adidahiya Any updates on this issue? Are there any time estimations? We are migrating our tooling to vitejs and this is a blocker at the moment since Vite doesn't support node-sass.

@adidahiya
Copy link
Contributor

adidahiya commented Apr 1, 2022

Heads up to anyone in this thread who is importing Blueprint .scss files and compiling the core styles with node-sass as a consumer of the library:

While the ability to customize Blueprint's Sass variables to achieve some basic "theme" variations can be useful, I consider this a feature which uses semi-private APIs. Blueprint's API surface area is already very large, and I do not wish to make its Sass syntax part of the public API. To that end, it is not guaranteed to be protected from breaking changes under semantic versioning.

I plan to migrate Blueprint's Sass code from node-sass to dart-sass (using the work in #4820 and #5216) in a 4.x minor release, likely landing in v4.1.0. Styles and Sass/Less variables will continue to be backwards-compatible, as they have always been public API. But if you are pulling in the @blueprintjs/*/src/**/*.scss files and compiling with node-sass, your build may break after this migration. I suggest protecting yourself from this breakage by using a ~4.0.0 dependency range.

@adidahiya adidahiya modified the milestones: 5.0.0, core v4.1 Apr 1, 2022
@lwouis
Copy link

lwouis commented Apr 2, 2022

@adidahiya if scss variables are not public API, it would be nice to have an official/public way to customize things like colors. A lot of people want to use blueprintjs but they want to brand it a bit by changing the default blue. It would be nice to offer some supported way to do it, like other components libraries do

@adidahiya
Copy link
Contributor

it would be nice to have an official/public way to customize things like colors

@lwouis yep, that would be a nice feature. I'm open to ideas for proposals on how to do this in a clean way that doesn't add too much maintenance burden. We can discuss such proposals in a new issue thread.

@dlech
Copy link
Contributor

dlech commented May 11, 2022

it would be nice to have an official/public way to customize things like colors

@lwouis yep, that would be a nice feature. I'm open to ideas for proposals on how to do this in a clean way that doesn't add too much maintenance burden. We can discuss such proposals in a new issue thread.

I started #5297 to discuss this.

tillprochaska added a commit to alephdata/aleph that referenced this issue Oct 17, 2022
Building Blueprint 4 from the SCSS source requires custom Sass functions [1].

As we’re using react-scripts, we can’t directly modify Webpack loader options. Common workarounds include libraries like react-scripts-rewired, craco, and others. Most of them aren’t actively maintained, but craco’s latest alpha release has support for react-scripts v5, so I’m using this for now. In the mid/long term, I hope to get rid of react-scripts/Webpack and be able to use a simpler build setup.

[1]: palantir/blueprint#4032 (comment)
Rosencrantz pushed a commit to alephdata/aleph that referenced this issue Oct 17, 2022
Building Blueprint 4 from the SCSS source requires custom Sass functions [1].

As we’re using react-scripts, we can’t directly modify Webpack loader options. Common workarounds include libraries like react-scripts-rewired, craco, and others. Most of them aren’t actively maintained, but craco’s latest alpha release has support for react-scripts v5, so I’m using this for now. In the mid/long term, I hope to get rid of react-scripts/Webpack and be able to use a simpler build setup.

[1]: palantir/blueprint#4032 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment