[node-build-scripts] feat: use non-legacy Sass compiler, remove node-sass #5904

adidahiya · 2023-01-30T17:15:47Z

Fixes #5334

Changes proposed in this pull request:

This PR completely revamps the sass-compile script in @blueprintjs/node-build-scripts:

We now use the standard async compiler instead of the legacy sync compiler API
The dependency on legacy node-sass custom Sass functions has been removed
- @vgrid/sass-inline-svg (which doesn't work well with dart-sass and fails to run in Node v19.x) has been removed in favor of our own custom SVG inliner implementation
We no longer use the deprecated "node-sass-package-importer"
Instead of the special ~ syntax for importing node modules (which seems to be webpack-specific via sass-loader), we now specify dart-sass includePaths to include nodes_modules paths
- I've implemented this in a way that should work with both Yarn and PNPM, but for now it only has to work with Yarn in this repo

This will unblock upgrading to Node v19.x 🎉

Implementation notes

Various sass import / alias libraries considered:
An alternative approach which may be worthwhile involves a simple sass language function to inline the SVGs rather than a custom function at the compiler level (see https://gist.github.com/adidahiya/74550ebdab2262eff92a4e103f4f8f26).
- Ultimately, I decided to keep the custom function, as it's more flexible and allows for arbitrary icons to be inlined.

Not done in this PR

This PR does not migrate from Sass @import to @use. That will happen in another PR.

Reviewers should focus on:

No regressions in CSS APIs for the following components:

Breadcrumbs
Checkbox
Radio

Screenshot

…ine-svg

packages/core/src/docs/classes.md

packages/node-build-scripts/generate-css-variables.mjs

packages/node-build-scripts/sass/sassCustomFunctions.mjs

packages/webpack-build-scripts/sassNodeModulesLoadPaths.mjs

adidahiya · 2023-02-24T18:52:00Z

export svg inliner, remove math from variables

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

styu · 2023-02-24T19:00:34Z

packages/core/src/common/_variables.scss

@@ -46,11 +55,13 @@ $pt-font-size-large: $pt-grid-size * 1.6 !default;
 $pt-font-size-small: $pt-grid-size * 1.2 !default;

 // a little bit extra to ensure the height comes out to just over 18px (and browser rounds to 18px)
-$pt-line-height: math.div($pt-grid-size * 1.8, $pt-font-size) + 0.0001 !default;


ooc why did you have to drop the math.div ?

get-sass-vars actually interprets and evaluates math for us (kind of surprising when you think about it). when I switched to postcss-simple-vars, we lost that evaluation step. But I think I can actually move back to get-sass-vars; see my other comment

styu · 2023-02-24T19:01:01Z

packages/core/src/components/panel-stack2/_panel-stack2.scss

@@ -2,7 +2,7 @@
 // Licensed under the Apache License, Version 2.0.

 @import "../../common/variables";


ooc should these change to @use at some point?

yeah, at some point...

styu · 2023-02-24T19:01:33Z

packages/core/src/docs/colors.md

@@ -54,7 +54,7 @@ Blueprint provides variables for colors in Sass, Less, and JavaScript.
 Example in Sass:

 ```scss
-@import "~@blueprintjs/core/lib/scss/variables";
+@import "@blueprintjs/core/lib/scss/variables";


should we be recommending @use "@blueprintjs/core/lib/scss/variables" as bp instead?

I will change the recommendation once we switch to @use in this repo

adidahiya · 2023-02-24T19:11:44Z

packages/node-build-scripts/sass/sassVariableParser.mjs

+export const DEFAULT_FLAG = "!default";
+
+/**
+ * Parses Sass source file contents for variables using "postcss-simple-vars"


on second thought, we might be able to stick with get-sass-vars instead of switching to this custom implementation. I think at some point during the long history of this PR, I got convinced that we had to migrate away from that library because of our custom functions, but I think it can continue to work for us. and it seems to be maintained well enough (last update was Jan 2023).

adidahiya · 2023-02-24T19:12:36Z

packages/core/src/common/_variables.scss

@@ -46,11 +55,13 @@ $pt-font-size-large: $pt-grid-size * 1.6 !default;
 $pt-font-size-small: $pt-grid-size * 1.2 !default;

 // a little bit extra to ensure the height comes out to just over 18px (and browser rounds to 18px)
-$pt-line-height: math.div($pt-grid-size * 1.8, $pt-font-size) + 0.0001 !default;


get-sass-vars actually interprets and evaluates math for us (kind of surprising when you think about it). when I switched to postcss-simple-vars, we lost that evaluation step. But I think I can actually move back to get-sass-vars; see my other comment

adidahiya · 2023-02-24T21:33:42Z

Fix math reference

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

WIP fork sass-inline-svg implementation

fcf9c5c

adidahiya added the Domain: tooling label Jan 30, 2023

adidahiya added 7 commits January 30, 2023 12:16

update lockfile

365215f

dedupe lockfile

3e364d8

Attempt to switch to modern sass compiler API

7ae78a9

Merge remote-tracking branch 'origin/develop' into ad/remove-sass-inl…

bc43424

…ine-svg

migrate from node-sass custom functions to postcss-simple-vars

b99fece

drop ~ from import paths, fix sass custom functions

60af867

remove extraneous quotes

c56a76e

adidahiya changed the title ~~[DRAFT] use our own sass-inline-svg implementation~~ [node-build-scripts] feat(sass-compile): use non-legacy compiler, completely remove node-sass Feb 24, 2023

adidahiya changed the title ~~[node-build-scripts] feat(sass-compile): use non-legacy compiler, completely remove node-sass~~ [node-build-scripts] feat: use non-legacy Sass compiler, completely remove node-sass Feb 24, 2023

adidahiya marked this pull request as ready for review February 24, 2023 18:16

adidahiya added 2 commits February 24, 2023 13:20

update lockfile

817cee8

dedupe lockfile

ac5895d

adidahiya mentioned this pull request Feb 24, 2023

svg-icon - (path: (fill: #5f6b7c)) isn't a valid CSS value #5334

Closed

export svg inliner, remove math from variables

1bf332b

adidahiya commented Feb 24, 2023

View reviewed changes

styu reviewed Feb 24, 2023

View reviewed changes

adidahiya commented Feb 24, 2023

View reviewed changes

adidahiya added 3 commits February 24, 2023 16:02

switch back to get-sass-vars

d78169d

update docs

7138b24

Fix math reference

b54c1a7

adidahiya changed the title ~~[node-build-scripts] feat: use non-legacy Sass compiler, completely remove node-sass~~ [node-build-scripts] feat: use non-legacy Sass compiler, remove node-sass Feb 24, 2023

adidahiya merged commit 1ff4cc5 into develop Feb 24, 2023

adidahiya deleted the ad/remove-sass-inline-svg branch February 24, 2023 21:35

adidahiya mentioned this pull request Mar 1, 2023

Regression in box-shadow Sass variable syntax #5987

Closed

adidahiya mentioned this pull request Mar 14, 2023

chore: upgrade to Typescript 4.8 #6021

Merged

adidahiya mentioned this pull request Aug 29, 2023

[node-build-scripts] fix: remove unused 'ts-node' dep #6353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[node-build-scripts] feat: use non-legacy Sass compiler, remove node-sass #5904

[node-build-scripts] feat: use non-legacy Sass compiler, remove node-sass #5904

adidahiya commented Jan 30, 2023 •

edited

Loading

adidahiya commented Feb 24, 2023

styu Feb 24, 2023

adidahiya Feb 24, 2023

styu Feb 24, 2023

adidahiya Feb 24, 2023

styu Feb 24, 2023

adidahiya Feb 24, 2023

adidahiya Feb 24, 2023

adidahiya Feb 24, 2023

adidahiya commented Feb 24, 2023

		@@ -2,7 +2,7 @@
		// Licensed under the Apache License, Version 2.0.

		@import "../../common/variables";

[node-build-scripts] feat: use non-legacy Sass compiler, remove node-sass #5904

[node-build-scripts] feat: use non-legacy Sass compiler, remove node-sass #5904

Conversation

adidahiya commented Jan 30, 2023 • edited Loading

Changes proposed in this pull request:

Implementation notes

Not done in this PR

Reviewers should focus on:

Screenshot

adidahiya commented Feb 24, 2023

export svg inliner, remove math from variables

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented Feb 24, 2023

Fix math reference

adidahiya commented Jan 30, 2023 •

edited

Loading